From cbccfed4b39fd49cdb5366194b516a32ace58d62 Mon Sep 17 00:00:00 2001 From: Jonathan Tan Date: Thu, 23 May 2019 13:28:53 -0700 Subject: [PATCH] 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 --- .../jgit/transport/UploadPackTest.java | 29 +++++++++++++ .../eclipse/jgit/transport/UploadPack.java | 42 +++++++++++++++++++ 2 files changed, 71 insertions(+) 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 9b090e894..fb7afbc37 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 @@ -194,6 +194,35 @@ public class UploadPackTest { } } + @Test + public void testFetchReachableBlobWithoutBitmapButFilterAllowed() throws Exception { + InMemoryRepository server2 = newRepo("server2"); + try (TestRepository 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"); 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 17632c10a..db0d26746 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 wants, Set 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 notAdvertisedWants, Set 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