Use bitmaps for non-commit reachability checks

Currently, unless RequestPolicy#ANY is used, UploadPack rejects all
non-commit "want" lines unless they were advertized. This is fine,
except when "uploadpack.allowreachablesha1inwant" is true
(corresponding to RequestPolicy#REACHABLE_COMMIT), in which case one
would expect that "want"-ing anything reachable would work.

(There is no restriction that "want" lines must only contain commits -
it is allowed for refs to directly point to trees and blobs, and
requesting for them using "want" lines works.)

This commit has been written to avoid performance regressions as much
as possible. In the usual (and currently working) case where the only
unadvertized things requested are commits, we do a standard RevWalk in
order to avoid incurring the cost of loading bitmaps. However, if
unadvertized non-commits are requested, bitmaps are used instead, and
if there are no bitmaps, a WantNotValidException is thrown (as is
currently done).

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

2 files changed, 120 insertions, 2 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 908bddfa97..a8127abd36 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
@@ -4,24 +4,33 @@ import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
 
 import java.util.Collections;
+import org.eclipse.jgit.errors.TransportException;
+import org.eclipse.jgit.internal.storage.dfs.DfsGarbageCollector;
 import org.eclipse.jgit.internal.storage.dfs.DfsRepositoryDescription;
 import org.eclipse.jgit.internal.storage.dfs.InMemoryRepository;
 import org.eclipse.jgit.junit.TestRepository;
 import org.eclipse.jgit.lib.NullProgressMonitor;
 import org.eclipse.jgit.lib.Repository;
+import org.eclipse.jgit.revwalk.RevBlob;
 import org.eclipse.jgit.revwalk.RevCommit;
 import org.eclipse.jgit.transport.UploadPack.RequestPolicy;
 import org.eclipse.jgit.transport.resolver.ServiceNotAuthorizedException;
 import org.eclipse.jgit.transport.resolver.ServiceNotEnabledException;
 import org.eclipse.jgit.transport.resolver.UploadPackFactory;
+import org.hamcrest.Matchers;
 import org.junit.After;
 import org.junit.Before;
+import org.junit.Rule;
 import org.junit.Test;
+import org.junit.rules.ExpectedException;
 
 /**
  * Tests for server upload-pack utilities.
  */
 public class UploadPackTest {
+	@Rule
+	public ExpectedException thrown = ExpectedException.none();
+
 	private URIish uri;
 
 	private TestProtocol<Object> testProtocol;
@@ -51,6 +60,25 @@ public class UploadPackTest {
 		return new InMemoryRepository(new DfsRepositoryDescription(name));
 	}
 
+	private void generateBitmaps(InMemoryRepository repo) throws Exception {
+		new DfsGarbageCollector(repo).pack(null);
+		repo.scanForRepoChanges();
+	}
+
+	private static TestProtocol<Object> generateReachableCommitUploadPackProtocol() {
+		return new TestProtocol<>(
+				new UploadPackFactory<Object>() {
+					@Override
+					public UploadPack create(Object req, Repository db)
+							throws ServiceNotEnabledException,
+							ServiceNotAuthorizedException {
+						UploadPack up = new UploadPack(db);
+						up.setRequestPolicy(RequestPolicy.REACHABLE_COMMIT);
+						return up;
+					}
+				}, null);
+	}
+
 	@Test
 	public void testFetchParentOfShallowCommit() throws Exception {
 		RevCommit commit0 = remote.commit().message("0").create();
@@ -83,4 +111,63 @@ public class UploadPackTest {
 			assertTrue(client.hasObject(commit0.toObjectId()));
 		}
 	}
+
+	@Test
+	public void testFetchUnreachableBlobWithBitmap() throws Exception {
+		RevBlob blob = remote.blob("foo");
+		remote.commit(remote.tree(remote.file("foo", blob)));
+		generateBitmaps(server);
+
+		testProtocol = generateReachableCommitUploadPackProtocol();
+		uri = testProtocol.register(ctx, server);
+
+		assertFalse(client.hasObject(blob.toObjectId()));
+
+		try (Transport tn = testProtocol.open(uri, client, "server")) {
+			thrown.expect(TransportException.class);
+			thrown.expectMessage(Matchers.containsString(
+						"want " + blob.name() + " not valid"));
+			tn.fetch(NullProgressMonitor.INSTANCE,
+					Collections.singletonList(new RefSpec(blob.name())));
+		}
+	}
+
+	@Test
+	public void testFetchReachableBlobWithBitmap() throws Exception {
+		RevBlob blob = remote.blob("foo");
+		RevCommit commit = remote.commit(remote.tree(remote.file("foo", blob)));
+		remote.update("master", commit);
+		generateBitmaps(server);
+
+		testProtocol = generateReachableCommitUploadPackProtocol();
+		uri = testProtocol.register(ctx, server);
+
+		assertFalse(client.hasObject(blob.toObjectId()));
+
+		try (Transport tn = testProtocol.open(uri, client, "server")) {
+			tn.fetch(NullProgressMonitor.INSTANCE,
+					Collections.singletonList(new RefSpec(blob.name())));
+			assertTrue(client.hasObject(blob.toObjectId()));
+		}
+	}
+
+	@Test
+	public void testFetchReachableBlobWithoutBitmap() throws Exception {
+		RevBlob blob = remote.blob("foo");
+		RevCommit commit = remote.commit(remote.tree(remote.file("foo", blob)));
+		remote.update("master", commit);
+
+		testProtocol = generateReachableCommitUploadPackProtocol();
+		uri = testProtocol.register(ctx, server);
+
+		assertFalse(client.hasObject(blob.toObjectId()));
+
+		try (Transport tn = testProtocol.open(uri, client, "server")) {
+			thrown.expect(TransportException.class);
+			thrown.expectMessage(Matchers.containsString(
+						"want " + blob.name() + " not valid"));
+			tn.fetch(NullProgressMonitor.INSTANCE,
+					Collections.singletonList(new RefSpec(blob.name())));
+		}
+	}
 }
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 cf070c6348..cab532288b 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -77,14 +77,18 @@ import org.eclipse.jgit.errors.MissingObjectException;
 import org.eclipse.jgit.errors.PackProtocolException;
 import org.eclipse.jgit.internal.JGitText;
 import org.eclipse.jgit.internal.storage.pack.PackWriter;
+import org.eclipse.jgit.lib.BitmapIndex;
+import org.eclipse.jgit.lib.BitmapIndex.BitmapBuilder;
 import org.eclipse.jgit.lib.Constants;
 import org.eclipse.jgit.lib.NullProgressMonitor;
 import org.eclipse.jgit.lib.ObjectId;
+import org.eclipse.jgit.lib.ObjectReader;
 import org.eclipse.jgit.lib.ProgressMonitor;
 import org.eclipse.jgit.lib.Ref;
 import org.eclipse.jgit.lib.RefDatabase;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.revwalk.AsyncRevObjectQueue;
+import org.eclipse.jgit.revwalk.BitmapWalker;
 import org.eclipse.jgit.revwalk.DepthWalk;
 import org.eclipse.jgit.revwalk.ObjectWalk;
 import org.eclipse.jgit.revwalk.RevCommit;
@@ -1315,6 +1319,18 @@ public class UploadPack {
 		}
 	}
 
+	private static void checkNotAdvertisedWantsUsingBitmap(ObjectReader reader,
+			BitmapIndex bitmapIndex, List<ObjectId> notAdvertisedWants,
+			Set<ObjectId> reachableFrom) throws IOException {
+		BitmapWalker bitmapWalker = new BitmapWalker(new ObjectWalk(reader), bitmapIndex, null);
+		BitmapBuilder reachables = bitmapWalker.findObjects(reachableFrom, null, false);
+		for (ObjectId oid : notAdvertisedWants) {
+			if (!reachables.contains(oid)) {
+				throw new WantNotValidException(oid);
+			}
+		}
+	}
+
 	private static void checkNotAdvertisedWants(UploadPack up,
 			List<ObjectId> notAdvertisedWants, Set<ObjectId> reachableFrom)
 			throws MissingObjectException, IncorrectObjectTypeException, IOException {
@@ -1324,13 +1340,28 @@ public class UploadPack {
 		// into an advertised branch it will be marked UNINTERESTING and no commits
 		// return.
 
-		try (RevWalk walk = new RevWalk(up.getRevWalk().getObjectReader())) {
+		ObjectReader reader = up.getRevWalk().getObjectReader();
+		try (RevWalk walk = new RevWalk(reader)) {
 			AsyncRevObjectQueue q = walk.parseAny(notAdvertisedWants, true);
 			try {
 				RevObject obj;
 				while ((obj = q.next()) != null) {
-					if (!(obj instanceof RevCommit))
+					if (!(obj instanceof RevCommit)) {
+						// If unadvertized non-commits are requested, use
+						// bitmaps. If there are no bitmaps, instead of
+						// incurring the expense of a manual walk, reject
+						// the request.
+						BitmapIndex bitmapIndex = reader.getBitmapIndex();
+						if (bitmapIndex != null) {
+							checkNotAdvertisedWantsUsingBitmap(
+									reader,
+									bitmapIndex,
+									notAdvertisedWants,
+									reachableFrom);
+							return;
+						}
 						throw new WantNotValidException(obj);
+					}
 					walk.markStart((RevCommit) obj);
 				}
 			} catch (MissingObjectException notFound) {