UploadPack: Return correct peer user agent on v2 requests

UploadPack.getPeerUserAgent() doesn't produce the expected results for
protocol v2 requests. In v2, the agent reported in the request (in an
"agent=" line) is not in the clientCapabilities but in a field on its
own. This makes getPeerUserAgent default to the transport user agent.

Making "agent" a shared property between protocol v0/v1 and v2 fixes the
problem, simplifies the function and harmonizes the implementation
between protocol versions.

In a follow up commit the "agent" will be identified on parsing time,
instead of taking it from the client capabilities.

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

4 files changed, 51 insertions, 24 deletions

diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchRequest.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchRequest.java
index 5d28a4d9f2..40ba3a3ad2 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchRequest.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchRequest.java
@@ -48,6 +48,7 @@ import java.util.List;
 import java.util.Set;
 
 import org.eclipse.jgit.annotations.NonNull;
+import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.lib.ObjectId;
 
 /**
@@ -69,6 +70,9 @@ abstract class FetchRequest {
 
 	final List<String> deepenNotRefs;
 
+	@Nullable
+	final String agent;
+
 	/**
 	 * Initialize the common fields of a fetch request.
 	 *
@@ -88,11 +92,13 @@ abstract class FetchRequest {
 	 * @param deepenSince
 	 *            Requests that the shallow clone/fetch should be cut at a
 	 *            specific time, instead of depth
+	 * @param agent
+	 *            agent as reported by the client in the request body
 	 */
 	FetchRequest(@NonNull Set<ObjectId> wantIds, int depth,
 			@NonNull Set<ObjectId> clientShallowCommits, long filterBlobLimit,
 			@NonNull Set<String> clientCapabilities, int deepenSince,
-			@NonNull List<String> deepenNotRefs) {
+			@NonNull List<String> deepenNotRefs, @Nullable String agent) {
 		this.wantIds = requireNonNull(wantIds);
 		this.depth = depth;
 		this.clientShallowCommits = requireNonNull(clientShallowCommits);
@@ -100,6 +106,7 @@ abstract class FetchRequest {
 		this.clientCapabilities = requireNonNull(clientCapabilities);
 		this.deepenSince = deepenSince;
 		this.deepenNotRefs = requireNonNull(deepenNotRefs);
+		this.agent = agent;
 	}
 
 	/**
@@ -146,7 +153,11 @@ abstract class FetchRequest {
 	 * These options are listed and well-defined in the git protocol
 	 * specification.
 	 *
-	 * @return capabilities sent by the client
+	 * The agent capability is not included in this set. It can be retrieved via
+	 * {@link #getAgent()}.
+	 *
+	 * @return capabilities sent by the client (excluding the "agent"
+	 *         capability)
 	 */
 	@NonNull
 	Set<String> getClientCapabilities() {
@@ -171,4 +182,13 @@ abstract class FetchRequest {
 	List<String> getDeepenNotRefs() {
 		return deepenNotRefs;
 	}
+
+	/**
+	 * @return string identifying the agent (as sent in the request body by the
+	 *         client)
+	 */
+	@Nullable
+	String getAgent() {
+		return agent;
+	}
 }
\ No newline at end of file
diff --git a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV0Request.java b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV0Request.java
index bb358c886f..5a97036eb7 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV0Request.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV0Request.java
@@ -48,6 +48,7 @@ import java.util.HashSet;
 import java.util.Set;
 
 import org.eclipse.jgit.annotations.NonNull;
+import org.eclipse.jgit.annotations.Nullable;
 import org.eclipse.jgit.lib.ObjectId;
 
 /**
@@ -57,9 +58,9 @@ final class FetchV0Request extends FetchRequest {
 
 	FetchV0Request(@NonNull Set<ObjectId> wantIds, int depth,
 			@NonNull Set<ObjectId> clientShallowCommits, long filterBlobLimit,
-			@NonNull Set<String> clientCapabilities) {
+			@NonNull Set<String> clientCapabilities, @Nullable String agent) {
 		super(wantIds, depth, clientShallowCommits, filterBlobLimit,
-				clientCapabilities, 0, Collections.emptyList());
+				clientCapabilities, 0, Collections.emptyList(), agent);
 	}
 
 	static final class Builder {
@@ -74,6 +75,8 @@ final class FetchV0Request extends FetchRequest {
 
 		final Set<String> clientCaps = new HashSet<>();
 
+		String agent;
+
 		/**
 		 * @param objectId
 		 *            object id received in a "want" line
@@ -111,7 +114,24 @@ final class FetchV0Request extends FetchRequest {
 		 * @return this builder
 		 */
 		Builder addClientCapabilities(Collection<String> clientCapabilities) {
-			clientCaps.addAll(clientCapabilities);
+			for (String cap: clientCapabilities) {
+				// TODO(ifrade): Do this is done on parse time
+				if (cap.startsWith("agent=")) { //$NON-NLS-1$
+					agent = cap.substring("agent=".length()); //$NON-NLS-1$
+				} else {
+					clientCaps.add(cap);
+				}
+			}
+			return this;
+		}
+
+		/**
+		 * @param clientAgent
+		 *            agent line sent by the client in the request body
+		 * @return this builder
+		 */
+		Builder setAgent(String clientAgent) {
+			agent = clientAgent;
 			return this;
 		}
 
@@ -127,7 +147,8 @@ final class FetchV0Request extends FetchRequest {
 
 		FetchV0Request build() {
 			return new FetchV0Request(wantIds, depth, clientShallowCommits,
-					filterBlobLimit, clientCaps);
+					filterBlobLimit, clientCaps, agent);
 		}
+
 	}
 }
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 951d2d79ea..8e36a109e9 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/FetchV2Request.java
@@ -71,9 +71,6 @@ public final class FetchV2Request extends FetchRequest {
 
 	private final boolean doneReceived;
 
-	@Nullable
-	private final String agent;
-
 	@NonNull
 	private final List<String> serverOptions;
 
@@ -86,11 +83,10 @@ public final class FetchV2Request extends FetchRequest {
 			boolean doneReceived, @NonNull Set<String> clientCapabilities,
 			@Nullable String agent, @NonNull List<String> serverOptions) {
 		super(wantIds, depth, clientShallowCommits, filterBlobLimit,
-				clientCapabilities, deepenSince, deepenNotRefs);
+				clientCapabilities, deepenSince, deepenNotRefs, agent);
 		this.peerHas = requireNonNull(peerHas);
 		this.wantedRefs = requireNonNull(wantedRefs);
 		this.doneReceived = doneReceived;
-		this.agent = agent;
 		this.serverOptions = requireNonNull(serverOptions);
 	}
 
@@ -118,15 +114,6 @@ public final class FetchV2Request extends FetchRequest {
 	}
 
 	/**
-	 * @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
 	 *
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 535c914fc3..fde9ce9179 100644
--- a/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
+++ b/org.eclipse.jgit/src/org/eclipse/jgit/transport/UploadPack.java
@@ -1369,12 +1369,11 @@ public class UploadPack {
 	 * @since 4.0
 	 */
 	public String getPeerUserAgent() {
-		if (currentRequest == null) {
-			return userAgent;
+		if (currentRequest != null && currentRequest.getAgent() != null) {
+			return currentRequest.getAgent();
 		}
 
-		return UserAgent.getAgent(currentRequest.getClientCapabilities(),
-				userAgent);
+		return userAgent;
 	}
 
 	private boolean negotiate(FetchRequest req,