tree:<depth>: do not revisit tree during packing

If a tree is visited during pack and filtered out with tree:<depth>, we
may need to include it if it is visited again at a lower depth.

Until now we revisit it no matter what the depth is. Now, avoid
visiting it if it has been visited at a lower or equal depth.

Change-Id: I68cc1d08f1999a8336684a05fe16e7ae51898866
Signed-off-by: Matthew DeVore <matvore@gmail.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 f5470729db36da0eb20c3efa9d1e4e76bd293d76..9fd7483787ea99fa63e693051cdec2adeff000ed 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
@@ -1731,6 +1731,128 @@ public class UploadPackTest {
                                .has(preparator.foo.toObjectId()));
        }
 
+       /**
+        * Creates a commit with the following files:
+        * <pre>
+        * a/x/b/foo
+        * b/u/c/baz
+        * y/x/b/foo
+        * z/v/c/baz
+        * </pre>
+        * which has two pairs of identical trees:
+        * <ul>
+        * <li>one at /a and /y
+        * <li>one at /b/u and /z/v
+        * </ul>
+        * Note that this class defines unique 8 trees (rootTree and subtree1-7)
+        * which means PackStatistics should report having visited 8 trees.
+        */
+       class RepeatedSubtreeAtSameLevelPreparator {
+               RevBlob foo = remote.blob("foo");
+
+               /** foo */
+               RevTree subtree1 = remote.tree(remote.file("foo", foo));
+
+               /** b/foo */
+               RevTree subtree2 = (new TreeBuilder() {
+                       @Override
+                       void addElements(DirCacheBuilder dcBuilder) throws Exception {
+                               dcBuilder.addTree(new byte[] {'b'}, DirCacheEntry.STAGE_0,
+                                               remote.getRevWalk().getObjectReader(), subtree1);
+                       }
+               }).build();
+
+               /** x/b/foo */
+               RevTree subtree3 = (new TreeBuilder() {
+                       @Override
+                       void addElements(DirCacheBuilder dcBuilder) throws Exception {
+                               dcBuilder.addTree(new byte[] {'x'}, DirCacheEntry.STAGE_0,
+                                               remote.getRevWalk().getObjectReader(), subtree2);
+                       }
+               }).build();
+
+               RevBlob baz = remote.blob("baz");
+
+               /** baz */
+               RevTree subtree4 = remote.tree(remote.file("baz", baz));
+
+               /** c/baz */
+               RevTree subtree5 = (new TreeBuilder() {
+                       @Override
+                       void addElements(DirCacheBuilder dcBuilder) throws Exception {
+                               dcBuilder.addTree(new byte[] {'c'}, DirCacheEntry.STAGE_0,
+                                               remote.getRevWalk().getObjectReader(), subtree4);
+                       }
+               }).build();
+
+               /** u/c/baz */
+               RevTree subtree6 = (new TreeBuilder() {
+                       @Override
+                       void addElements(DirCacheBuilder dcBuilder) throws Exception {
+                               dcBuilder.addTree(new byte[] {'u'}, DirCacheEntry.STAGE_0,
+                                               remote.getRevWalk().getObjectReader(), subtree5);
+                       }
+               }).build();
+
+               /** v/c/baz */
+               RevTree subtree7 = (new TreeBuilder() {
+                       @Override
+                       void addElements(DirCacheBuilder dcBuilder) throws Exception {
+                               dcBuilder.addTree(new byte[] {'v'}, DirCacheEntry.STAGE_0,
+                                               remote.getRevWalk().getObjectReader(), subtree5);
+                       }
+               }).build();
+
+               RevTree rootTree = (new TreeBuilder() {
+                       @Override
+                       void addElements(DirCacheBuilder dcBuilder) throws Exception {
+                               dcBuilder.addTree(new byte[] {'a'}, DirCacheEntry.STAGE_0,
+                                               remote.getRevWalk().getObjectReader(), subtree3);
+                               dcBuilder.addTree(new byte[] {'b'}, DirCacheEntry.STAGE_0,
+                                               remote.getRevWalk().getObjectReader(), subtree6);
+                               dcBuilder.addTree(new byte[] {'y'}, DirCacheEntry.STAGE_0,
+                                               remote.getRevWalk().getObjectReader(), subtree3);
+                               dcBuilder.addTree(new byte[] {'z'}, DirCacheEntry.STAGE_0,
+                                               remote.getRevWalk().getObjectReader(), subtree7);
+                       }
+               }).build();
+               RevCommit commit = remote.commit(rootTree);
+
+               RepeatedSubtreeAtSameLevelPreparator() throws Exception {}
+       }
+
+       @Test
+       public void testV2FetchFilterTreeDepth_repeatTreeAtSameLevelIncludeFile()
+                       throws Exception {
+               RepeatedSubtreeAtSameLevelPreparator preparator =
+                               new RepeatedSubtreeAtSameLevelPreparator();
+               remote.update("master", preparator.commit);
+
+               uploadV2WithTreeDepthFilter(5, preparator.commit.toObjectId());
+
+               assertTrue(client.getObjectDatabase()
+                               .has(preparator.foo.toObjectId()));
+               assertTrue(client.getObjectDatabase()
+                               .has(preparator.baz.toObjectId()));
+               assertEquals(8, stats.getTreesTraversed());
+       }
+
+       @Test
+       public void testV2FetchFilterTreeDepth_repeatTreeAtSameLevelExcludeFile()
+                       throws Exception {
+               RepeatedSubtreeAtSameLevelPreparator preparator =
+                               new RepeatedSubtreeAtSameLevelPreparator();
+               remote.update("master", preparator.commit);
+
+               uploadV2WithTreeDepthFilter(4, preparator.commit.toObjectId());
+
+               assertFalse(client.getObjectDatabase()
+                               .has(preparator.foo.toObjectId()));
+               assertFalse(client.getObjectDatabase()
+                               .has(preparator.baz.toObjectId()));
+               assertEquals(8, stats.getTreesTraversed());
+       }
+
        @Test
        public void testWantFilteredObject() throws Exception {
                RepeatedSubtreePreparator preparator = new RepeatedSubtreePreparator();

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 c40094aef3c907fb44a2cb61e6d2c96dcd3ea556..95d8f17a7b79a076a7059ca00aba9bf409de01ab 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
@@ -63,6 +63,7 @@ import java.util.Arrays;
 import java.util.Collection;
 import java.util.Collections;
 import java.util.Comparator;
+import java.util.HashMap;
 import java.util.HashSet;
 import java.util.Iterator;
 import java.util.List;
@@ -873,19 +874,35 @@ public class PackWriter implements AutoCloseable {
        }
 
        /**
-        * A visitation policy which causes objects to be visited repeatedly by
-        * making {@code shouldVisit} always return {@code true}.
+        * A visitation policy which uses the depth at which the object is seen to
+        * decide if re-traversal is necessary. In particular, if the object has
+        * already been visited at this depth or shallower, it is not necessary to
+        * re-visit at this depth.
         */
-       private static final ObjectWalk.VisitationPolicy ALWAYS_VISIT_POLICY =
-                       new ObjectWalk.VisitationPolicy() {
+       private class DepthAwareVisitationPolicy
+                       implements ObjectWalk.VisitationPolicy {
+               private final Map<ObjectId, Long> lowestDepthVisited = new HashMap<>();
+
+               private final ObjectWalk walk;
+
+               DepthAwareVisitationPolicy(ObjectWalk walk) {
+                       this.walk = requireNonNull(walk);
+               }
+
                @Override
                public boolean shouldVisit(RevObject o) {
-                       return true;
+                       Long lastDepth = lowestDepthVisited.get(o);
+                       if (lastDepth == null) {
+                               return true;
+                       }
+                       return walk.getTreeDepth() < lastDepth;
                }
 
                @Override
-               public void visited(RevObject o) {}
-       };
+               public void visited(RevObject o) {
+                       lowestDepthVisited.put(o, (long) walk.getTreeDepth());
+               }
+       }
 
        /**
         * Prepare the list of objects to be written to the pack stream.
@@ -929,7 +946,7 @@ public class PackWriter implements AutoCloseable {
                        throw new IllegalArgumentException(
                                        JGitText.get().shallowPacksRequireDepthWalk);
                if (filterSpec.getTreeDepthLimit() >= 0) {
-                       walk.setVisitationPolicy(ALWAYS_VISIT_POLICY);
+                       walk.setVisitationPolicy(new DepthAwareVisitationPolicy(walk));
                }
                findObjectsToPack(countingMonitor, walk, interestingObjects,
                                uninterestingObjects, noBitmaps);