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 9f65ee2..7130d59 100644
--- 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 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 void testReuseBloomFilters() throws Exception {
"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 0d9815e..37a0de8 100644
--- 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 @@ private void writeCommitData(CancellableDigestOutputStream out)
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 @@ private BloomFilterChunks computeBloomFilterChunks(ProgressMonitor monitor)
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 @@ private void writeExtraEdges(CancellableDigestOutputStream out)
}
}
+ // 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;
