DepthGenerator: fix multi-child boundary handling

Suppose that a repository has the following commit graph:

 B   C
  \ /
   A

and it was cloned with --shallow-exclude=A. DepthGenerator does not mark
C as shallow, causing an invalid repository to be produced on the
client, because A is not sent. (A similar issue occurs when
--shallow-since is used to exclude A but neither B nor C.)

This happens whenever an excluded commit has more than one child that is
to be sent to the client. Fix DepthGenerator to handle this case
correctly.

While we're editing DepthWalk.Commit, fix the documentation of
DepthWalk.Commit#isBoundary.

Change-Id: I7068abf0fe0c864d1b0e56e1616dad1aa8719411
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
index 4b3fe28cfc6ae2544dae2f6c70d711bf6ab668dd..8acbcce36dd4ab2c9f4c635d64282b84fe3d52fd 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java
@@ -1279,6 +1279,48 @@ public class UploadPackTest {
                assertTrue(client.hasObject(merge.toObjectId()));
        }
 
+       @Test
+       public void testV2FetchShallowSince_excludedParentWithMultipleChildren() throws Exception {
+               PersonIdent person = new PersonIdent(remote.getRepository());
+
+               RevCommit base = remote.commit()
+                       .committer(new PersonIdent(person, 1500000000, 0)).create();
+               RevCommit child1 = remote.commit().parent(base)
+                       .committer(new PersonIdent(person, 1510000000, 0)).create();
+               RevCommit child2 = remote.commit().parent(base)
+                       .committer(new PersonIdent(person, 1520000000, 0)).create();
+
+               remote.update("branch1", child1);
+               remote.update("branch2", child2);
+
+               ByteArrayInputStream recvStream = uploadPackV2(
+                       "command=fetch\n",
+                       PacketLineIn.DELIM,
+                       "deepen-since 1510000\n",
+                       "want " + child1.toObjectId().getName() + "\n",
+                       "want " + child2.toObjectId().getName() + "\n",
+                       "done\n",
+                       PacketLineIn.END);
+               PacketLineIn pckIn = new PacketLineIn(recvStream);
+               assertThat(pckIn.readString(), is("shallow-info"));
+
+               // "base" is excluded, so its children are shallow.
+               assertThat(
+                       Arrays.asList(pckIn.readString(), pckIn.readString()),
+                       hasItems(
+                               "shallow " + child1.toObjectId().getName(),
+                               "shallow " + child2.toObjectId().getName()));
+
+               assertThat(pckIn.readString(), theInstance(PacketLineIn.DELIM));
+               assertThat(pckIn.readString(), is("packfile"));
+               parsePack(recvStream);
+
+               // Only the children are sent.
+               assertFalse(client.hasObject(base.toObjectId()));
+               assertTrue(client.hasObject(child1.toObjectId()));
+               assertTrue(client.hasObject(child2.toObjectId()));
+       }
+
        @Test
        public void testV2FetchShallowSince_noCommitsSelected() throws Exception {
                PersonIdent person = new PersonIdent(remote.getRepository());
@@ -1405,6 +1447,49 @@ public class UploadPackTest {
                assertTrue(client.hasObject(four.toObjectId()));
        }
 
+       @Test
+       public void testV2FetchDeepenNot_excludedParentWithMultipleChildren() throws Exception {
+               PersonIdent person = new PersonIdent(remote.getRepository());
+
+               RevCommit base = remote.commit()
+                       .committer(new PersonIdent(person, 1500000000, 0)).create();
+               RevCommit child1 = remote.commit().parent(base)
+                       .committer(new PersonIdent(person, 1510000000, 0)).create();
+               RevCommit child2 = remote.commit().parent(base)
+                       .committer(new PersonIdent(person, 1520000000, 0)).create();
+
+               remote.update("base", base);
+               remote.update("branch1", child1);
+               remote.update("branch2", child2);
+
+               ByteArrayInputStream recvStream = uploadPackV2(
+                       "command=fetch\n",
+                       PacketLineIn.DELIM,
+                       "deepen-not base\n",
+                       "want " + child1.toObjectId().getName() + "\n",
+                       "want " + child2.toObjectId().getName() + "\n",
+                       "done\n",
+                       PacketLineIn.END);
+               PacketLineIn pckIn = new PacketLineIn(recvStream);
+               assertThat(pckIn.readString(), is("shallow-info"));
+
+               // "base" is excluded, so its children are shallow.
+               assertThat(
+                       Arrays.asList(pckIn.readString(), pckIn.readString()),
+                       hasItems(
+                               "shallow " + child1.toObjectId().getName(),
+                               "shallow " + child2.toObjectId().getName()));
+
+               assertThat(pckIn.readString(), theInstance(PacketLineIn.DELIM));
+               assertThat(pckIn.readString(), is("packfile"));
+               parsePack(recvStream);
+
+               // Only the children are sent.
+               assertFalse(client.hasObject(base.toObjectId()));
+               assertTrue(client.hasObject(child1.toObjectId()));
+               assertTrue(client.hasObject(child2.toObjectId()));
+       }
+
        @Test
        public void testV2FetchUnrecognizedArgument() throws Exception {
                thrown.expect(PackProtocolException.class);

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthGenerator.java b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthGenerator.java
index c422234088fee488edf15d80006b6bdd48f90264..515492039303f33f505bbc976513ec77084ba9dc 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthGenerator.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthGenerator.java
@@ -208,10 +208,14 @@ class DepthGenerator extends Generator {
                                                        !p.has(DEEPEN_NOT)) {
                                                pending.add(p);
                                        } else {
-                                               c.isBoundary = true;
+                                               dp.makesChildBoundary = true;
                                        }
                                }
 
+                               if (dp.makesChildBoundary) {
+                                       c.isBoundary = true;
+                               }
+
                                // If the current commit has become unshallowed, everything
                                // below us is new to the client.  Mark its parent as
                                // re-interesting, and carry that flag downward to all

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 66e8497a12d484dace0bbb52d7e13f11aa77f2c8..2eca12be524468e3916c551334f38293a3e49b97 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthWalk.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/revwalk/DepthWalk.java
@@ -112,14 +112,20 @@ public interface DepthWalk {
 
                boolean isBoundary;
 
+               /**
+                * True if this commit was excluded due to a shallow fetch
+                * setting. All its children are thus boundary commits.
+                */
+               boolean makesChildBoundary;
+
                /** @return depth of this commit, as found by the shortest path. */
                public int getDepth() {
                        return depth;
                }
 
                /**
-                * @return true if at least one of this commit's children was excluded
-                *         due to a depth or shallow-since restriction, false otherwise
+                * @return true if at least one of this commit's parents was excluded
+                *         due to a shallow fetch setting, false otherwise
                 * @since 5.2
                 */
                public boolean isBoundary() {