Shallow fetch: avoid sending unneeded blobs

When doing an incremental fetch from JGit, "have" commits are marked
as "uninteresting". In a non-shallow fetch, when the RevWalk hits an
"uninteresting" commit it marks the commit's corresponding tree as
uninteresting. That has the effect of dropping those trees and all the
trees and blobs they reference out of the thin pack returned to the
client.

However, shallow fetches use a DepthWalk to limit the RevWalk, which
nearly always causes the RevWalk to terminate before encountering the
"have" commits. As a result the pack created for the incremental fetch
never encounters "uninteresting" tree objects and thus includes
duplicate objects that it knows the client already has.

Change-Id: I7b1f7c3b0d83e04d34cd2fa676f1ad4fec904c05
Signed-off-by: Terry Parker <tparker@google.com>

1 files changed, 69 insertions, 3 deletions

diff --git a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java
index 5afb3eac0d..de72793585 100644
--- a/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java
+++ b/org.eclipse.jgit.test/tst/org/eclipse/jgit/internal/storage/file/PackWriterTest.java
@@ -76,6 +76,9 @@ import org.eclipse.jgit.lib.ObjectId;
 import org.eclipse.jgit.lib.ObjectIdSet;
 import org.eclipse.jgit.lib.ObjectInserter;
 import org.eclipse.jgit.lib.Sets;
+import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.DepthWalk;
+import org.eclipse.jgit.revwalk.ObjectWalk;
 import org.eclipse.jgit.revwalk.RevBlob;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevObject;
@@ -542,15 +545,74 @@ public class PackWriterTest extends SampleDataRepositoryTestCase {
 					expected.contains(pi.getObjectId(i)));
 	}
 
+	@Test
+	public void testShallowIsMinimal() throws Exception {
+		FileRepository repo = createBareRepository();
+		TestRepository<Repository> r = new TestRepository<Repository>(repo);
+		BranchBuilder bb = r.branch("refs/heads/master");
+		RevBlob contentA = r.blob("A");
+		RevBlob contentB = r.blob("B");
+		RevBlob contentC = r.blob("C");
+		RevBlob contentD = r.blob("D");
+		RevBlob contentE = r.blob("E");
+		RevCommit c1 = bb.commit().add("a", contentA).create();
+		RevCommit c2 = bb.commit().add("b", contentB).create();
+		RevCommit c3 = bb.commit().add("c", contentC).create();
+		RevCommit c4 = bb.commit().add("d", contentD).create();
+		RevCommit c5 = bb.commit().add("e", contentE).create();
+		r.getRevWalk().parseHeaders(c1);
+		r.getRevWalk().parseHeaders(c2);
+		r.getRevWalk().parseHeaders(c3);
+		r.getRevWalk().parseHeaders(c4);
+		r.getRevWalk().parseHeaders(c5);
+
+		PackIndex idx = writeShallowPack(repo, 1, wants(c2), NONE, NONE);
+		assertContent(idx,
+				Arrays.asList(c1.getId(), c2.getId(), c1.getTree().getId(),
+						c2.getTree().getId(), contentA.getId(),
+						contentB.getId()));
+
+		// Client already has blobs A and B, verify those are not packed.
+		idx = writeShallowPack(repo, 1, wants(c5), haves(c1, c2), shallows(c1));
+		assertContent(idx,
+				Arrays.asList(c4.getId(), c5.getId(), c4.getTree().getId(),
+						c5.getTree().getId(), contentC.getId(),
+						contentD.getId(), contentE.getId()));
+	}
+
 	private static PackIndex writePack(FileRepository repo,
 			Set<? extends ObjectId> want, Set<ObjectIdSet> excludeObjects)
-			throws IOException {
+					throws IOException {
+		RevWalk walk = new RevWalk(repo);
+		return writePack(repo, walk, 0, want, NONE, excludeObjects);
+	}
+
+	private static PackIndex writeShallowPack(FileRepository repo, int depth,
+			Set<? extends ObjectId> want, Set<? extends ObjectId> have,
+			Set<? extends ObjectId> shallow) throws IOException {
+		// During negotiation, UploadPack would have set up a DepthWalk and
+		// marked the client's "shallow" commits. Emulate that here.
+		DepthWalk.RevWalk walk = new DepthWalk.RevWalk(repo, depth);
+		walk.assumeShallow(shallow);
+		return writePack(repo, walk, depth, want, have, EMPTY_ID_SET);
+	}
+
+	private static PackIndex writePack(FileRepository repo, RevWalk walk,
+			int depth, Set<? extends ObjectId> want,
+			Set<? extends ObjectId> have, Set<ObjectIdSet> excludeObjects)
+					throws IOException {
 		try (PackWriter pw = new PackWriter(repo)) {
 			pw.setDeltaBaseAsOffset(true);
 			pw.setReuseDeltaCommits(false);
-			for (ObjectIdSet idx : excludeObjects)
+			for (ObjectIdSet idx : excludeObjects) {
 				pw.excludeObjects(idx);
-			pw.preparePack(NullProgressMonitor.INSTANCE, want, NONE);
+			}
+			if (depth > 0) {
+				pw.setShallowPack(depth, null);
+			}
+			ObjectWalk ow = walk.toObjectWalkWithSameObjects();
+
+			pw.preparePack(NullProgressMonitor.INSTANCE, ow, want, have);
 			String id = pw.computeName().getName();
 			File packdir = new File(repo.getObjectsDirectory(), "pack");
 			File packFile = new File(packdir, "pack-" + id + ".pack");
@@ -737,4 +799,8 @@ public class PackWriterTest extends SampleDataRepositoryTestCase {
 	private static Set<ObjectId> wants(ObjectId... objects) {
 		return Sets.of(objects);
 	}
+
+	private static Set<ObjectId> shallows(ObjectId... objects) {
+		return Sets.of(objects);
+	}
 }