From d385a7a5e5cacd875da4301ddeb46a7c56a725fa Mon Sep 17 00:00:00 2001 From: Terry Parker Date: Wed, 3 Aug 2016 09:01:22 -0700 Subject: [PATCH] Shallow fetch: Respect "shallow" lines When fetching from a shallow clone, the client sends "have" lines to tell the server about objects it already has and "shallow" lines to tell where its local history terminates. In some circumstances, the server fails to honor the shallow lines and fails to return objects that the client needs. UploadPack passes the "have" lines to PackWriter so PackWriter can omit them from the generated pack. UploadPack processes "shallow" lines by calling RevWalk.assumeShallow() with the set of shallow commits. RevWalk creates and caches RevCommits for these shallow commits, clearing out their parents. That way, walks correctly terminate at the shallow commits instead of assuming the client has history going back behind them. UploadPack converts its RevWalk to an ObjectWalk, maintaining the cached RevCommits, and passes it to PackWriter. Unfortunately, to support shallow fetches the PackWriter does the following: if (shallowPack && !(walk instanceof DepthWalk.ObjectWalk)) walk = new DepthWalk.ObjectWalk(reader, depth); That is, when the client sends a "deepen" line (fetch --depth=) and the caller has not passed in a DepthWalk.ObjectWalk, PackWriter throws away the RevWalk that was passed in and makes a new one. The cleared parent lists prepared by RevWalk.assumeShallow() are lost. Fortunately UploadPack intends to pass in a DepthWalk.ObjectWalk. It tries to create it by calling toObjectWalkWithSameObjects() on a DepthWalk.RevWalk. But it doesn't work: because DepthWalk.RevWalk does not override the standard RevWalk#toObjectWalkWithSameObjects implementation, the result is a plain ObjectWalk instead of an instance of DepthWalk.ObjectWalk. The result is that the "shallow" information is thrown away and objects reachable from the shallow commits can be omitted from the pack sent when fetching with --depth from a shallow clone. Multiple factors collude to limit the circumstances under which this bug can be observed: 1. Commits with depth != 0 don't enter DepthGenerator's pending queue. That means a "have" cannot have any effect on DepthGenerator unless it is also a "want". 2. DepthGenerator#next() doesn't call carryFlagsImpl(), so the uninteresting flag is not propagated to ancestors there even if a "have" is also a "want". 3. JGit treats a depth of 1 as "1 past the wants". Because of (2), the only place the UNINTERESTING flag can leak to a shallow commit's parents is in the carryFlags() call from markUninteresting(). carryFlags() only traverses commits that have already been parsed: commits yet to be parsed are supposed to inherit correct flags from their parent in PendingGenerator#next (which doesn't happen here --- that is (2)). So the list of commits that have already been parsed becomes relevant. When we hit the markUninteresting() call, all "want"s, "have"s, and commits to be unshallowed have been parsed. carryFlags() only affects the parsed commits. If the "want" is a direct parent of a "have", then it carryFlags() marks it as uninteresting. If the "have" was also a "shallow", then its parent pointer should have been null and the "want" shouldn't have been marked, so we see the bug. If the "want" is a more distant ancestor then (2) keeps the uninteresting state from propagating to the "want" and we don't see the bug. If the "shallow" is not also a "have" then the shallow commit isn't parsed so (2) keeps the uninteresting state from propagating to the "want so we don't see the bug. Here is a reproduction case (time flowing left to right, arrows pointing to parents). "C" must be a commit that the client reports as a "have" during negotiation. That can only happen if the server reports it as an existing branch or tag in the first round of negotiation: A <-- B <-- C <-- D First do git clone --depth 1 which yields D as a "have" and C as a "shallow" commit. Then try git fetch --depth 1 B:refs/heads/B Negotiation sets up: have D, shallow C, have C, want B. But due to this bug B is marked as uninteresting and is not sent. Change-Id: I6e14b57b2f85e52d28cdcf356df647870f475440 Signed-off-by: Terry Parker --- .../internal/storage/file/PackWriterTest.java | 97 ++++++++++++++----- .../eclipse/jgit/internal/JGitText.properties | 1 + .../org/eclipse/jgit/internal/JGitText.java | 1 + .../internal/storage/pack/PackWriter.java | 5 +- .../org/eclipse/jgit/revwalk/DepthWalk.java | 8 ++ 5 files changed, 88 insertions(+), 24 deletions(-) diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java index de7279358..50d831031 100644 --- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java +++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java @@ -111,6 +111,26 @@ public class PackWriterTest extends SampleDataRepositoryTestCase { private FileRepository dst; + private RevBlob contentA; + + private RevBlob contentB; + + private RevBlob contentC; + + private RevBlob contentD; + + private RevBlob contentE; + + private RevCommit c1; + + private RevCommit c2; + + private RevCommit c3; + + private RevCommit c4; + + private RevCommit c5; + @Before public void setUp() throws Exception { super.setUp(); @@ -517,16 +537,16 @@ public class PackWriterTest extends SampleDataRepositoryTestCase { TestRepository testRepo = new TestRepository( repo); BranchBuilder bb = testRepo.branch("refs/heads/master"); - RevBlob contentA = testRepo.blob("A"); - RevCommit c1 = bb.commit().add("f", contentA).create(); + contentA = testRepo.blob("A"); + c1 = bb.commit().add("f", contentA).create(); testRepo.getRevWalk().parseHeaders(c1); PackIndex pf1 = writePack(repo, wants(c1), EMPTY_ID_SET); assertContent( pf1, Arrays.asList(c1.getId(), c1.getTree().getId(), contentA.getId())); - RevBlob contentB = testRepo.blob("B"); - RevCommit c2 = bb.commit().add("f", contentB).create(); + contentB = testRepo.blob("B"); + c2 = bb.commit().add("f", contentB).create(); testRepo.getRevWalk().parseHeaders(c2); PackIndex pf2 = writePack(repo, wants(c2), Sets.of((ObjectIdSet) pf1)); assertContent( @@ -547,24 +567,7 @@ public class PackWriterTest extends SampleDataRepositoryTestCase { @Test public void testShallowIsMinimal() throws Exception { - FileRepository repo = createBareRepository(); - TestRepository r = new TestRepository(repo); - BranchBuilder bb = r.branch("refs/heads/master"); - RevBlob contentA = r.blob("A"); - RevBlob contentB = r.blob("B"); - RevBlob contentC = r.blob("C"); - RevBlob contentD = r.blob("D"); - RevBlob contentE = r.blob("E"); - RevCommit c1 = bb.commit().add("a", contentA).create(); - RevCommit c2 = bb.commit().add("b", contentB).create(); - RevCommit c3 = bb.commit().add("c", contentC).create(); - RevCommit c4 = bb.commit().add("d", contentD).create(); - RevCommit c5 = bb.commit().add("e", contentE).create(); - r.getRevWalk().parseHeaders(c1); - r.getRevWalk().parseHeaders(c2); - r.getRevWalk().parseHeaders(c3); - r.getRevWalk().parseHeaders(c4); - r.getRevWalk().parseHeaders(c5); + FileRepository repo = setupRepoForShallowFetch(); PackIndex idx = writeShallowPack(repo, 1, wants(c2), NONE, NONE); assertContent(idx, @@ -580,6 +583,56 @@ public class PackWriterTest extends SampleDataRepositoryTestCase { contentD.getId(), contentE.getId())); } + @Test + public void testShallowFetchShallowParent() throws Exception { + FileRepository repo = setupRepoForShallowFetch(); + + PackIndex idx = writeShallowPack(repo, 1, wants(c5), NONE, NONE); + assertContent(idx, + Arrays.asList(c4.getId(), c5.getId(), c4.getTree().getId(), + c5.getTree().getId(), contentA.getId(), + contentB.getId(), contentC.getId(), contentD.getId(), + contentE.getId())); + + idx = writeShallowPack(repo, 1, wants(c3), haves(c4, c5), shallows(c4)); + assertContent(idx, Arrays.asList(c2.getId(), c3.getId(), + c2.getTree().getId(), c3.getTree().getId())); + } + + @Test + public void testShallowFetchShallowAncestor() throws Exception { + FileRepository repo = setupRepoForShallowFetch(); + + PackIndex idx = writeShallowPack(repo, 1, wants(c5), NONE, NONE); + assertContent(idx, + Arrays.asList(c4.getId(), c5.getId(), c4.getTree().getId(), + c5.getTree().getId(), contentA.getId(), + contentB.getId(), contentC.getId(), contentD.getId(), + contentE.getId())); + + idx = writeShallowPack(repo, 1, wants(c2), haves(c4, c5), shallows(c4)); + assertContent(idx, Arrays.asList(c1.getId(), c2.getId(), + c1.getTree().getId(), c2.getTree().getId())); + } + + private FileRepository setupRepoForShallowFetch() throws Exception { + FileRepository repo = createBareRepository(); + TestRepository r = new TestRepository(repo); + BranchBuilder bb = r.branch("refs/heads/master"); + contentA = r.blob("A"); + contentB = r.blob("B"); + contentC = r.blob("C"); + contentD = r.blob("D"); + contentE = r.blob("E"); + c1 = bb.commit().add("a", contentA).create(); + c2 = bb.commit().add("b", contentB).create(); + c3 = bb.commit().add("c", contentC).create(); + c4 = bb.commit().add("d", contentD).create(); + c5 = bb.commit().add("e", contentE).create(); + r.getRevWalk().parseHeaders(c5); // fully initialize the tip RevCommit + return repo; + } + private static PackIndex writePack(FileRepository repo, Set want, Set excludeObjects) throws IOException { 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 5dcadd385..681b6ac9f 100644 --- a/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties +++ b/org.eclipse.jgit/resources/org/eclipse/jgit/internal/JGitText.properties @@ -557,6 +557,7 @@ sequenceTooLargeForDiffAlgorithm=Sequence too large for difference algorithm. serviceNotEnabledNoName=Service not enabled serviceNotPermitted={0} not permitted shallowCommitsAlreadyInitialized=Shallow commits have already been initialized +shallowPacksRequireDepthWalk=Shallow packs require a DepthWalk shortCompressedStreamAt=Short compressed stream at {0} shortReadOfBlock=Short read of block. shortReadOfOptionalDIRCExtensionExpectedAnotherBytes=Short read of optional DIRC extension {0}; expected another {1} bytes within the section. 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 fc54fd80a..127596fe3 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/JGitText.java @@ -616,6 +616,7 @@ public class JGitText extends TranslationBundle { /***/ public String serviceNotEnabledNoName; /***/ public String serviceNotPermitted; /***/ public String shallowCommitsAlreadyInitialized; + /***/ public String shallowPacksRequireDepthWalk; /***/ public String shortCompressedStreamAt; /***/ public String shortReadOfBlock; /***/ public String shortReadOfOptionalDIRCExtensionExpectedAnotherBytes; diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java index 4c0fc47f6..106308f4a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/pack/PackWriter.java @@ -752,7 +752,8 @@ public class PackWriter implements AutoCloseable { if (countingMonitor == null) countingMonitor = NullProgressMonitor.INSTANCE; if (shallowPack && !(walk instanceof DepthWalk.ObjectWalk)) - walk = new DepthWalk.ObjectWalk(reader, depth); + throw new IllegalArgumentException( + JGitText.get().shallowPacksRequireDepthWalk); findObjectsToPack(countingMonitor, walk, interestingObjects, uninterestingObjects); } @@ -1654,7 +1655,7 @@ public class PackWriter implements AutoCloseable { List wantTags = new ArrayList(want.size()); // Retrieve the RevWalk's versions of "want" and "have" objects to - // maintain any flags previously set in the RevWalk. + // maintain any state previously set in the RevWalk. AsyncRevObjectQueue q = walker.parseAny(all, true); try { for (;;) { diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthWalk.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthWalk.java index 89d05dba5..914b8a8d1 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthWalk.java +++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthWalk.java @@ -149,6 +149,14 @@ public interface DepthWalk { public RevFlag getReinterestingFlag() { return REINTERESTING; } + + @Override + public ObjectWalk toObjectWalkWithSameObjects() { + ObjectWalk ow = new ObjectWalk(reader, depth); + ow.objects = objects; + ow.freeFlags = freeFlags; + return ow; + } } /** Subclass of ObjectWalk that performs depth filtering. */