BasePackFetchConnection: Avoid full clone with useNegotiationTip

With the useNegotiationTip flag (introduced in change 738dacb), the client sends to the server only the tips of the wanted refs for the negotiation. Some wanted refs may not exist in the client (yet) and our implementation ignores them. So when only non-existing refs are wanted, jgit doesn't send any tips and the server understands it is a full clone.

In useNegotiationTip, send ALL_REFS if any of the wanted refs does not exists locally.

Change-Id: Ide04c5df785b9212abcd9d3cba194515e0af166f

2 files changed, 110 insertions, 21 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 f9687f9f82..278c9f8616 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
@@ -2215,7 +2215,8 @@ public class UploadPackTest {
 
 			uri = testProtocol.register(ctx, server);
 
-			TestProtocol.setFetchConfig(new FetchConfig(true, MAX_HAVES, true));
+			TestProtocol.setFetchConfig(new FetchConfig(true, MAX_HAVES,
+					/* useNegotiationTip= */true));
 			try (Transport tn = testProtocol.open(uri,
 					clientRepo.getRepository(), "server")) {
 
@@ -2335,7 +2336,8 @@ public class UploadPackTest {
 			}, null);
 
 			uri = testProtocol.register(ctx, server);
-			TestProtocol.setFetchConfig(new FetchConfig(true, MAX_HAVES, true));
+			TestProtocol.setFetchConfig(new FetchConfig(true, MAX_HAVES,
+					/* useNegotiationTip= */true));
 			try (Transport tn = testProtocol.open(uri,
 					clientRepo.getRepository(), "server")) {
 
@@ -2362,6 +2364,67 @@ public class UploadPackTest {
 		}
 	}
 
+	/**
+	 * <pre>
+	 * remote:
+	 *    foo &lt;- foofoo &lt;-- branchFoo
+	 *    bar &lt;- barbar &lt;-- branchBar
+	 *
+	 * client:
+	 *    none
+	 *
+	 * fetch(branchFoo) should not send have and should get only branchFoo back
+	 * </pre>
+	 */
+	@Test
+	public void testNegotiationTipDoesNotDoFullClone() throws Exception {
+		RevCommit fooParent = remote.commit().message("foo").create();
+		RevCommit fooChild = remote.commit().message("foofoo").parent(fooParent)
+				.create();
+		RevCommit barParent = remote.commit().message("bar").create();
+		RevCommit barChild = remote.commit().message("barbar").parent(barParent)
+				.create();
+
+		// Remote has branchFoo at fooChild and branchBar at barChild
+		remote.update("branchFoo", fooChild);
+		remote.update("branchBar", barChild);
+
+		AtomicReference<UploadPack> uploadPack = new AtomicReference<>();
+		CountHavesPreUploadHook countHavesHook = new CountHavesPreUploadHook();
+
+		// Client does not have branchFoo & branchBar
+		try (TestRepository<InMemoryRepository> clientRepo = new TestRepository<>(
+				client)) {
+			testProtocol = new TestProtocol<>((Object req, Repository db) -> {
+				UploadPack up = new UploadPack(db);
+				up.setPreUploadHook(countHavesHook);
+				uploadPack.set(up);
+				return up;
+			}, null);
+
+			uri = testProtocol.register(ctx, server);
+
+			TestProtocol.setFetchConfig(new FetchConfig(true, MAX_HAVES,
+					/* useNegotiationTip= */true));
+			try (Transport tn = testProtocol.open(uri,
+					clientRepo.getRepository(), "server")) {
+
+				tn.fetch(NullProgressMonitor.INSTANCE,
+						Collections.singletonList(
+								new RefSpec("refs/heads/branchFoo")),
+						"branchFoo");
+			}
+		}
+
+		assertTrue(client.getObjectDatabase().has(fooParent.toObjectId()));
+		assertTrue(client.getObjectDatabase().has(fooChild.toObjectId()));
+		assertFalse(client.getObjectDatabase().has(barParent.toObjectId()));
+		assertFalse(client.getObjectDatabase().has(barChild.toObjectId()));
+
+		assertEquals(0, uploadPack.get().getStatistics().getHaves());
+		assertTrue(countHavesHook.havesSentDuringNegotiation.isEmpty());
+	}
+
 	private static class CountHavesPreUploadHook implements PreUploadHook {
 		Set<ObjectId> havesSentDuringNegotiation = new HashSet<>();
 
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
index cdda3c0388..e0a351772b 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/BasePackFetchConnection.java
@@ -36,6 +36,8 @@ import java.util.Date;
 import java.util.HashSet;
 import java.util.LinkedHashSet;
 import java.util.List;
+import java.util.Map;
+import java.util.Objects;
 import java.util.Set;
 import java.util.stream.Collectors;
 
@@ -682,29 +684,23 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 	}
 
 	private void markReachable(Collection<Ref> want, Set<ObjectId> have,
-			int maxTime)
-			throws IOException {
-		Set<String> wantRefs = want.stream().map(Ref::getName)
-				.collect(Collectors.toSet());
-
-		for (Ref r : local.getRefDatabase().getRefs()) {
-			if (useNegotiationTip && !wantRefs.contains(r.getName())) {
-				continue;
+			int maxTime) throws IOException {
+		Collection<Ref> refsToMark;
+		if (useNegotiationTip) {
+			refsToMark = translateToLocalTips(want);
+			if (refsToMark.size() < want.size()) {
+				refsToMark.addAll(local.getRefDatabase().getRefs());
 			}
-
-			ObjectId id = r.getPeeledObjectId();
-			if (id == null)
-				id = r.getObjectId();
-			if (id == null)
-				continue;
-			parseReachable(id);
+		} else {
+			refsToMark = local.getRefDatabase().getRefs();
 		}
+		markReachableRefTips(refsToMark);
 
 		for (ObjectId id : local.getAdditionalHaves())
-			parseReachable(id);
+			markReachable(id);
 
 		for (ObjectId id : have)
-			parseReachable(id);
+			markReachable(id);
 
 		if (maxTime > 0) {
 			// Mark reachable commits until we reach maxTime. These may
@@ -731,7 +727,37 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 		}
 	}
 
-	private void parseReachable(ObjectId id) {
+	private Collection<Ref> translateToLocalTips(Collection<Ref> want)
+			throws IOException {
+		String[] refs = want.stream().map(Ref::getName)
+				.collect(Collectors.toSet()).toArray(String[]::new);
+		Map<String, Ref> wantRefMap = local.getRefDatabase().exactRef(refs);
+		return wantRefMap.values().stream()
+				.filter(r -> getRefObjectId(r) != null)
+				.collect(Collectors.toList());
+	}
+
+	/**
+	 * Marks commits reachable.
+	 *
+	 * @param refsToMark
+	 *            references that client is requesting to be marked.
+	 */
+	private void markReachableRefTips(Collection<Ref> refsToMark) {
+		refsToMark.stream().map(BasePackFetchConnection::getRefObjectId)
+				.filter(Objects::nonNull)
+				.forEach(oid -> markReachable(oid));
+	}
+
+	private static ObjectId getRefObjectId(Ref ref) {
+		ObjectId id = ref.getPeeledObjectId();
+		if (id == null) {
+			id = ref.getObjectId();
+		}
+		return id;
+	}
+
+	private void markReachable(ObjectId id) {
 		try {
 			RevCommit o = walk.parseCommit(id);
 			if (!o.has(REACHABLE)) {
@@ -838,7 +864,7 @@ public abstract class BasePackFetchConnection extends BasePackConnection
 		if (statelessRPC && multiAck != MultiAck.DETAILED) {
 			// Our stateless RPC implementation relies upon the detailed
 			// ACK status to tell us common objects for reuse in future
-			// requests.  If its not enabled, we can't talk to the peer.
+			// requests. If its not enabled, we can't talk to the peer.
 			//
 			throw new PackProtocolException(uri, MessageFormat.format(
 					JGitText.get().statelessRPCRequiresOptionToBeEnabled,