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 <masayasuzuki@google.com>

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 e5469f6b8388b9a4640c39e4d3fe25d177beb1a9..5611d23be740c26e97fce29827bb1511836b32e2 100644 (file)
--- 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 e5ca7368245f8439a2f39db53b854d6d76461945..108065913a6d3b34fd824ad41115e9d2c183c76d 100644 (file)
--- 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 0d16f79ea14cec568f5d224259ba827c30d62617..3c1916b6420f75d34dd83c8cd88e325259add721 100644 (file)
--- 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 c1027f0f75d528f684dbc33e4111143da4ca66a0..fc334f0275fb1d91f4b3db0ab03b0daf5c576d8e 100644 (file)
--- 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;