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>

3 files changed, 56 insertions, 26 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/junit/TestRepositoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/junit/TestRepositoryTest.java
index 8b54dabce3..b7027f3272 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/junit/TestRepositoryTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/junit/TestRepositoryTest.java
@@ -62,6 +62,7 @@ import org.eclipse.jgit.lib.ObjectId;
 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 class TestRepositoryTest {
 		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);
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsRefDatabase.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsRefDatabase.java
index 5611d23be7..4ddcec1673 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsRefDatabase.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsRefDatabase.java
@@ -311,6 +311,15 @@ public abstract class DfsRefDatabase extends RefDatabase {
 
 	/**
 	 * 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
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java
index 2e1c90d3ce..6f390a4b3b 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java
@@ -24,7 +24,6 @@ import org.eclipse.jgit.lib.ProgressMonitor;
 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 @@ public class InMemoryRepository extends DfsRepository {
 					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 @@ public class InMemoryRepository extends DfsRepository {
 		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());
 		}
 	}
 }