From 3e27fb37196bd43ba626235850af8fa684e23e1f Mon Sep 17 00:00:00 2001 From: Masaya Suzuki Date: Wed, 13 Jul 2016 16:57:03 -0700 Subject: [PATCH] Do not fake a SymbolicRef as an ObjectIdRef When doing a detaching operation, JGit fakes a SymbolicRef as an ObjectIdRef. This is because RefUpdate#updateImpl dereferences the SymbolicRef when updating it. For example, assume that HEAD is pointing to refs/heads/master. If I try to make a detached HEAD pointing to a commit c0ffee, RefUpdate dereferences HEAD as refs/heads/master first and changes refs/heads/master to c0ffee. The detach argument of RefDatabase#newUpdate avoids this dereference by faking HEAD as ObjectIdRef. This faking is problematic for the linking operation of DfsRefDatabase. It does a compare-and-swap operation on every reference change because of its distributed systems nature. If a SymbolicRef is faked as an ObjectRef, it thinks that there is a racing change in the reference and rejects the update. Because of this, DFS based repositories cannot change the link target of symbolic refs. This has not been a problem for file-based repositories because they have a file-lock based semantics instead of the CAS based one. The reference implementation, InMemoryRepository, is not affected because it only compares ObjectIds. When [1] introduced this faking code, there was no way for RefUpdate to distinguish the detaching operation. When [2] fixed the detaching operation, it introduced a detachingSymbolicRef flag. This commit uses this flag to control whether it needs to dereference the symbolic refs by calling Ref#getLeaf. The same flag is used in the reflog update operation. This commit does not affect any operation that succeeds currently. In some DFS repository implementations, this fixes a ref linking operation, which is currently failing. [1]: https://eclipse.googlesource.com/jgit/jgit/+/01b5392cdbc12ce2e21fd1d1afbd61fdf97e1c38 [2]: https://eclipse.googlesource.com/jgit/jgit/+/3a86868c0883d2a564db88bf9ae4a5fe235bb63f Change-Id: I118f85f0414dbfad02250944e28d74dddd59469b Signed-off-by: Masaya Suzuki --- .../jgit/internal/storage/dfs/DfsRefDatabase.java | 4 ---- .../eclipse/jgit/internal/storage/file/RefDirectory.java | 4 +--- .../jgit/internal/storage/file/RefDirectoryUpdate.java | 6 ++++-- org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java | 9 +++++++-- 4 files changed, 12 insertions(+), 11 deletions(-) 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 e5469f6b8..5611d23be 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 @@ -217,10 +217,6 @@ public abstract class DfsRefDatabase extends RefDatabase { else detachingSymbolicRef = detach && ref.isSymbolic(); - if (detachingSymbolicRef) { - ref = new ObjectIdRef.Unpeeled(NEW, refName, ref.getObjectId()); - } - DfsRefUpdate update = new DfsRefUpdate(this, ref); if (detachingSymbolicRef) update.setDetachingSymbolicRef(); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java index e5ca73682..108065913 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectory.java @@ -545,8 +545,6 @@ public class RefDirectory extends RefDatabase { ref = new ObjectIdRef.Unpeeled(NEW, name, null); else { detachingSymbolicRef = detach && ref.isSymbolic(); - if (detachingSymbolicRef) - ref = new ObjectIdRef.Unpeeled(LOOSE, name, ref.getObjectId()); } RefDirectoryUpdate refDirUpdate = new RefDirectoryUpdate(this, ref); if (detachingSymbolicRef) @@ -579,7 +577,7 @@ public class RefDirectory extends RefDatabase { } void delete(RefDirectoryUpdate update) throws IOException { - Ref dst = update.getRef().getLeaf(); + Ref dst = update.getRef(); String name = dst.getName(); // Write the packed-refs file using an atomic update. We might diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java index 0d16f79ea..3c1916b64 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/file/RefDirectoryUpdate.java @@ -56,6 +56,7 @@ import org.eclipse.jgit.lib.Repository; class RefDirectoryUpdate extends RefUpdate { private final RefDirectory database; + private boolean shouldDeref; private LockFile lock; RefDirectoryUpdate(final RefDirectory r, final Ref ref) { @@ -75,6 +76,7 @@ class RefDirectoryUpdate extends RefUpdate { @Override protected boolean tryLock(boolean deref) throws IOException { + shouldDeref = deref; Ref dst = getRef(); if (deref) dst = dst.getLeaf(); @@ -117,7 +119,7 @@ class RefDirectoryUpdate extends RefUpdate { msg = strResult; } } - database.log(this, msg, true); + database.log(this, msg, shouldDeref); } if (!lock.commit()) return Result.LOCK_FAILURE; @@ -140,7 +142,7 @@ class RefDirectoryUpdate extends RefUpdate { @Override protected Result doDelete(final Result status) throws IOException { - if (getRef().getLeaf().getStorage() != Ref.Storage.NEW) + if (getRef().getStorage() != Ref.Storage.NEW) database.delete(this); return status; } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java index c1027f0f7..fc334f027 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java @@ -551,7 +551,9 @@ public abstract class RefUpdate { * @throws IOException */ public Result delete(final RevWalk walk) throws IOException { - final String myName = getRef().getLeaf().getName(); + final String myName = detachingSymbolicRef + ? getRef().getName() + : getRef().getLeaf().getName(); if (myName.startsWith(Constants.R_HEADS) && !getRepository().isBare()) { // Don't allow the currently checked out branch to be deleted. Ref head = getRefDatabase().getRef(Constants.HEAD); @@ -628,7 +630,10 @@ public abstract class RefUpdate { if (oldValue == null && checkConflicting && getRefDatabase().isNameConflicting(getName())) return Result.LOCK_FAILURE; try { - if (!tryLock(true)) + // If we're detaching a symbolic reference, we should update the reference + // itself. Otherwise, we will update the leaf reference, which should be + // an ObjectIdRef. + if (!tryLock(!detachingSymbolicRef)) return Result.LOCK_FAILURE; if (expValue != null) { final ObjectId o;