UploadPack: support lazy fetches if no bitmap

If a client clones with "--filter=blob:none", the checkout that "git
clone" automatically does causes the client to fetch all blobs at HEAD.
When fetching from a non-bitmapped repository, this will fail if an
object walk is ever needed, because JGit currently rejects such requests
- see the commit message of d3021788d2 ("Use bitmaps for non-commit
reachability checks", 2017-11-10) for more information.

Rejecting such requests in the absence of bitmaps is probably
overzealous: it is true that the server would prefer to have bitmaps in
this case, but there might be a small proportion of repos (for example,
very small repos or newly created ones) that do not have bitmaps, yet
the server would still like to have partial clones for them.

So, allow such requests, performing the object walk reachability check
if necessary. Limit this to servers with "uploadpack.allowFilter"
configured, so that servers wanting to support partial clone have this
functionality, and servers that do not support partial clone do not have
to pay the object walk reachability check cost.

Change-Id: I51964bafec68696a799625d627615b4f45ddbbbf
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>

2 files changed, 71 insertions, 0 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 9b090e8949..fb7afbc378 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
@@ -195,6 +195,35 @@ public class UploadPackTest {
 	}
 
 	@Test
+	public void testFetchReachableBlobWithoutBitmapButFilterAllowed() throws Exception {
+		InMemoryRepository server2 = newRepo("server2");
+		try (TestRepository<InMemoryRepository> remote2 = new TestRepository<>(
+				server2)) {
+			RevBlob blob = remote2.blob("foo");
+			RevCommit commit = remote2.commit(remote2.tree(remote2.file("foo", blob)));
+			remote2.update("master", commit);
+
+			server2.getConfig().setBoolean("uploadpack", null, "allowfilter",
+					true);
+
+			testProtocol = new TestProtocol<>((Object req, Repository db) -> {
+				UploadPack up = new UploadPack(db);
+				up.setRequestPolicy(RequestPolicy.REACHABLE_COMMIT);
+				return up;
+			}, null);
+			uri = testProtocol.register(ctx, server2);
+
+			assertFalse(client.getObjectDatabase().has(blob.toObjectId()));
+
+			try (Transport tn = testProtocol.open(uri, client, "server2")) {
+				tn.fetch(NullProgressMonitor.INSTANCE,
+						Collections.singletonList(new RefSpec(blob.name())));
+				assertTrue(client.getObjectDatabase().has(blob.toObjectId()));
+			}
+		}
+	}
+
+	@Test
 	public void testFetchWithBlobNoneFilter() throws Exception {
 		InMemoryRepository server2 = newRepo("server2");
 		try (TestRepository<InMemoryRepository> remote2 = new TestRepository<>(
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
index 17632c10a7..db0d267462 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -113,6 +113,7 @@ import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.revwalk.RevFlag;
 import org.eclipse.jgit.revwalk.RevFlagSet;
 import org.eclipse.jgit.revwalk.RevObject;
+import org.eclipse.jgit.revwalk.RevSort;
 import org.eclipse.jgit.revwalk.RevTag;
 import org.eclipse.jgit.revwalk.RevWalk;
 import org.eclipse.jgit.revwalk.filter.CommitTimeRevFilter;
@@ -1868,6 +1869,36 @@ public class UploadPack {
 		}
 	}
 
+	private static void checkReachabilityByWalkingObjects(ObjectWalk walk,
+			List<RevObject> wants, Set<ObjectId> reachableFrom) throws IOException {
+
+		walk.sort(RevSort.TOPO);
+		for (RevObject want : wants) {
+			walk.markStart(want);
+		}
+		for (ObjectId have : reachableFrom) {
+			RevObject o = walk.parseAny(have);
+			walk.markUninteresting(o);
+
+			RevObject peeled = walk.peel(o);
+			if (peeled instanceof RevCommit) {
+				// By default, for performance reasons, ObjectWalk does not mark a
+				// tree as uninteresting when we mark a commit. Mark it ourselves so
+				// that we can determine reachability exactly.
+				walk.markUninteresting(((RevCommit) peeled).getTree());
+			}
+		}
+
+		RevCommit commit = walk.next();
+		if (commit != null) {
+			throw new WantNotValidException(commit);
+		}
+		RevObject object = walk.nextObject();
+		if (object != null) {
+			throw new WantNotValidException(object);
+		}
+	}
+
 	private static void checkNotAdvertisedWants(UploadPack up,
 			List<ObjectId> notAdvertisedWants, Set<ObjectId> reachableFrom)
 			throws IOException {
@@ -1888,6 +1919,17 @@ public class UploadPack {
 
 			if (!allWantsAreCommits) {
 				if (!repoHasBitmaps) {
+					if (up.transferConfig.isAllowFilter()) {
+						// Use allowFilter as an indication that the server
+						// operator is willing to pay the cost of these
+						// reachability checks.
+						try (ObjectWalk objWalk = walk.toObjectWalkWithSameObjects()) {
+							checkReachabilityByWalkingObjects(objWalk,
+									wantsAsObjs, reachableFrom);
+						}
+						return;
+					}
+
 					// If unadvertized non-commits are requested, use
 					// bitmaps. If there are no bitmaps, instead of
 					// incurring the expense of a manual walk, reject