Fix deleting symrefs

The RefDirectory implementation of doDelete never considered whether to
delete a symref or its leaf, because the detachingSymbolicRef bit was
never exposed from RefUpdate. The behavior was thus incorrectly to
always delete the symref, never the leaf.

There was no test for this behavior. The only thing that attempted to be
a test was testDeleteHeadInBareRepo, but this test was broken for
reasons unrelated to this bug. Specifically, it set the leaf to point to
a completely nonexistent object, and then asserted that deleting HEAD
resulted in NO_CHANGE. The only reason this test ever passed is because
of a quirk of updateImpl, which treats a missing object as the same as
null. This quirk aside, the test wasn't really testing the right thing.
Turn this into a real test by writing out a real object and pointing the
leaf at that.

Also, add a test for the detachingSymbolicRef case, i.e. deleting the
symref and leaving the leaf alone.

Change-Id: Ib96d2a35b4f99eba0734725486085fc6f9d78aa5

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java
index daef91d53cbff1368f9bf82883493388aa3890bb..e4e5d28f7d8ee3b075b026af55c5a2a594ac64ed 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefUpdateTest.java
@@ -45,6 +45,7 @@
 
 package org.eclipse.jgit.internal.storage.file;
 
+import static java.nio.charset.StandardCharsets.UTF_8;
 import static org.eclipse.jgit.junit.Assert.assertEquals;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
@@ -64,6 +65,7 @@ import java.util.Map.Entry;
 import org.eclipse.jgit.lib.AnyObjectId;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.PersonIdent;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefRename;
@@ -240,14 +242,73 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase {
        @Test
        public void testDeleteHeadInBareRepo() throws IOException {
                Repository bareRepo = createBareRepository();
+               String master = "refs/heads/master";
+               Ref head = bareRepo.exactRef(Constants.HEAD);
+               assertNotNull(head);
+               assertTrue(head.isSymbolic());
+               assertEquals(master, head.getLeaf().getName());
+               assertNull(head.getObjectId());
+               assertNull(bareRepo.exactRef(master));
+
+               ObjectId blobId;
+               try (ObjectInserter ins = bareRepo.newObjectInserter()) {
+                       blobId = ins.insert(Constants.OBJ_BLOB, "contents".getBytes(UTF_8));
+                       ins.flush();
+               }
+
+               // Create master via HEAD, so we delete it.
                RefUpdate ref = bareRepo.updateRef(Constants.HEAD);
-               ref.setNewObjectId(ObjectId
-                               .fromString("0123456789012345678901234567890123456789"));
-               // Create the HEAD ref so we can delete it.
+               ref.setNewObjectId(blobId);
                assertEquals(Result.NEW, ref.update());
+
+               head = bareRepo.exactRef(Constants.HEAD);
+               assertTrue(head.isSymbolic());
+               assertEquals(master, head.getLeaf().getName());
+               assertEquals(blobId, head.getLeaf().getObjectId());
+               assertEquals(blobId, bareRepo.exactRef(master).getObjectId());
+
+               // Unlike in a non-bare repo, deleting the HEAD is allowed, and leaves HEAD
+               // back in a dangling state.
                ref = bareRepo.updateRef(Constants.HEAD);
-               delete(bareRepo, ref, Result.NO_CHANGE, true, true);
+               ref.setExpectedOldObjectId(blobId);
+               ref.setForceUpdate(true);
+               delete(bareRepo, ref, Result.FORCED, true, true);
+
+               head = bareRepo.exactRef(Constants.HEAD);
+               assertNotNull(head);
+               assertTrue(head.isSymbolic());
+               assertEquals(master, head.getLeaf().getName());
+               assertNull(head.getObjectId());
+               assertNull(bareRepo.exactRef(master));
+       }
+
+       @Test
+       public void testDeleteSymref() throws IOException {
+               RefUpdate dst = updateRef("refs/heads/abc");
+               assertEquals(Result.NEW, dst.update());
+               ObjectId id = dst.getNewObjectId();
+
+               RefUpdate u = db.updateRef("refs/symref");
+               assertEquals(Result.NEW, u.link(dst.getName()));
+
+               Ref ref = db.exactRef(u.getName());
+               assertNotNull(ref);
+               assertTrue(ref.isSymbolic());
+               assertEquals(dst.getName(), ref.getLeaf().getName());
+               assertEquals(id, ref.getLeaf().getObjectId());
+
+               u = db.updateRef(u.getName());
+               u.setDetachingSymbolicRef();
+               u.setForceUpdate(true);
+               assertEquals(Result.FORCED, u.delete());
+
+               assertNull(db.exactRef(u.getName()));
+               ref = db.exactRef(dst.getName());
+               assertNotNull(ref);
+               assertFalse(ref.isSymbolic());
+               assertEquals(id, ref.getObjectId());
        }
+
        /**
         * Delete a loose ref and make sure the directory in refs is deleted too,
         * and the reflog dir too

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 e03bd8723f53000ccd5664ff6438a46ad6ff31de..c8c2dd586719f9f13d9cb33ec06f2b71e5ab6948 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
@@ -580,6 +580,9 @@ public class RefDirectory extends RefDatabase {
 
        void delete(RefDirectoryUpdate update) throws IOException {
                Ref dst = update.getRef();
+               if (!update.isDetachingSymbolicRef()) {
+                       dst = dst.getLeaf();
+               }
                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/lib/RefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java
index fc334f0275fb1d91f4b3db0ab03b0daf5c576d8e..61fda943c0e24108b014870aac5ba6390fb9cbdf 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java
@@ -277,6 +277,16 @@ public abstract class RefUpdate {
                detachingSymbolicRef = true;
        }
 
+       /**
+        * Return whether this update is actually detaching a symbolic ref.
+        *
+        * @return true if detaching a symref.
+        * @since 4.9
+        */
+       public boolean isDetachingSymbolicRef() {
+               return detachingSymbolicRef;
+       }
+
        /**
         * Set the new value the ref will update to.
         *