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>

1 files changed, 87 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 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())));
+		}
+	}
 }