Accept protocol v2 server options on fetch and ls-refs requests

In protocol v2, a command request can be followed by server options
(lines like "agent=<>" and "server-option=<>"), but current code
doesn't accept those lines.

Advertise the "server-option" capability, parse the lines and add
them to the request objects.

Other code in JGit can see this options and act accordingly via the
protocol v2 hooks.

This should not require any change in the client side.

Change-Id: If3946390f9cc02d29644b6ca52534b6f757bda9f
Signed-off-by: Ivan Frade <ifrade@google.com>

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

@@ -7,6 +7,7 @@ import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.theInstance;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertThat;
 import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
@@ -485,6 +486,8 @@ public class UploadPackTest {
 
 		private LsRefsV2Request lsRefsRequest;
 
+		private FetchV2Request fetchRequest;
+
 		@Override
 		public void onCapabilities(CapabilitiesV2Request req) {
 			capabilitiesRequest = req;
@@ -494,6 +497,11 @@ public class UploadPackTest {
 		public void onLsRefs(LsRefsV2Request req) {
 			lsRefsRequest = req;
 		}
+
+		@Override
+		public void onFetch(FetchV2Request req) {
+			fetchRequest = req;
+		}
 	}
 
 	@Test
@@ -502,18 +510,18 @@ public class UploadPackTest {
 		ByteArrayInputStream recvStream =
 				uploadPackV2Setup(null, null, hook, PacketLineIn.END);
 		PacketLineIn pckIn = new PacketLineIn(recvStream);
-
 		assertThat(hook.capabilitiesRequest, notNullValue());
 		assertThat(pckIn.readString(), is("version 2"));
 		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=shallow"));
+				Arrays.asList(pckIn.readString(), 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=shallow", "server-option"));
 		assertTrue(pckIn.readString() == PacketLineIn.END);
 	}
 
@@ -526,10 +534,11 @@ public class UploadPackTest {
 
 		assertThat(pckIn.readString(), is("version 2"));
 		assertThat(
-			Arrays.asList(pckIn.readString(), pckIn.readString()),
-			// TODO(jonathantanmy) This check overspecifies the
-			// order of the capabilities of "fetch".
-			hasItems("ls-refs", "fetch=filter shallow"));
+				Arrays.asList(pckIn.readString(), pckIn.readString(),
+						pckIn.readString()),
+				// TODO(jonathantanmy) This check overspecifies the
+				// order of the capabilities of "fetch".
+				hasItems("ls-refs", "fetch=filter shallow", "server-option"));
 		assertTrue(pckIn.readString() == PacketLineIn.END);
 	}
 
@@ -542,10 +551,12 @@ public class UploadPackTest {
 
 		assertThat(pckIn.readString(), is("version 2"));
 		assertThat(
-			Arrays.asList(pckIn.readString(), pckIn.readString()),
-			// TODO(jonathantanmy) This check overspecifies the
-			// order of the capabilities of "fetch".
-			hasItems("ls-refs", "fetch=ref-in-want shallow"));
+				Arrays.asList(pckIn.readString(), pckIn.readString(),
+						pckIn.readString()),
+				// TODO(jonathantanmy) This check overspecifies the
+				// order of the capabilities of "fetch".
+				hasItems("ls-refs", "fetch=ref-in-want shallow",
+						"server-option"));
 		assertTrue(pckIn.readString() == PacketLineIn.END);
 	}
 
@@ -558,8 +569,9 @@ public class UploadPackTest {
 
 		assertThat(pckIn.readString(), is("version 2"));
 		assertThat(
-			Arrays.asList(pckIn.readString(), pckIn.readString()),
-			hasItems("ls-refs", "fetch=shallow"));
+				Arrays.asList(pckIn.readString(), pckIn.readString(),
+						pckIn.readString()),
+				hasItems("ls-refs", "fetch=shallow", "server-option"));
 		assertTrue(pckIn.readString() == PacketLineIn.END);
 	}
 
@@ -573,8 +585,9 @@ public class UploadPackTest {
 
 		assertThat(pckIn.readString(), is("version 2"));
 		assertThat(
-			Arrays.asList(pckIn.readString(), pckIn.readString()),
-			hasItems("ls-refs", "fetch=shallow"));
+				Arrays.asList(pckIn.readString(), pckIn.readString(),
+						pckIn.readString()),
+				hasItems("ls-refs", "fetch=shallow", "server-option"));
 		assertTrue(pckIn.readString() == PacketLineIn.END);
 	}
 
@@ -719,6 +732,21 @@ public class UploadPackTest {
 			PacketLineIn.END);
 	}
 
+	@Test
+	public void testV2LsRefsServerOptions() throws Exception {
+		String[] lines = { "command=ls-refs\n",
+				"server-option=one\n", "server-option=two\n",
+				PacketLineIn.DELIM,
+				PacketLineIn.END };
+
+		TestV2Hook testHook = new TestV2Hook();
+		uploadPackV2Setup(null, null, testHook, lines);
+
+		LsRefsV2Request req = testHook.lsRefsRequest;
+		assertEquals(2, req.getServerOptions().size());
+		assertThat(req.getServerOptions(), hasItems("one", "two"));
+	}
+
 	/*
 	 * Parse multiplexed packfile output from upload-pack using protocol V2
 	 * into the client repository.
@@ -1255,6 +1283,21 @@ public class UploadPackTest {
 			PacketLineIn.END);
 	}
 
+	@Test
+	public void testV2FetchServerOptions() throws Exception {
+		String[] lines = { "command=fetch\n", "server-option=one\n",
+				"server-option=two\n", PacketLineIn.DELIM,
+				PacketLineIn.END };
+
+		TestV2Hook testHook = new TestV2Hook();
+		uploadPackV2Setup(null, null, testHook, lines);
+
+		FetchV2Request req = testHook.fetchRequest;
+		assertNotNull(req);
+		assertEquals(2, req.getServerOptions().size());
+		assertThat(req.getServerOptions(), hasItems("one", "two"));
+	}
+
 	@Test
 	public void testV2FetchFilter() throws Exception {
 		RevBlob big = remote.blob("foobar");

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

@@ -45,6 +45,7 @@ package org.eclipse.jgit.transport;
 import static java.util.Objects.requireNonNull;
 
 import java.util.ArrayList;
+import java.util.Collections;
 import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
@@ -52,6 +53,7 @@ import java.util.Set;
 import java.util.TreeMap;
 
 import org.eclipse.jgit.annotations.NonNull;
+import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.lib.ObjectId;
 
 /**
@@ -69,18 +71,27 @@ public final class FetchV2Request extends FetchRequest {
 
 	private final boolean doneReceived;
 
+	@Nullable
+	private final String agent;
+
+	@NonNull
+	private final List<String> serverOptions;
+
 	FetchV2Request(@NonNull List<ObjectId> peerHas,
 			@NonNull TreeMap<String, ObjectId> wantedRefs,
 			@NonNull Set<ObjectId> wantIds,
 			@NonNull Set<ObjectId> clientShallowCommits, int deepenSince,
 			@NonNull List<String> deepenNotRefs, int depth,
 			long filterBlobLimit,
-			boolean doneReceived, @NonNull Set<String> clientCapabilities) {
+			boolean doneReceived, @NonNull Set<String> clientCapabilities,
+			@Nullable String agent, @NonNull List<String> serverOptions) {
 		super(wantIds, depth, clientShallowCommits, filterBlobLimit,
 				clientCapabilities, deepenSince, deepenNotRefs);
 		this.peerHas = requireNonNull(peerHas);
 		this.wantedRefs = requireNonNull(wantedRefs);
 		this.doneReceived = doneReceived;
+		this.agent = agent;
+		this.serverOptions = requireNonNull(serverOptions);
 	}
 
 	/**
@@ -106,6 +117,28 @@ public final class FetchV2Request extends FetchRequest {
 		return doneReceived;
 	}
 
+	/**
+	 * @return string identifying the agent (as sent in the request body by the
+	 *         client)
+	 */
+	@Nullable
+	String getAgent() {
+		return agent;
+	}
+
+	/**
+	 * Options received in server-option lines. The caller can choose to act on
+	 * these in an application-specific way
+	 *
+	 * @return Immutable list of server options received in the request
+	 *
+	 * @since 5.2
+	 */
+	@NonNull
+	public List<String> getServerOptions() {
+		return serverOptions;
+	}
+
 	/** @return A builder of {@link FetchV2Request}. */
 	static Builder builder() {
 		return new Builder();
@@ -133,6 +166,11 @@ public final class FetchV2Request extends FetchRequest {
 
 		boolean doneReceived;
 
+		@Nullable
+		String agent;
+
+		final List<String> serverOptions = new ArrayList<>();
+
 		private Builder() {
 		}
 
@@ -264,13 +302,43 @@ public final class FetchV2Request extends FetchRequest {
 			return this;
 		}
 
+		/**
+		 * Value of an agent line received after the command and before the
+		 * arguments. E.g. "agent=a.b.c/1.0" should set "a.b.c/1.0".
+		 *
+		 * @param agentValue
+		 *            the client-supplied agent capability, without the leading
+		 *            "agent="
+		 * @return this builder
+		 */
+		Builder setAgent(@Nullable String agentValue) {
+			agent = agentValue;
+			return this;
+		}
+
+		/**
+		 * Records an application-specific option supplied in a server-option
+		 * line, for later retrieval with
+		 * {@link FetchV2Request#getServerOptions}.
+		 *
+		 * @param value
+		 *            the client-supplied server-option capability, without
+		 *            leading "server-option=".
+		 * @return this builder
+		 */
+		Builder addServerOption(@NonNull String value) {
+			serverOptions.add(value);
+			return this;
+		}
+
 		/**
 		 * @return Initialized fetch request
 		 */
 		FetchV2Request build() {
 			return new FetchV2Request(peerHas, wantedRefs, wantIds,
 					clientShallowCommits, deepenSince, deepenNotRefs,
-					depth, filterBlobLimit, doneReceived, clientCapabilities);
+					depth, filterBlobLimit, doneReceived, clientCapabilities,
+					agent, Collections.unmodifiableList(serverOptions));
 		}
 	}
 }

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

@@ -244,6 +244,20 @@ public final class GitProtocolConstants {
 	 */
 	public static final String CAPABILITY_REF_IN_WANT = "ref-in-want"; //$NON-NLS-1$
 
+	/**
+	 * The server supports arbitrary options
+	 *
+	 * @since 5.2
+	 */
+	public static final String CAPABILITY_SERVER_OPTION = "server-option"; //$NON-NLS-1$
+
+	/**
+	 * Option for passing application-specific options to the server.
+	 *
+	 * @since 5.2
+	 */
+	public static final String OPTION_SERVER_OPTION = "server-option"; //$NON-NLS-1$
+
 	/**
 	 * The server supports listing refs using protocol v2.
 	 *

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

@@ -42,9 +42,15 @@
  */
 package org.eclipse.jgit.transport;
 
+import static java.util.Objects.requireNonNull;
+
+import java.util.ArrayList;
 import java.util.Collections;
 import java.util.List;
 
+import org.eclipse.jgit.annotations.NonNull;
+import org.eclipse.jgit.annotations.Nullable;
+
 /**
  * ls-refs protocol v2 request.
  *
@@ -60,11 +66,20 @@ public final class LsRefsV2Request {
 
 	private final boolean peel;
 
+	@Nullable
+	private final String agent;
+
+	@NonNull
+	private final List<String> serverOptions;
+
 	private LsRefsV2Request(List<String> refPrefixes, boolean symrefs,
-			boolean peel) {
+			boolean peel, @Nullable String agent,
+			@NonNull List<String> serverOptions) {
 		this.refPrefixes = refPrefixes;
 		this.symrefs = symrefs;
 		this.peel = peel;
+		this.agent = agent;
+		this.serverOptions = requireNonNull(serverOptions);
 	}
 
 	/** @return ref prefixes that the client requested. */
@@ -82,6 +97,34 @@ public final class LsRefsV2Request {
 		return peel;
 	}
 
+	/**
+	 * @return agent as reported by the client
+	 *
+	 * @since 5.2
+	 */
+	@Nullable
+	public String getAgent() {
+		return agent;
+	}
+
+	/**
+	 * Get application-specific options provided by the client using
+	 * --server-option.
+	 * <p>
+	 * It returns just the content, without the "server-option=" prefix. E.g. a
+	 * request with server-option=A and server-option=B lines returns the list
+	 * [A, B].
+	 *
+	 * @return application-specific options from the client as an unmodifiable
+	 *         list
+	 *
+	 * @since 5.2
+	 */
+	@NonNull
+	public List<String> getServerOptions() {
+		return serverOptions;
+	}
+
 	/** @return A builder of {@link LsRefsV2Request}. */
 	public static Builder builder() {
 		return new Builder();
@@ -95,6 +138,10 @@ public final class LsRefsV2Request {
 
 		private boolean peel;
 
+		private final List<String> serverOptions = new ArrayList<>();
+
+		private String agent;
+
 		private Builder() {
 		}
 
@@ -125,10 +172,43 @@ public final class LsRefsV2Request {
 			return this;
 		}
 
+		/**
+		 * Records an application-specific option supplied in a server-option
+		 * line, for later retrieval with
+		 * {@link LsRefsV2Request#getServerOptions}.
+		 *
+		 * @param value
+		 *            the client-supplied server-option capability, without
+		 *            leading "server-option=".
+		 * @return this builder
+		 * @since 5.2
+		 */
+		public Builder addServerOption(@NonNull String value) {
+			serverOptions.add(value);
+			return this;
+		}
+
+		/**
+		 * Value of an agent line received after the command and before the
+		 * arguments. E.g. "agent=a.b.c/1.0" should set "a.b.c/1.0".
+		 *
+		 * @param value
+		 *            the client-supplied agent capability, without leading
+		 *            "agent="
+		 * @return this builder
+		 *
+		 * @since 5.2
+		 */
+		public Builder setAgent(@Nullable String value) {
+			agent = value;
+			return this;
+		}
+
 		/** @return LsRefsV2Request */
 		public LsRefsV2Request build() {
 			return new LsRefsV2Request(
-					Collections.unmodifiableList(refPrefixes), symrefs, peel);
+					Collections.unmodifiableList(refPrefixes), symrefs, peel,
+					agent, Collections.unmodifiableList(serverOptions));
 		}
 	}
 }

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

@@ -44,9 +44,11 @@ package org.eclipse.jgit.transport;
 
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_DEEPEN_RELATIVE;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_FILTER;
+import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_AGENT;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_INCLUDE_TAG;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_NO_PROGRESS;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_OFS_DELTA;
+import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_SERVER_OPTION;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_SIDE_BAND_64K;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_THIN_PACK;
 import static org.eclipse.jgit.transport.GitProtocolConstants.OPTION_WANT_REF;
@@ -55,6 +57,7 @@ import java.io.IOException;
 import java.text.MessageFormat;
 import java.util.ArrayList;
 import java.util.List;
+import java.util.function.Consumer;
 
 import org.eclipse.jgit.errors.PackProtocolException;
 import org.eclipse.jgit.internal.JGitText;
@@ -77,6 +80,35 @@ final class ProtocolV2Parser {
 		this.transferConfig = transferConfig;
 	}
 
+	/*
+	 * Read lines until DELIM or END, calling the appropiate consumer.
+	 *
+	 * Returns the last read line (so caller can check if there is more to read
+	 * in the line).
+	 */
+	private static String consumeCapabilities(PacketLineIn pckIn,
+			Consumer<String> serverOptionConsumer,
+			Consumer<String> agentConsumer) throws IOException {
+
+		String serverOptionPrefix = OPTION_SERVER_OPTION + '=';
+		String agentPrefix = OPTION_AGENT + '=';
+
+		String line = pckIn.readString();
+		while (line != PacketLineIn.DELIM && line != PacketLineIn.END) {
+			if (line.startsWith(serverOptionPrefix)) {
+				serverOptionConsumer
+						.accept(line.substring(serverOptionPrefix.length()));
+			} else if (line.startsWith(agentPrefix)) {
+				agentConsumer.accept(line.substring(agentPrefix.length()));
+			} else {
+				// Unrecognized capability. Ignore it.
+			}
+			line = pckIn.readString();
+		}
+
+		return line;
+	}
+
 	/**
 	 * Parse the incoming fetch request arguments from the wire. The caller must
 	 * be sure that what is comings is a fetch request before coming here.
@@ -106,13 +138,18 @@ final class ProtocolV2Parser {
 		// lengths.
 		reqBuilder.addClientCapability(OPTION_SIDE_BAND_64K);
 
-		String line;
+		String line = consumeCapabilities(pckIn,
+				serverOption -> reqBuilder.addServerOption(serverOption),
+				agent -> reqBuilder.setAgent(agent));
 
-		// Currently, we do not support any capabilities, so the next
-		// line is DELIM.
-		if ((line = pckIn.readString()) != PacketLineIn.DELIM) {
-			throw new PackProtocolException(MessageFormat
-					.format(JGitText.get().unexpectedPacketLine, line));
+		if (line == PacketLineIn.END) {
+			return reqBuilder.build();
+		}
+
+		if (line != PacketLineIn.DELIM) {
+			throw new PackProtocolException(
+					MessageFormat.format(JGitText.get().unexpectedPacketLine,
+							line));
 		}
 
 		boolean filterReceived = false;
@@ -226,27 +263,33 @@ final class ProtocolV2Parser {
 			throws PackProtocolException, IOException {
 		LsRefsV2Request.Builder builder = LsRefsV2Request.builder();
 		List<String> prefixes = new ArrayList<>();
-		String line = pckIn.readString();
-		// Currently, we do not support any capabilities, so the next
-		// line is DELIM if there are arguments or END if not.
-		if (line == PacketLineIn.DELIM) {
-			while ((line = pckIn.readString()) != PacketLineIn.END) {
-				if (line.equals("peel")) { //$NON-NLS-1$
-					builder.setPeel(true);
-				} else if (line.equals("symrefs")) { //$NON-NLS-1$
-					builder.setSymrefs(true);
-				} else if (line.startsWith("ref-prefix ")) { //$NON-NLS-1$
-					prefixes.add(line.substring("ref-prefix ".length())); //$NON-NLS-1$
-				} else {
-					throw new PackProtocolException(MessageFormat
-							.format(JGitText.get().unexpectedPacketLine, line));
-				}
-			}
-		} else if (line != PacketLineIn.END) {
+
+		String line = consumeCapabilities(pckIn,
+				serverOption -> builder.addServerOption(serverOption),
+				agent -> builder.setAgent(agent));
+
+		if (line == PacketLineIn.END) {
+			return builder.build();
+		}
+
+		if (line != PacketLineIn.DELIM) {
 			throw new PackProtocolException(MessageFormat
 					.format(JGitText.get().unexpectedPacketLine, line));
 		}
 
+		while ((line = pckIn.readString()) != PacketLineIn.END) {
+			if (line.equals("peel")) { //$NON-NLS-1$
+				builder.setPeel(true);
+			} else if (line.equals("symrefs")) { //$NON-NLS-1$
+				builder.setSymrefs(true);
+			} else if (line.startsWith("ref-prefix ")) { //$NON-NLS-1$
+				prefixes.add(line.substring("ref-prefix ".length())); //$NON-NLS-1$
+			} else {
+				throw new PackProtocolException(MessageFormat
+						.format(JGitText.get().unexpectedPacketLine, line));
+			}
+		}
+
 		return builder.setRefPrefixes(prefixes).build();
 	}
 

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

@@ -48,6 +48,7 @@ import static org.eclipse.jgit.lib.RefDatabase.ALL;
 import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_REF_IN_WANT;
 import static org.eclipse.jgit.transport.GitProtocolConstants.COMMAND_FETCH;
 import static org.eclipse.jgit.transport.GitProtocolConstants.COMMAND_LS_REFS;
+import static org.eclipse.jgit.transport.GitProtocolConstants.CAPABILITY_SERVER_OPTION;
 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;
@@ -1082,6 +1083,7 @@ public class UploadPack {
 				(transferConfig.isAllowFilter() ? OPTION_FILTER + ' ' : "") + //$NON-NLS-1$
 				(advertiseRefInWant ? CAPABILITY_REF_IN_WANT + ' ' : "") + //$NON-NLS-1$
 				OPTION_SHALLOW);
+		caps.add(CAPABILITY_SERVER_OPTION);
 		return caps;
 	}