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=<n>)
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 <repo>

which yields D as a "have" and C as a "shallow" commit. Then try

  git fetch --depth 1 <repo> 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 <tparker@google.com>

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 de727935856181c6d26f9fec418294e33ba906d4..50d83103145bfc9652a6fbafe33990136925b917 100644 (file)
--- 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<FileRepository> testRepo = new TestRepository<FileRepository>(
                                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<Repository> r = new TestRepository<Repository>(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<Repository> r = new TestRepository<Repository>(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<? extends ObjectId> want, Set<ObjectIdSet> 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 5dcadd38584965726ae716968bf54bfaa31f32d8..681b6ac9f0af269b7f2fa8c035691c69ede86872 100644 (file)
--- 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 fc54fd80a669489f777c702ba4f27e325bb73361..127596fe3d7eddc991a9359cafcf328103aa288a 100644 (file)
--- 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 4c0fc47f6781b657296efb42c792b87c3a1b3513..106308f4ad69f6b035377068f76a431760cb356c 100644 (file)
--- 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<RevTag> wantTags = new ArrayList<RevTag>(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 89d05dba59fd19ec7c2323b68c9a2c93eed3fcda..914b8a8d10517552f839b89536554ac91d3b6d35 100644 (file)
--- 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. */