Clarify the semantics of DfsRefDatabase#compareAndPut

DfsRefDatabase#compareAndPut had a vague semantics for reference
matching. Because of this, an operation to make a symbolic
reference had been broken for some DFS implementations even if they
followed the contract of compareAndPut. The clarified semantics
requires the implementations to satisfy the followings:

* Matching references should be both symbolic references or both
  object ID references.
* If both are symbolic references, both should have the same target
  name.
* If both are object ID references, both should have the same object
  ID.

This semantics is defined based on
https://git.eclipse.org/r/#/c/77416/. Before this commit,
DfsRefDatabase couldn't see the target of symbolic references.

InMemoryRepository is changed to comply with the new semantics. This
semantics change can affect the existing DFS implementations that only
checks object IDs. This commit adds two tests that the previous
InMemoryRepository couldn't pass.

Change-Id: I6c6b5d3cc8241a81f4a37782381c88e8a59fdf15
Signed-off-by: Masaya Suzuki <masayasuzuki@google.com>

org.eclipse.jgit.test/tst/org/eclipse/jgit/junit/TestRepositoryTest.java

@@ -62,6 +62,7 @@
 import org.eclipse.jgit.lib.ObjectLoader;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Ref;
+import org.eclipse.jgit.lib.RefUpdate;
 import org.eclipse.jgit.revwalk.RevBlob;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevObject;
@@ -374,6 +375,44 @@ public void cherryPickWithIdenticalContents() throws Exception {
 		assertEquals(head, repo.exactRef("HEAD").getObjectId());
 	}
 
+	@Test
+	public void reattachToMaster_Race() throws Exception {
+		RevCommit commit = tr.branch("master").commit().create();
+		tr.branch("master").update(commit);
+		tr.branch("other").update(commit);
+		repo.updateRef("HEAD").link("refs/heads/master");
+
+		// Create a detached HEAD that is not an .
+		tr.reset(commit);
+		Ref head = repo.exactRef("HEAD");
+		assertEquals(commit, head.getObjectId());
+		assertFalse(head.isSymbolic());
+
+		// Try to reattach to master.
+		RefUpdate refUpdate = repo.updateRef("HEAD");
+
+		// Make a change during reattachment.
+		repo.updateRef("HEAD").link("refs/heads/other");
+
+		assertEquals(
+				RefUpdate.Result.LOCK_FAILURE, refUpdate.link("refs/heads/master"));
+	}
+
+	@Test
+	public void nonRacingChange() throws Exception {
+		tr.branch("master").update(tr.branch("master").commit().create());
+		tr.branch("other").update(tr.branch("other").commit().create());
+		repo.updateRef("HEAD").link("refs/heads/master");
+
+		// Try to update HEAD.
+		RefUpdate refUpdate = repo.updateRef("HEAD");
+
+		// Proceed a master. This should not affect changing HEAD.
+		tr.branch("master").update(tr.branch("master").commit().create());
+
+		assertEquals(RefUpdate.Result.FORCED, refUpdate.link("refs/heads/other"));
+	}
+
 	private String blobAsString(AnyObjectId treeish, String path)
 			throws Exception {
 		RevObject obj = tr.get(rw.parseTree(treeish), path);

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsRefDatabase.java

@@ -311,6 +311,15 @@ private RefCache read() throws IOException {
 
 	/**
 	 * Compare a reference, and put if it matches.
+	 * <p>
+	 * Two reference match if and only if they satisfy the following:
+	 *
+	 * <ul>
+	 * <li>If one reference is a symbolic ref, the other one should be a symbolic
+	 * ref.
+	 * <li>If both are symbolic refs, the target names should be same.
+	 * <li>If both are object ID refs, the object IDs should be same.
+	 * </ul>
 	 *
 	 * @param oldRef
 	 *            old value to compare to. If the reference is expected to not

org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java

@@ -24,7 +24,6 @@
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.Ref.Storage;
 import org.eclipse.jgit.lib.RefDatabase;
-import org.eclipse.jgit.lib.SymbolicRef;
 import org.eclipse.jgit.revwalk.RevObject;
 import org.eclipse.jgit.revwalk.RevTag;
 import org.eclipse.jgit.revwalk.RevWalk;
@@ -401,27 +400,8 @@ protected boolean compareAndPut(Ref oldRef, Ref newRef)
 					return refs.putIfAbsent(name, newRef) == null;
 
 				Ref cur = refs.get(name);
-				Ref toCompare = cur;
-				if (toCompare != null) {
-					if (toCompare.isSymbolic()) {
-						// Arm's-length dereference symrefs before the compare, since
-						// DfsRefUpdate#doLink(String) stores them undereferenced.
-						Ref leaf = toCompare.getLeaf();
-						if (leaf.getObjectId() == null) {
-							leaf = refs.get(leaf.getName());
-							if (leaf.isSymbolic())
-								// Not supported at the moment.
-								throw new IllegalArgumentException();
-							toCompare = new SymbolicRef(
-									name,
-									new ObjectIdRef.Unpeeled(
-											Storage.NEW,
-											leaf.getName(),
-											leaf.getObjectId()));
-						} else
-							toCompare = toCompare.getLeaf();
-					}
-					if (eq(toCompare, oldRef))
+				if (cur != null) {
+					if (eq(cur, oldRef))
 						return refs.replace(name, cur, newRef);
 				}
 
@@ -452,10 +432,12 @@ protected boolean compareAndRemove(Ref oldRef) throws IOException {
 		private boolean eq(Ref a, Ref b) {
 			if (!Objects.equals(a.getName(), b.getName()))
 				return false;
-			// Compare leaf object IDs, since the oldRef passed into compareAndPut
-			// when detaching a symref is an ObjectIdRef.
-			return Objects.equals(a.getLeaf().getObjectId(),
-					b.getLeaf().getObjectId());
+			if (a.isSymbolic() != b.isSymbolic())
+				return false;
+			if (a.isSymbolic())
+				return Objects.equals(a.getTarget().getName(), b.getTarget().getName());
+			else
+				return Objects.equals(a.getObjectId(), b.getObjectId());
 		}
 	}
 }