Skip to main content
aboutsummaryrefslogtreecommitdiffstats
diff options
context:
space:
mode:
authorMasaya Suzuki2016-08-26 01:25:48 +0000
committerMasaya Suzuki2016-08-26 02:12:39 +0000
commit1227165e27c589a5002840b5fd88f0bfc8a6433b (patch)
treef2c78a83ef8cd96e9250a1ba176c0a5a439b14d5
parent1836a7b273b63d6e217e5b58322cb1a079517101 (diff)
downloadjgit-1227165e27c589a5002840b5fd88f0bfc8a6433b.tar.gz
jgit-1227165e27c589a5002840b5fd88f0bfc8a6433b.tar.xz
jgit-1227165e27c589a5002840b5fd88f0bfc8a6433b.zip
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>
-rw-r--r--org.eclipse.jgit.test/tst/org/eclipse/jgit/junit/TestRepositoryTest.java39
-rw-r--r--org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/DfsRefDatabase.java9
-rw-r--r--org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/dfs/InMemoryRepository.java34
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());
}
}
}

Back to the top