From 0a15cb3a2bc14feec11baa1977567179ce3094bc Mon Sep 17 00:00:00 2001 From: Matthew DeVore Date: Wed, 27 Mar 2019 14:35:51 -0700 Subject: [PATCH] tree:: do not revisit tree during packing If a tree is visited during pack and filtered out with tree:, 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 --- .../jgit/transport/UploadPackTest.java | 122 ++++++++++++++++++ .../internal/storage/pack/PackWriter.java | 37 ++++-- 2 files changed, 149 insertions(+), 10 deletions(-) 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 f5470729d..9fd748378 100644 --- 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 void testV2FetchFilterTreeDepth_iterateOverTreeAtTwoLevels() .has(preparator.foo.toObjectId())); } + /** + * Creates a commit with the following files: + * 
+	 * a/x/b/foo
+	 * b/u/c/baz
+	 * y/x/b/foo
+	 * z/v/c/baz
+	 *
+ * which has two pairs of identical trees: + *
    + *
  • one at /a and /y + *
  • one at /b/u and /z/v + *
+ * 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 c40094aef..95d8f17a7 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 @@ -63,6 +63,7 @@ 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 @@ private ObjectWalk getObjectWalk() { } /** - * 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() { - @Override - public boolean shouldVisit(RevObject o) { - return true; + private class DepthAwareVisitationPolicy + implements ObjectWalk.VisitationPolicy { + private final Map lowestDepthVisited = new HashMap<>(); + + private final ObjectWalk walk; + + DepthAwareVisitationPolicy(ObjectWalk walk) { + this.walk = requireNonNull(walk); } @Override - public void visited(RevObject o) {} - }; + public boolean shouldVisit(RevObject o) { + Long lastDepth = lowestDepthVisited.get(o); + if (lastDepth == null) { + return true; + } + return walk.getTreeDepth() < lastDepth; + } + + @Override + 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 void preparePack(ProgressMonitor countingMonitor, 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);