From 00a72e22e6d5a4416b8f2484643d25c94d805c96 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 5 Jul 2017 13:45:25 -0400 Subject: BatchRefUpdate: Document when getPushOptions is null Change-Id: I4cccda0ec3a8598edb723dc49101a16d603d1e82 --- org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java index 3043d4fcda..3c5ecfb1c8 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/lib/BatchRefUpdate.java @@ -58,6 +58,7 @@ import java.util.HashSet; import java.util.List; import java.util.concurrent.TimeoutException; +import org.eclipse.jgit.annotations.Nullable; import org.eclipse.jgit.internal.JGitText; import org.eclipse.jgit.lib.RefUpdate.Result; import org.eclipse.jgit.revwalk.RevWalk; @@ -323,9 +324,11 @@ public class BatchRefUpdate { /** * Gets the list of option strings associated with this update. * - * @return pushOptions + * @return push options that were passed to {@link #execute}; prior to calling + * {@link #execute}, always returns null. * @since 4.5 */ + @Nullable public List getPushOptions() { return pushOptions; } -- cgit v1.2.3 From 21ec281f3e53e72edd77ea3ef142c953ebe0c465 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 5 Jul 2017 14:07:17 -0400 Subject: ReceiveCommand: Explicitly check constructor preconditions Some downstream code checks whether a ReceiveCommand is a create or a delete based on the type field. Other downstream code (in particular a good chunk of Gerrit code I wrote) checks the same thing by comparing oldId/newId to zeroId. Unfortunately, there were no strict checks in the constructor that ensures that zeroId is only set for oldId/newId if the type argument corresponds, so a caller that passed mismatched IDs and types would observe completely undefined behavior as a result. This is and always has been a misuse of the API; throw IllegalArgumentException so the caller knows that it is a misuse. Similarly, throw from the constructor if oldId/newId are null. The non-nullness requirement was already documented. Fix RefDirectoryTest to not do the wrong thing. Change-Id: Ie2d0bfed8a2d89e807a41925d548f0f0ce243ecf --- .../internal/storage/file/RefDirectoryTest.java | 31 ++++++-------- .../org/eclipse/jgit/internal/JGitText.properties | 6 +++ .../src/org/eclipse/jgit/internal/JGitText.java | 6 +++ .../org/eclipse/jgit/transport/ReceiveCommand.java | 47 ++++++++++++++++++++-- 4 files changed, 68 insertions(+), 22 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java index 53db123d34..97130f288b 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/RefDirectoryTest.java @@ -46,6 +46,7 @@ package org.eclipse.jgit.internal.storage.file; import static org.eclipse.jgit.lib.Constants.HEAD; import static org.eclipse.jgit.lib.Constants.R_HEADS; import static org.eclipse.jgit.lib.Constants.R_TAGS; +import static org.eclipse.jgit.lib.ObjectId.zeroId; import static org.eclipse.jgit.lib.Ref.Storage.LOOSE; import static org.eclipse.jgit.lib.Ref.Storage.NEW; import static org.junit.Assert.assertEquals; @@ -83,7 +84,6 @@ import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.revwalk.RevTag; import org.eclipse.jgit.revwalk.RevWalk; import org.eclipse.jgit.transport.ReceiveCommand; -import org.eclipse.jgit.transport.ReceiveCommand.Type; import org.junit.Before; import org.junit.Test; @@ -1297,9 +1297,9 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase { writeLooseRef("refs/heads/master", A); writeLooseRef("refs/heads/masters", B); List commands = Arrays.asList( - newCommand(A, B, "refs/heads/master", + new ReceiveCommand(A, B, "refs/heads/master", ReceiveCommand.Type.UPDATE), - newCommand(B, A, "refs/heads/masters", + new ReceiveCommand(B, A, "refs/heads/masters", ReceiveCommand.Type.UPDATE_NONFASTFORWARD)); BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); batchUpdate.addCommand(commands); @@ -1319,9 +1319,9 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase { writeLooseRef("refs/heads/master", A); writeLooseRef("refs/heads/masters", B); List commands = Arrays.asList( - newCommand(A, B, "refs/heads/master", + new ReceiveCommand(A, B, "refs/heads/master", ReceiveCommand.Type.UPDATE), - newCommand(B, A, "refs/heads/masters", + new ReceiveCommand(B, A, "refs/heads/masters", ReceiveCommand.Type.UPDATE_NONFASTFORWARD)); BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); batchUpdate.setAllowNonFastForwards(true); @@ -1341,7 +1341,7 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase { throws IOException { writeLooseRef("refs/heads/master", B); List commands = Arrays.asList( - newCommand(B, A, "refs/heads/master", + new ReceiveCommand(B, A, "refs/heads/master", ReceiveCommand.Type.UPDATE_NONFASTFORWARD)); BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); batchUpdate.setAllowNonFastForwards(true); @@ -1362,11 +1362,12 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase { writeLooseRef("refs/heads/master", A); writeLooseRef("refs/heads/masters", B); List commands = Arrays.asList( - newCommand(A, B, "refs/heads/master", + new ReceiveCommand(A, B, "refs/heads/master", ReceiveCommand.Type.UPDATE), - newCommand(null, A, "refs/heads/master/x", + new ReceiveCommand(zeroId(), A, "refs/heads/master/x", ReceiveCommand.Type.CREATE), - newCommand(null, A, "refs/heads", ReceiveCommand.Type.CREATE)); + new ReceiveCommand(zeroId(), A, "refs/heads", + ReceiveCommand.Type.CREATE)); BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); batchUpdate.setAllowNonFastForwards(true); batchUpdate.addCommand(commands); @@ -1389,11 +1390,11 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase { writeLooseRef("refs/heads/master", A); writeLooseRef("refs/heads/masters", B); List commands = Arrays.asList( - newCommand(A, B, "refs/heads/master", + new ReceiveCommand(A, B, "refs/heads/master", ReceiveCommand.Type.UPDATE), - newCommand(null, A, "refs/heads/masters/x", + new ReceiveCommand(zeroId(), A, "refs/heads/masters/x", ReceiveCommand.Type.CREATE), - newCommand(B, null, "refs/heads/masters", + new ReceiveCommand(B, zeroId(), "refs/heads/masters", ReceiveCommand.Type.DELETE)); BatchRefUpdate batchUpdate = refdir.newBatchUpdate(); batchUpdate.setAllowNonFastForwards(true); @@ -1408,12 +1409,6 @@ public class RefDirectoryTest extends LocalDiskRepositoryTestCase { assertEquals(A.getId(), refs.get("refs/heads/masters/x").getObjectId()); } - private static ReceiveCommand newCommand(RevCommit a, RevCommit b, - String string, Type update) { - return new ReceiveCommand(a != null ? a.getId() : null, - b != null ? b.getId() : null, string, update); - } - private void writeLooseRef(String name, AnyObjectId id) throws IOException { writeLooseRef(name, id.name() + "\n"); } diff --git a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties index 627007dc04..6e793dab75 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -208,12 +208,14 @@ corruptPack=Pack file {0} is corrupt, removing it from pack list createBranchFailedUnknownReason=Create branch failed for unknown reason createBranchUnexpectedResult=Create branch returned unexpected result {0} createNewFileFailed=Could not create new file {0} +createRequiresZeroOldId=Create requires old ID to be zero credentialPassword=Password credentialUsername=Username daemonAlreadyRunning=Daemon already running daysAgo={0} days ago deleteBranchUnexpectedResult=Delete branch returned unexpected result {0} deleteFileFailed=Could not delete file {0} +deleteRequiresZeroNewId=Delete requires new ID to be zero deleteTagUnexpectedResult=Delete tag returned unexpected result {0} deletingNotSupported=Deleting {0} not supported. destinationIsNotAWildcard=Destination is not a wildcard. @@ -242,6 +244,7 @@ encryptionError=Encryption error: {0} encryptionOnlyPBE=Encryption error: only password-based encryption (PBE) algorithms are supported. endOfFileInEscape=End of file in escape entryNotFoundByPath=Entry not found by path: {0} +enumValueNotSupported0=Invalid value: {0} enumValueNotSupported2=Invalid value: {0}.{1}={2} enumValueNotSupported3=Invalid value: {0}.{1}.{2}={3} enumValuesNotAvailable=Enumerated values of type {0} not available @@ -425,6 +428,7 @@ need2Arguments=Need 2 arguments needPackOut=need packOut needsAtLeastOneEntry=Needs at least one entry needsWorkdir=Needs workdir +newIdMustNotBeNull=New ID must not be null newlineInQuotesNotAllowed=Newline in quotes not allowed noApplyInDelete=No apply in delete noClosingBracket=No closing {0} found for {1} at index {2}. @@ -458,6 +462,7 @@ objectNotFound=Object {0} not found. objectNotFoundIn=Object {0} not found in {1}. obtainingCommitsForCherryPick=Obtaining commits that need to be cherry-picked offsetWrittenDeltaBaseForObjectNotFoundInAPack=Offset-written delta base for object not found in a pack +oldIdMustNotBeNull=Expected old ID must not be null onlyAlreadyUpToDateAndFastForwardMergesAreAvailable=only already-up-to-date and fast forward merges are available onlyOneFetchSupported=Only one fetch supported onlyOneOperationCallPerConnectionIsSupported=Only one operation call per connection is supported. @@ -684,6 +689,7 @@ unsupportedOperationNotAddAtEnd=Not add-at-end: {0} unsupportedPackIndexVersion=Unsupported pack index version {0} unsupportedPackVersion=Unsupported pack version {0}. unsupportedRepositoryDescription=Repository description not supported +updateRequiresOldIdAndNewId=Update requires both old ID and new ID to be nonzero updatingHeadFailed=Updating HEAD failed updatingReferences=Updating references updatingRefFailed=Updating the ref {0} to {1} failed. ReturnCode from RefUpdate.update() was {2} diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java index 2ba3b8fd1c..ea752b950d 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -267,12 +267,14 @@ public class JGitText extends TranslationBundle { /***/ public String createBranchFailedUnknownReason; /***/ public String createBranchUnexpectedResult; /***/ public String createNewFileFailed; + /***/ public String createRequiresZeroOldId; /***/ public String credentialPassword; /***/ public String credentialUsername; /***/ public String daemonAlreadyRunning; /***/ public String daysAgo; /***/ public String deleteBranchUnexpectedResult; /***/ public String deleteFileFailed; + /***/ public String deleteRequiresZeroNewId; /***/ public String deleteTagUnexpectedResult; /***/ public String deletingNotSupported; /***/ public String destinationIsNotAWildcard; @@ -301,6 +303,7 @@ public class JGitText extends TranslationBundle { /***/ public String encryptionOnlyPBE; /***/ public String endOfFileInEscape; /***/ public String entryNotFoundByPath; + /***/ public String enumValueNotSupported0; /***/ public String enumValueNotSupported2; /***/ public String enumValueNotSupported3; /***/ public String enumValuesNotAvailable; @@ -484,6 +487,7 @@ public class JGitText extends TranslationBundle { /***/ public String needPackOut; /***/ public String needsAtLeastOneEntry; /***/ public String needsWorkdir; + /***/ public String newIdMustNotBeNull; /***/ public String newlineInQuotesNotAllowed; /***/ public String noApplyInDelete; /***/ public String noClosingBracket; @@ -517,6 +521,7 @@ public class JGitText extends TranslationBundle { /***/ public String objectNotFoundIn; /***/ public String obtainingCommitsForCherryPick; /***/ public String offsetWrittenDeltaBaseForObjectNotFoundInAPack; + /***/ public String oldIdMustNotBeNull; /***/ public String onlyAlreadyUpToDateAndFastForwardMergesAreAvailable; /***/ public String onlyOneFetchSupported; /***/ public String onlyOneOperationCallPerConnectionIsSupported; @@ -743,6 +748,7 @@ public class JGitText extends TranslationBundle { /***/ public String unsupportedPackIndexVersion; /***/ public String unsupportedPackVersion; /***/ public String unsupportedRepositoryDescription; + /***/ public String updateRequiresOldIdAndNewId; /***/ public String updatingHeadFailed; /***/ public String updatingReferences; /***/ public String updatingRefFailed; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java index 2b21c4a8fe..a3f75010eb 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ReceiveCommand.java @@ -210,7 +210,7 @@ public class ReceiveCommand { * Create a new command for {@link BaseReceivePack}. * * @param oldId - * the old object id; must not be null. Use + * the expected old object id; must not be null. Use * {@link ObjectId#zeroId()} to indicate a ref creation. * @param newId * the new object id; must not be null. Use @@ -220,15 +220,23 @@ public class ReceiveCommand { */ public ReceiveCommand(final ObjectId oldId, final ObjectId newId, final String name) { + if (oldId == null) { + throw new IllegalArgumentException(JGitText.get().oldIdMustNotBeNull); + } + if (newId == null) { + throw new IllegalArgumentException(JGitText.get().newIdMustNotBeNull); + } this.oldId = oldId; this.newId = newId; this.name = name; type = Type.UPDATE; - if (ObjectId.zeroId().equals(oldId)) + if (ObjectId.zeroId().equals(oldId)) { type = Type.CREATE; - if (ObjectId.zeroId().equals(newId)) + } + if (ObjectId.zeroId().equals(newId)) { type = Type.DELETE; + } } /** @@ -243,14 +251,45 @@ public class ReceiveCommand { * @param name * name of the ref being affected. * @param type - * type of the command. + * type of the command. Must be {@link Type#CREATE} if {@code + * oldId} is zero, or {@link Type#DELETE} if {@code newId} is zero. * @since 2.0 */ public ReceiveCommand(final ObjectId oldId, final ObjectId newId, final String name, final Type type) { + if (oldId == null) { + throw new IllegalArgumentException(JGitText.get().oldIdMustNotBeNull); + } + if (newId == null) { + throw new IllegalArgumentException(JGitText.get().newIdMustNotBeNull); + } this.oldId = oldId; this.newId = newId; this.name = name; + switch (type) { + case CREATE: + if (!ObjectId.zeroId().equals(oldId)) { + throw new IllegalArgumentException( + JGitText.get().createRequiresZeroOldId); + } + break; + case DELETE: + if (!ObjectId.zeroId().equals(newId)) { + throw new IllegalArgumentException( + JGitText.get().deleteRequiresZeroNewId); + } + break; + case UPDATE: + case UPDATE_NONFASTFORWARD: + if (ObjectId.zeroId().equals(newId) + || ObjectId.zeroId().equals(oldId)) { + throw new IllegalArgumentException( + JGitText.get().updateRequiresOldIdAndNewId); + } + break; + default: + throw new IllegalStateException(JGitText.get().enumValueNotSupported0); + } this.type = type; } -- cgit v1.2.3 From 9c33f7364d41956240818ba12d8b79d5ea846162 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Wed, 5 Jul 2017 14:19:36 -0400 Subject: RefDirectory: Throw exception if CAS of packed ref list fails The contents of the packedRefList AtomicReference should never differ from what we expect prior to writing, because this segment of the code is protected by the packed-refs lock file on disk. If it does happen, whether due to programmer error or a rogue process not respecting the locking protocol, it's better to let the caller know than to silently drop the whole commit operation on the floor. The existing concurrentOnlyOneWritesPackedRefs test is inherently nondeterministic as written, and was already about 6% flaky as measured by bazel: $ bazel test --runs_per_test=200 //org.eclipse.jgit.test:org_eclipse_jgit_internal_storage_file_GcPackRefsTest ... INFO: Elapsed time: 42.608s, Critical Path: 10.35s //org.eclipse.jgit.test:org_eclipse_jgit_internal_storage_file_GcPackRefsTest FAILED in 12 out of 200 in 1.6s Stats over 200 runs: max = 1.6s, min = 1.1s, avg = 1.3s, dev = 0.1s This flakiness was caused by the assumption that exactly one of the 2 threads would fail, when both might actually succeed in practice due to racing on the compare-and-swap. For whatever reason, this change affected the interleaving behavior in such a way that the flakiness jumped to around 50%. Making the interleaving of the test fully deterministic is beyond the scope of this change, but a simple tweak to the assertion is enough to make it pass consistently 200+ times both before and after this change. Change-Id: I5ff4dc39ee05bda88d47909acb70118f3d0c8f74 --- .../jgit/internal/storage/file/GcPackRefsTest.java | 28 ++++++++++------------ .../jgit/internal/storage/file/RefDirectory.java | 20 ++++++++++++++-- 2 files changed, 31 insertions(+), 17 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java index 11a2a22edb..c43bdbd298 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/GcPackRefsTest.java @@ -43,12 +43,13 @@ package org.eclipse.jgit.internal.storage.file; -import static java.lang.Integer.valueOf; +import static org.hamcrest.Matchers.lessThanOrEqualTo; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertNull; import static org.junit.Assert.assertSame; +import static org.junit.Assert.assertThat; import java.io.File; import java.io.IOException; @@ -74,6 +75,7 @@ import org.eclipse.jgit.revwalk.RevCommit; import org.eclipse.jgit.storage.file.FileBasedConfig; import org.junit.Test; +@SuppressWarnings("boxing") public class GcPackRefsTest extends GcTestCase { @Test public void looseRefPacked() throws Exception { @@ -100,27 +102,23 @@ public class GcPackRefsTest extends GcTestCase { RevBlob a = tr.blob("a"); tr.lightweightTag("t", a); - final CyclicBarrier syncPoint = new CyclicBarrier(2); + CyclicBarrier syncPoint = new CyclicBarrier(2); - Callable packRefs = new Callable() { - - /** @return 0 for success, 1 in case of error when writing pack */ - @Override - public Integer call() throws Exception { - syncPoint.await(); - try { - gc.packRefs(); - return valueOf(0); - } catch (IOException e) { - return valueOf(1); - } + // Returns 0 for success, 1 in case of error when writing pack. + Callable packRefs = () -> { + syncPoint.await(); + try { + gc.packRefs(); + return 0; + } catch (IOException e) { + return 1; } }; ExecutorService pool = Executors.newFixedThreadPool(2); try { Future p1 = pool.submit(packRefs); Future p2 = pool.submit(packRefs); - assertEquals(1, p1.get().intValue() + p2.get().intValue()); + assertThat(p1.get() + p2.get(), lessThanOrEqualTo(1)); } finally { pool.shutdown(); pool.awaitTermination(Long.MAX_VALUE, TimeUnit.SECONDS); 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 5d66a4fbd3..e03bd8723f 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 @@ -914,8 +914,24 @@ public class RefDirectory extends RefDatabase { throw new ObjectWritingException(MessageFormat.format(JGitText.get().unableToWrite, name)); byte[] digest = Constants.newMessageDigest().digest(content); - packedRefs.compareAndSet(oldPackedList, new PackedRefList(refs, - lck.getCommitSnapshot(), ObjectId.fromRaw(digest))); + PackedRefList newPackedList = new PackedRefList( + refs, lck.getCommitSnapshot(), ObjectId.fromRaw(digest)); + + // This thread holds the file lock, so no other thread or process should + // be able to modify the packed-refs file on disk. If the list changed, + // it means something is very wrong, so throw an exception. + // + // However, we can't use a naive compareAndSet to check whether the + // update was successful, because another thread might _read_ the + // packed refs file that was written out by this thread while holding + // the lock, and update the packedRefs reference to point to that. So + // compare the actual contents instead. + PackedRefList afterUpdate = packedRefs.updateAndGet( + p -> p.id.equals(oldPackedList.id) ? newPackedList : p); + if (!afterUpdate.id.equals(newPackedList.id)) { + throw new ObjectWritingException( + MessageFormat.format(JGitText.get().unableToWrite, name)); + } } }.writePackedRefs(); } -- cgit v1.2.3 From f529fa6729a9b8b5c1329bf75639d26b0067f9d9 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 7 Jul 2017 11:24:51 -0400 Subject: 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 --- .../jgit/internal/storage/file/RefUpdateTest.java | 69 ++++++++++++++++++++-- .../jgit/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 daef91d53c..e4e5d28f7d 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 e03bd8723f..c8c2dd5867 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 fc334f0275..61fda943c0 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. * -- cgit v1.2.3 From 28202a67586931f1adf5b4a612e85f559b9dbe43 Mon Sep 17 00:00:00 2001 From: Dave Borowitz Date: Fri, 7 Jul 2017 10:31:01 -0400 Subject: Add tests for updating single refs to missing objects The reader may find it surprising that this succeeds without incident unless there is peeling or a fast-forward check involved. This behavior may be changed in the future, but for now, just document the current behavior. Change-Id: I348b37e93e0264dc0905c4d58ce881852d1dfe5e --- .../jgit/internal/storage/file/RefUpdateTest.java | 88 ++++++++++++++++++++++ 1 file changed, 88 insertions(+) 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 e4e5d28f7d..1203e83dce 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 @@ -58,10 +58,12 @@ import static org.junit.Assert.fail; import java.io.File; import java.io.IOException; +import java.util.Arrays; import java.util.List; import java.util.Map; import java.util.Map.Entry; +import org.eclipse.jgit.errors.MissingObjectException; import org.eclipse.jgit.lib.AnyObjectId; import org.eclipse.jgit.lib.Constants; import org.eclipse.jgit.lib.ObjectId; @@ -959,6 +961,92 @@ public class RefUpdateTest extends SampleDataRepositoryTestCase { "HEAD").getReverseEntries().get(0).getComment()); } + @Test + public void testCreateMissingObject() throws IOException { + String name = "refs/heads/abc"; + ObjectId bad = + ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + RefUpdate ru = db.updateRef(name); + ru.setNewObjectId(bad); + Result update = ru.update(); + assertEquals(Result.NEW, update); + + Ref ref = db.exactRef(name); + assertNotNull(ref); + assertFalse(ref.isPeeled()); + assertEquals(bad, ref.getObjectId()); + + try (RevWalk rw = new RevWalk(db)) { + rw.parseAny(ref.getObjectId()); + fail("Expected MissingObjectException"); + } catch (MissingObjectException expected) { + assertEquals(bad, expected.getObjectId()); + } + + RefDirectory refdir = (RefDirectory) db.getRefDatabase(); + try { + // Packing requires peeling, which fails. + refdir.pack(Arrays.asList(name)); + } catch (MissingObjectException expected) { + assertEquals(bad, expected.getObjectId()); + } + } + + @Test + public void testUpdateMissingObject() throws IOException { + String name = "refs/heads/abc"; + RefUpdate ru = updateRef(name); + Result update = ru.update(); + assertEquals(Result.NEW, update); + ObjectId oldId = ru.getNewObjectId(); + + ObjectId bad = + ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + ru = db.updateRef(name); + ru.setNewObjectId(bad); + update = ru.update(); + assertEquals(Result.REJECTED, update); + + Ref ref = db.exactRef(name); + assertNotNull(ref); + assertEquals(oldId, ref.getObjectId()); + } + + @Test + public void testForceUpdateMissingObject() throws IOException { + String name = "refs/heads/abc"; + RefUpdate ru = updateRef(name); + Result update = ru.update(); + assertEquals(Result.NEW, update); + + ObjectId bad = + ObjectId.fromString("deadbeefdeadbeefdeadbeefdeadbeefdeadbeef"); + ru = db.updateRef(name); + ru.setNewObjectId(bad); + update = ru.forceUpdate(); + assertEquals(Result.FORCED, update); + + Ref ref = db.exactRef(name); + assertNotNull(ref); + assertFalse(ref.isPeeled()); + assertEquals(bad, ref.getObjectId()); + + try (RevWalk rw = new RevWalk(db)) { + rw.parseAny(ref.getObjectId()); + fail("Expected MissingObjectException"); + } catch (MissingObjectException expected) { + assertEquals(bad, expected.getObjectId()); + } + + RefDirectory refdir = (RefDirectory) db.getRefDatabase(); + try { + // Packing requires peeling, which fails. + refdir.pack(Arrays.asList(name)); + } catch (MissingObjectException expected) { + assertEquals(bad, expected.getObjectId()); + } + } + private static void writeReflog(Repository db, ObjectId newId, String msg, String refName) throws IOException { RefDirectory refs = (RefDirectory) db.getRefDatabase(); -- cgit v1.2.3