From: Ivan Frade Date: Thu, 26 Jul 2018 22:49:09 +0000 (-0700) Subject: Accept protocol v2 server options on fetch and ls-refs requests X-Git-Tag: v5.2.0.201811281532-m3~76 X-Git-Url: https://source.dussan.org/?a=commitdiff_plain;h=40f5b28545989f7e200f6e0b2007459d63279103;p=jgit.git 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 --- 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 20fb12b3ef..aaef45883a 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 @@ -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"); diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java index cd3906ea61..951d2d79ea 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java +++ b/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 serverOptions; + FetchV2Request(@NonNull List peerHas, @NonNull TreeMap wantedRefs, @NonNull Set wantIds, @NonNull Set clientShallowCommits, int deepenSince, @NonNull List deepenNotRefs, int depth, long filterBlobLimit, - boolean doneReceived, @NonNull Set clientCapabilities) { + boolean doneReceived, @NonNull Set clientCapabilities, + @Nullable String agent, @NonNull List 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 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 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)); } } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/GitProtocolConstants.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/GitProtocolConstants.java index 760ac6c1d7..1561c93b95 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/GitProtocolConstants.java +++ b/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. * diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/LsRefsV2Request.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/LsRefsV2Request.java index 3aff584a00..add373147c 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/LsRefsV2Request.java +++ b/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 serverOptions; + private LsRefsV2Request(List refPrefixes, boolean symrefs, - boolean peel) { + boolean peel, @Nullable String agent, + @NonNull List 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. + *

+ * 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 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 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)); } } } diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java index 71b5773a42..a03f02146a 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/ProtocolV2Parser.java +++ b/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 serverOptionConsumer, + Consumer 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 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(); } 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 58cd564114..535c914fc3 100644 --- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java +++ b/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; }