CommitGraphWriter: Move path diff calculation to its own class

To verify that we have the right paths between commits we are writing
the bloom filters, reading them and querying. The path diff
calculation is tricky enough for correctness and performance that
should be tested on its own.

Move the path diff calculation to its own class, so we can test it on
its own.

This is a noop refactor so we can verify later the steps taken in the
walk.

Change-Id: Ifbdcb752891c4adb08553802f87287de1155bb7c

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/commitgraph/CommitGraphWriterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/commitgraph/CommitGraphWriterTest.java
index 9f65ee20749d7c9a34a71fd507d25efd98d63873..7130d59b95ed7d5ee74acf267bee8c4d74df3344 100644 (file)
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/commitgraph/CommitGraphWriterTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/commitgraph/CommitGraphWriterTest.java
@@ -10,6 +10,7 @@
 
 package org.eclipse.jgit.internal.storage.commitgraph;
 
+import static java.util.stream.Collectors.toList;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.junit.Assert.assertArrayEquals;
@@ -19,8 +20,12 @@ import static org.junit.Assert.assertTrue;
 
 import java.io.ByteArrayOutputStream;
 import java.io.IOException;
+import java.nio.ByteBuffer;
+import java.nio.charset.StandardCharsets;
 import java.util.Collections;
 import java.util.HashSet;
+import java.util.List;
+import java.util.Optional;
 import java.util.Set;
 
 import org.eclipse.jgit.dircache.DirCacheEntry;
@@ -413,6 +418,25 @@ public class CommitGraphWriterTest extends RepositoryTestCase {
                                "119,69,63,-8,0,"));
        }
 
+       @Test
+       public void testPathDiffCalculator_skipUnchangedTree() throws Exception {
+               RevCommit root = tr.commit(tr.tree(
+                               tr.file("d/sd1/f1", tr.blob("f1")),
+                               tr.file("d/sd2/f2", tr.blob("f2"))));
+               RevCommit tip = tr.commit(tr.tree(
+                               tr.file("d/sd1/f1", tr.blob("f1")),
+                               tr.file("d/sd2/f2", tr.blob("f2B"))), root);
+               CommitGraphWriter.PathDiffCalculator c = new CommitGraphWriter.PathDiffCalculator();
+
+               Optional<HashSet<ByteBuffer>> byteBuffers = c.changedPaths(walk.getObjectReader(), tip);
+
+               assertTrue(byteBuffers.isPresent());
+               List<String> asString = byteBuffers.get().stream()
+                               .map(b -> StandardCharsets.UTF_8.decode(b).toString())
+                               .collect(toList());
+               assertThat(asString, containsInAnyOrder("d", "d/sd2", "d/sd2/f2"));
+       }
+
        RevCommit commit(RevCommit... parents) throws Exception {
                return tr.commit(parents);
        }

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/commitgraph/CommitGraphWriter.java b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/commitgraph/CommitGraphWriter.java
index 0d9815ecebada4737a122480830722a61d8fc330..37a0de84c45a6ade8e6375df8f12472a16425295 100644 (file)
--- a/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/commitgraph/CommitGraphWriter.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/internal/storage/commitgraph/CommitGraphWriter.java
@@ -71,6 +71,9 @@ public class CommitGraphWriter {
 
        private static final int MAX_CHANGED_PATHS = 512;
 
+       private static final PathDiffCalculator PATH_DIFF_CALCULATOR
+                       = new PathDiffCalculator();
+
        private final int hashsz;
 
        private final GraphCommits graphCommits;
@@ -374,37 +377,6 @@ public class CommitGraphWriter {
                return generations;
        }
 
-       private static Optional<HashSet<ByteBuffer>> computeBloomFilterPaths(
-                       ObjectReader or, RevCommit cmit) throws MissingObjectException,
-                       IncorrectObjectTypeException, CorruptObjectException, IOException {
-               HashSet<ByteBuffer> paths = new HashSet<>();
-               try (TreeWalk walk = new TreeWalk(null, or)) {
-                       walk.setRecursive(true);
-                       if (cmit.getParentCount() == 0) {
-                               walk.addTree(new EmptyTreeIterator());
-                       } else {
-                               walk.addTree(cmit.getParent(0).getTree());
-                       }
-                       walk.addTree(cmit.getTree());
-                       while (walk.next()) {
-                               if (walk.idEqual(0, 1)) {
-                                       continue;
-                               }
-                               byte[] rawPath = walk.getRawPath();
-                               paths.add(ByteBuffer.wrap(rawPath));
-                               for (int i = 0; i < rawPath.length; i++) {
-                                       if (rawPath[i] == '/') {
-                                               paths.add(ByteBuffer.wrap(rawPath, 0, i));
-                                       }
-                                       if (paths.size() > MAX_CHANGED_PATHS) {
-                                               return Optional.empty();
-                                       }
-                               }
-                       }
-               }
-               return Optional.of(paths);
-       }
-
        private BloomFilterChunks computeBloomFilterChunks(ProgressMonitor monitor)
                        throws MissingObjectException, IncorrectObjectTypeException,
                        CorruptObjectException, IOException {
@@ -435,8 +407,8 @@ public class CommitGraphWriter {
                                        filtersReused++;
                                } else {
                                        filtersComputed++;
-                                       Optional<HashSet<ByteBuffer>> paths = computeBloomFilterPaths(
-                                                       graphCommits.getObjectReader(), cmit);
+                                       Optional<HashSet<ByteBuffer>> paths = PATH_DIFF_CALCULATOR
+                                                       .changedPaths(graphCommits.getObjectReader(), cmit);
                                        if (paths.isEmpty()) {
                                                cpf = ChangedPathFilter.FULL;
                                        } else {
@@ -473,6 +445,40 @@ public class CommitGraphWriter {
                }
        }
 
+       // Visible for testing
+       static class PathDiffCalculator {
+               Optional<HashSet<ByteBuffer>> changedPaths(
+                               ObjectReader or, RevCommit cmit) throws MissingObjectException,
+                               IncorrectObjectTypeException, CorruptObjectException, IOException {
+                       HashSet<ByteBuffer> paths = new HashSet<>();
+                       try (TreeWalk walk = new TreeWalk(null, or)) {
+                               walk.setRecursive(true);
+                               if (cmit.getParentCount() == 0) {
+                                       walk.addTree(new EmptyTreeIterator());
+                               } else {
+                                       walk.addTree(cmit.getParent(0).getTree());
+                               }
+                               walk.addTree(cmit.getTree());
+                               while (walk.next()) {
+                                       if (walk.idEqual(0, 1)) {
+                                               continue;
+                                       }
+                                       byte[] rawPath = walk.getRawPath();
+                                       paths.add(ByteBuffer.wrap(rawPath));
+                                       for (int i = 0; i < rawPath.length; i++) {
+                                               if (rawPath[i] == '/') {
+                                                       paths.add(ByteBuffer.wrap(rawPath, 0, i));
+                                               }
+                                               if (paths.size() > MAX_CHANGED_PATHS) {
+                                                       return Optional.empty();
+                                               }
+                                       }
+                               }
+                       }
+                       return Optional.of(paths);
+               }
+       }
+
        private static class ChunkHeader {
                final int id;