From f529fa6729a9b8b5c1329bf75639d26b0067f9d9 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 7 Jul 2017 11:24:51 -0400 Subject: [PATCH] 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 --- .../internal/storage/file/RefUpdateTest.java | 69 +++++++++++++++++-- .../internal/storage/file/RefDirectory.java | 3 + .../src/org/eclipse/jgit/lib/RefUpdate.java | 10 +++ 3 files changed, 78 insertions(+), 4 deletions(-) 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 daef91d53..e4e5d28f7 100644 --- 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 e03bd8723..c8c2dd586 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 @@ -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 fc334f027..61fda943c 100644 --- 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. *