From 37a36201a1999690500e5af7f16fa881a484a0ca Mon Sep 17 00:00:00 2001 From: Ivan Frade Date: Fri, 31 May 2024 12:12:57 -0700 Subject: [PATCH] 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 --- .../commitgraph/CommitGraphWriterTest.java | 24 +++++++ .../commitgraph/CommitGraphWriter.java | 72 ++++++++++--------- 2 files changed, 63 insertions(+), 33 deletions(-) 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 9f65ee2074..7130d59b95 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 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> byteBuffers = c.changedPaths(walk.getObjectReader(), tip); + + assertTrue(byteBuffers.isPresent()); + List 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 0d9815eceb..37a0de84c4 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 @@ public class CommitGraphWriter { return generations; } - private static Optional> computeBloomFilterPaths( - ObjectReader or, RevCommit cmit) throws MissingObjectException, - IncorrectObjectTypeException, CorruptObjectException, IOException { - HashSet 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> paths = computeBloomFilterPaths( - graphCommits.getObjectReader(), cmit); + Optional> 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> changedPaths( + ObjectReader or, RevCommit cmit) throws MissingObjectException, + IncorrectObjectTypeException, CorruptObjectException, IOException { + HashSet 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; -- 2.39.5