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 daef91d..e4e5d28 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 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 @@
@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 e03bd87..c8c2dd5 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 @@
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 fc334f0..61fda94 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/RefUpdate.java
@@ -278,6 +278,16 @@
}
/**
+ * 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.
*
* @param id
