Teach UploadPack basic "fetch" command

Add basic support for the "fetch" command in the fetch-pack/upload-pack
protocol v2. This patch teaches "have" and "done".

The protocol specification (Documentation/technical/protocol-v2.txt in
the Git project) states:

    want <oid>
	Indicates to the server an object which the client wants to
	retrieve.  Wants can be anything and are not limited to
	advertised objects.

It is unspecified whether the server should respect the
uploadpack.allowtipsha1inwant option etc. when serving packfiles. This
patch is conservative in that the server respects them.

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

org.eclipse.jgit.test/tst/org/eclipse/jgit/transport/UploadPackTest.java

@@ -1,19 +1,26 @@
 package org.eclipse.jgit.transport;
 
+import static org.hamcrest.Matchers.hasItems;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.theInstance;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 
+import java.util.Arrays;
 import java.util.Collections;
+import java.util.HashMap;
+import java.util.Map;
 import java.io.ByteArrayInputStream;
 import java.io.ByteArrayOutputStream;
+import java.io.StringWriter;
 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.Ref;
 import org.eclipse.jgit.lib.Repository;
 import org.eclipse.jgit.lib.Sets;
 import org.eclipse.jgit.revwalk.RevBlob;
@@ -24,6 +31,7 @@ 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.eclipse.jgit.util.io.NullOutputStream;
 import org.hamcrest.Matchers;
 import org.junit.After;
 import org.junit.Before;
@@ -344,7 +352,8 @@ public class UploadPackTest {
 	 * Returns UploadPack's output stream, not including the capability
 	 * advertisement by the server.
 	 */
-	private ByteArrayInputStream uploadPackV2(String... inputLines) throws Exception {
+	private ByteArrayInputStream uploadPackV2(RequestPolicy requestPolicy,
+			RefFilter refFilter, String... inputLines) throws Exception {
 		ByteArrayOutputStream send = new ByteArrayOutputStream();
 		PacketLineOut pckOut = new PacketLineOut(send);
 		for (String line : inputLines) {
@@ -359,6 +368,10 @@ public class UploadPackTest {
 
 		server.getConfig().setString("protocol", null, "version", "2");
 		UploadPack up = new UploadPack(server);
+		if (requestPolicy != null)
+			up.setRequestPolicy(requestPolicy);
+		if (refFilter != null)
+			up.setRefFilter(refFilter);
 		up.setExtraParameters(Sets.of("version=2"));
 
 		ByteArrayOutputStream recv = new ByteArrayOutputStream();
@@ -369,11 +382,23 @@ public class UploadPackTest {
 
 		// capability advertisement (always sent)
 		assertThat(pckIn.readString(), is("version 2"));
-		assertThat(pckIn.readString(), is("ls-refs"));
+		assertThat(
+			Arrays.asList(pckIn.readString(), pckIn.readString()),
+			// TODO(jonathantanmy) This check is written this way
+			// to make it simple to see that we expect this list of
+			// capabilities, but probably should be loosened to
+			// allow additional commands to be added to the list,
+			// and additional capabilities to be added to existing
+			// commands without requiring test changes.
+			hasItems("ls-refs", "fetch"));
 		assertTrue(pckIn.readString() == PacketLineIn.END);
 		return recvStream;
 	}
 
+	private ByteArrayInputStream uploadPackV2(String... inputLines) throws Exception {
+		return uploadPackV2(null, null, inputLines);
+	}
+
 	@Test
 	public void testV2EmptyRequest() throws Exception {
 		ByteArrayInputStream recvStream = uploadPackV2(PacketLineIn.END);
@@ -500,4 +525,235 @@ public class UploadPackTest {
 		assertThat(pckIn.readString(), is(tip.toObjectId().getName() + " refs/heads/other"));
 		assertTrue(pckIn.readString() == PacketLineIn.END);
 	}
+
+	/*
+	 * Parse multiplexed packfile output from upload-pack using protocol V2
+	 * into the client repository.
+	 */
+	private void parsePack(ByteArrayInputStream recvStream) throws Exception {
+		SideBandInputStream sb = new SideBandInputStream(
+				recvStream, NullProgressMonitor.INSTANCE,
+				new StringWriter(), NullOutputStream.INSTANCE);
+		client.newObjectInserter().newPackParser(sb).parse(NullProgressMonitor.INSTANCE);
+	}
+
+	@Test
+	public void testV2FetchRequestPolicyAdvertised() throws Exception {
+		RevCommit advertized = remote.commit().message("x").create();
+		RevCommit unadvertized = remote.commit().message("y").create();
+		remote.update("branch1", advertized);
+
+		// This works
+		uploadPackV2(
+			RequestPolicy.ADVERTISED,
+			null,
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"want " + advertized.name() + "\n",
+			PacketLineIn.END);
+
+		// This doesn't
+		thrown.expect(TransportException.class);
+		thrown.expectMessage(Matchers.containsString(
+					"want " + unadvertized.name() + " not valid"));
+		uploadPackV2(
+			RequestPolicy.ADVERTISED,
+			null,
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"want " + unadvertized.name() + "\n",
+			PacketLineIn.END);
+	}
+
+	@Test
+	public void testV2FetchRequestPolicyReachableCommit() throws Exception {
+		RevCommit reachable = remote.commit().message("x").create();
+		RevCommit advertized = remote.commit().message("x").parent(reachable).create();
+		RevCommit unreachable = remote.commit().message("y").create();
+		remote.update("branch1", advertized);
+
+		// This works
+		uploadPackV2(
+			RequestPolicy.REACHABLE_COMMIT,
+			null,
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"want " + reachable.name() + "\n",
+			PacketLineIn.END);
+
+		// This doesn't
+		thrown.expect(TransportException.class);
+		thrown.expectMessage(Matchers.containsString(
+					"want " + unreachable.name() + " not valid"));
+		uploadPackV2(
+			RequestPolicy.REACHABLE_COMMIT,
+			null,
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"want " + unreachable.name() + "\n",
+			PacketLineIn.END);
+	}
+
+	@Test
+	public void testV2FetchRequestPolicyTip() throws Exception {
+		RevCommit parentOfTip = remote.commit().message("x").create();
+		RevCommit tip = remote.commit().message("y").parent(parentOfTip).create();
+		remote.update("secret", tip);
+
+		// This works
+		uploadPackV2(
+			RequestPolicy.TIP,
+			new RejectAllRefFilter(),
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"want " + tip.name() + "\n",
+			PacketLineIn.END);
+
+		// This doesn't
+		thrown.expect(TransportException.class);
+		thrown.expectMessage(Matchers.containsString(
+					"want " + parentOfTip.name() + " not valid"));
+		uploadPackV2(
+			RequestPolicy.TIP,
+			new RejectAllRefFilter(),
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"want " + parentOfTip.name() + "\n",
+			PacketLineIn.END);
+	}
+
+	@Test
+	public void testV2FetchRequestPolicyReachableCommitTip() throws Exception {
+		RevCommit parentOfTip = remote.commit().message("x").create();
+		RevCommit tip = remote.commit().message("y").parent(parentOfTip).create();
+		RevCommit unreachable = remote.commit().message("y").create();
+		remote.update("secret", tip);
+
+		// This works
+		uploadPackV2(
+			RequestPolicy.REACHABLE_COMMIT_TIP,
+			new RejectAllRefFilter(),
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"want " + parentOfTip.name() + "\n",
+			PacketLineIn.END);
+
+		// This doesn't
+		thrown.expect(TransportException.class);
+		thrown.expectMessage(Matchers.containsString(
+					"want " + unreachable.name() + " not valid"));
+		uploadPackV2(
+			RequestPolicy.REACHABLE_COMMIT_TIP,
+			new RejectAllRefFilter(),
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"want " + unreachable.name() + "\n",
+			PacketLineIn.END);
+	}
+
+	@Test
+	public void testV2FetchRequestPolicyAny() throws Exception {
+		RevCommit unreachable = remote.commit().message("y").create();
+
+		// Exercise to make sure that even unreachable commits can be fetched
+		uploadPackV2(
+			RequestPolicy.ANY,
+			null,
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"want " + unreachable.name() + "\n",
+			PacketLineIn.END);
+	}
+
+	@Test
+	public void testV2FetchServerDoesNotStopNegotiation() throws Exception {
+		RevCommit fooParent = remote.commit().message("x").create();
+		RevCommit fooChild = remote.commit().message("x").parent(fooParent).create();
+		RevCommit barParent = remote.commit().message("y").create();
+		RevCommit barChild = remote.commit().message("y").parent(barParent).create();
+		remote.update("branch1", fooChild);
+		remote.update("branch2", barChild);
+
+		ByteArrayInputStream recvStream = uploadPackV2(
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"want " + fooChild.toObjectId().getName() + "\n",
+			"want " + barChild.toObjectId().getName() + "\n",
+			"have " + fooParent.toObjectId().getName() + "\n",
+			PacketLineIn.END);
+		PacketLineIn pckIn = new PacketLineIn(recvStream);
+
+		assertThat(pckIn.readString(), is("acknowledgments"));
+		assertThat(pckIn.readString(), is("ACK " + fooParent.toObjectId().getName()));
+		assertThat(pckIn.readString(), theInstance(PacketLineIn.END));
+	}
+
+	@Test
+	public void testV2FetchServerStopsNegotiation() throws Exception {
+		RevCommit fooParent = remote.commit().message("x").create();
+		RevCommit fooChild = remote.commit().message("x").parent(fooParent).create();
+		RevCommit barParent = remote.commit().message("y").create();
+		RevCommit barChild = remote.commit().message("y").parent(barParent).create();
+		remote.update("branch1", fooChild);
+		remote.update("branch2", barChild);
+
+		ByteArrayInputStream recvStream = uploadPackV2(
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"want " + fooChild.toObjectId().getName() + "\n",
+			"want " + barChild.toObjectId().getName() + "\n",
+			"have " + fooParent.toObjectId().getName() + "\n",
+			"have " + barParent.toObjectId().getName() + "\n",
+			PacketLineIn.END);
+		PacketLineIn pckIn = new PacketLineIn(recvStream);
+
+		assertThat(pckIn.readString(), is("acknowledgments"));
+		assertThat(
+			Arrays.asList(pckIn.readString(), pckIn.readString()),
+			hasItems(
+				"ACK " + fooParent.toObjectId().getName(),
+				"ACK " + barParent.toObjectId().getName()));
+		assertThat(pckIn.readString(), is("ready"));
+		assertThat(pckIn.readString(), theInstance(PacketLineIn.DELIM));
+		assertThat(pckIn.readString(), is("packfile"));
+		parsePack(recvStream);
+		assertFalse(client.hasObject(fooParent.toObjectId()));
+		assertTrue(client.hasObject(fooChild.toObjectId()));
+		assertFalse(client.hasObject(barParent.toObjectId()));
+		assertTrue(client.hasObject(barChild.toObjectId()));
+	}
+
+	@Test
+	public void testV2FetchClientStopsNegotiation() throws Exception {
+		RevCommit fooParent = remote.commit().message("x").create();
+		RevCommit fooChild = remote.commit().message("x").parent(fooParent).create();
+		RevCommit barParent = remote.commit().message("y").create();
+		RevCommit barChild = remote.commit().message("y").parent(barParent).create();
+		remote.update("branch1", fooChild);
+		remote.update("branch2", barChild);
+
+		ByteArrayInputStream recvStream = uploadPackV2(
+			"command=fetch\n",
+			PacketLineIn.DELIM,
+			"want " + fooChild.toObjectId().getName() + "\n",
+			"want " + barChild.toObjectId().getName() + "\n",
+			"have " + fooParent.toObjectId().getName() + "\n",
+			"done\n",
+			PacketLineIn.END);
+		PacketLineIn pckIn = new PacketLineIn(recvStream);
+
+		assertThat(pckIn.readString(), is("packfile"));
+		parsePack(recvStream);
+		assertFalse(client.hasObject(fooParent.toObjectId()));
+		assertTrue(client.hasObject(fooChild.toObjectId()));
+		assertTrue(client.hasObject(barParent.toObjectId()));
+		assertTrue(client.hasObject(barChild.toObjectId()));
+	}
+
+	private static class RejectAllRefFilter implements RefFilter {
+		@Override
+		public Map<String, Ref> filter(Map<String, Ref> refs) {
+			return new HashMap<String, Ref>();
+		}
+	};
 }

org.eclipse.jgit/src/org/eclipse/jgit/transport/GitProtocolConstants.java

@@ -229,6 +229,13 @@ public class GitProtocolConstants {
 	 */
 	public static final String COMMAND_LS_REFS = "ls-refs"; //$NON-NLS-1$
 
+	/**
+	 * The server supports fetch using protocol v2.
+	 *
+	 * @since 5.0
+	 */
+	public static final String COMMAND_FETCH = "fetch"; //$NON-NLS-1$
+
 	static enum MultiAck {
 		OFF, CONTINUE, DETAILED;
 	}

org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java

@@ -45,6 +45,7 @@ package org.eclipse.jgit.transport;
 
 import static org.eclipse.jgit.lib.RefDatabase.ALL;
 import static org.eclipse.jgit.transport.GitProtocolConstants.COMMAND_LS_REFS;
+import static org.eclipse.jgit.transport.GitProtocolConstants.COMMAND_FETCH;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_AGENT;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_ALLOW_REACHABLE_SHA1_IN_WANT;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_ALLOW_TIP_SHA1_IN_WANT;
@@ -119,7 +120,8 @@ public class UploadPack {
 	// supports protocol version 2.
 	private static final String[] v2CapabilityAdvertisement = {
 		"version 2",
-		COMMAND_LS_REFS
+		COMMAND_LS_REFS,
+		COMMAND_FETCH
 	};
 
 	/** Policy the server uses to validate client requests */
@@ -916,6 +918,74 @@ public class UploadPack {
 		adv.end();
 	}
 
+	private void fetchV2() throws IOException {
+		options = new HashSet<>();
+
+		// Packs are always sent multiplexed and using full 64K
+		// lengths.
+		options.add(OPTION_SIDE_BAND_64K);
+
+		// Depending on the requestValidator, #processHaveLines may
+		// require that advertised be set. Set it only in the required
+		// circumstances (to avoid a full ref lookup in the case that
+		// we don't need it).
+		if (requestValidator instanceof TipRequestValidator ||
+				requestValidator instanceof ReachableCommitTipRequestValidator ||
+				requestValidator instanceof AnyRequestValidator) {
+			advertised = Collections.emptySet();
+		} else {
+			advertised = refIdSet(getAdvertisedOrDefaultRefs().values());
+		}
+
+		String line;
+		List<ObjectId> peerHas = new ArrayList<>();
+		boolean doneReceived = false;
+
+		// Currently, we do not support any capabilities, so the next
+		// line is DELIM.
+		if ((line = pckIn.readString()) != PacketLineIn.DELIM) {
+			throw new PackProtocolException("unexpected " + line);
+		}
+
+		while ((line = pckIn.readString()) != PacketLineIn.END) {
+			if (line.startsWith("want ")) {
+				wantIds.add(ObjectId.fromString(line.substring(5)));
+			} else if (line.startsWith("have ")) {
+				peerHas.add(ObjectId.fromString(line.substring(5)));
+			} else if (line.equals("done")) {
+				doneReceived = true;
+			}
+			// else ignore it
+		}
+		rawOut.stopBuffering();
+
+		boolean sectionSent = false;
+		if (doneReceived) {
+			processHaveLines(peerHas, ObjectId.zeroId(), new PacketLineOut(NullOutputStream.INSTANCE));
+		} else {
+			pckOut.writeString("acknowledgments\n");
+			for (ObjectId id : peerHas) {
+				if (walk.getObjectReader().has(id)) {
+					pckOut.writeString("ACK " + id.getName() + "\n");
+				}
+			}
+			processHaveLines(peerHas, ObjectId.zeroId(), new PacketLineOut(NullOutputStream.INSTANCE));
+			if (okToGiveUp()) {
+				pckOut.writeString("ready\n");
+			} else if (commonBase.isEmpty()) {
+				pckOut.writeString("NAK\n");
+			}
+			sectionSent = true;
+		}
+		if (doneReceived || okToGiveUp()) {
+			if (sectionSent)
+				pckOut.writeDelim();
+			pckOut.writeString("packfile\n");
+			sendPack(new PackStatistics.Accumulator());
+		}
+		pckOut.end();
+	}
+
 	/*
 	 * Returns true if this is the last command and we should tear down the
 	 * connection.
@@ -938,6 +1008,10 @@ public class UploadPack {
 			lsRefsV2();
 			return false;
 		}
+		if (command.equals("command=" + COMMAND_FETCH)) {
+			fetchV2();
+			return false;
+		}
 		throw new PackProtocolException("unknown command " + command);
 	}
 
@@ -1281,7 +1355,7 @@ public class UploadPack {
 			}
 
 			if (line == PacketLineIn.END) {
-				last = processHaveLines(peerHas, last);
+				last = processHaveLines(peerHas, last, pckOut);
 				if (commonBase.isEmpty() || multiAck != MultiAck.OFF)
 					pckOut.writeString("NAK\n"); //$NON-NLS-1$
 				if (noDone && sentReady) {
@@ -1296,7 +1370,7 @@ public class UploadPack {
 				peerHas.add(ObjectId.fromString(line.substring(5)));
 				accumulator.haves++;
 			} else if (line.equals("done")) { //$NON-NLS-1$
-				last = processHaveLines(peerHas, last);
+				last = processHaveLines(peerHas, last, pckOut);
 
 				if (commonBase.isEmpty())
 					pckOut.writeString("NAK\n"); //$NON-NLS-1$
@@ -1312,7 +1386,7 @@ public class UploadPack {
 		}
 	}
 
-	private ObjectId processHaveLines(List<ObjectId> peerHas, ObjectId last)
+	private ObjectId processHaveLines(List<ObjectId> peerHas, ObjectId last, PacketLineOut out)
 			throws IOException {
 		preUploadHook.onBeginNegotiateRound(this, wantIds, peerHas.size());
 		if (wantAll.isEmpty() && !wantIds.isEmpty())
@@ -1357,13 +1431,13 @@ public class UploadPack {
 				switch (multiAck) {
 				case OFF:
 					if (commonBase.size() == 1)
-						pckOut.writeString("ACK " + obj.name() + "\n"); //$NON-NLS-1$ //$NON-NLS-2$
+						out.writeString("ACK " + obj.name() + "\n"); //$NON-NLS-1$ //$NON-NLS-2$
 					break;
 				case CONTINUE:
-					pckOut.writeString("ACK " + obj.name() + " continue\n"); //$NON-NLS-1$ //$NON-NLS-2$
+					out.writeString("ACK " + obj.name() + " continue\n"); //$NON-NLS-1$ //$NON-NLS-2$
 					break;
 				case DETAILED:
-					pckOut.writeString("ACK " + obj.name() + " common\n"); //$NON-NLS-1$ //$NON-NLS-2$
+					out.writeString("ACK " + obj.name() + " common\n"); //$NON-NLS-1$ //$NON-NLS-2$
 					break;
 				}
 			}
@@ -1389,10 +1463,10 @@ public class UploadPack {
 						case OFF:
 							break;
 						case CONTINUE:
-							pckOut.writeString("ACK " + id.name() + " continue\n"); //$NON-NLS-1$ //$NON-NLS-2$
+							out.writeString("ACK " + id.name() + " continue\n"); //$NON-NLS-1$ //$NON-NLS-2$
 							break;
 						case DETAILED:
-							pckOut.writeString("ACK " + id.name() + " ready\n"); //$NON-NLS-1$ //$NON-NLS-2$
+							out.writeString("ACK " + id.name() + " ready\n"); //$NON-NLS-1$ //$NON-NLS-2$
 							sentReady = true;
 							break;
 						}
@@ -1404,7 +1478,7 @@ public class UploadPack {
 
 		if (multiAck == MultiAck.DETAILED && !didOkToGiveUp && okToGiveUp()) {
 			ObjectId id = peerHas.get(peerHas.size() - 1);
-			pckOut.writeString("ACK " + id.name() + " ready\n"); //$NON-NLS-1$ //$NON-NLS-2$
+			out.writeString("ACK " + id.name() + " ready\n"); //$NON-NLS-1$ //$NON-NLS-2$
 			sentReady = true;
 		}