Re: [PATCH 4/4] fetch-pack: mention server version with verbose output

[Jeff King <peff@peff.net>]

On Mon, Aug 13, 2012 at 05:11:10PM -0400, Jeff King wrote:

> On Mon, Aug 13, 2012 at 02:09:32PM -0700, Junio C Hamano wrote:
> 
> > >> +	if ((agent_feature = server_feature("agent", &agent_len)) != NULL &&
> > >> +	    5 < agent_len && agent_feature[5] == '=') {
> > >>  		agent_supported = 1;
> > >> +		if (args.verbose) {
> > >> +			fprintf(stderr, "Server version is %.*s\n",
> > >> +				agent_len - 6, agent_feature + 6);
> > >> +		}
> > >> +	}
> > >
> > > Yeah, this is exactly the kind of ugliness I was trying to avoid with my
> > > allocating wrapper. Still, there is only one call site, so I do not care
> > > overly much (and I as I've already said, I'm lukewarm on the final two
> > > patches, anyway).
> > 
> > Actually, the above is vastly superiour compared to the allocating
> > kind.  Be honest and think about it.  If the caller wants to
> > allocate, it could, and it does not even have to count.  If the
> > caller does not want to allocate, it does not have to pay the price.
> 
> My point is that the run-time allocation price is quite small, but the
> readability cost of that ugly conditional with the magic "5" is
> non-trivial. But they are apples and oranges, so it is hard to compare
> their amounts directly.

So if we want to avoid the allocation, then this is how I would do it:
by returning the feature's _value_ and not the whole key. Since we know
that the beginning part must obviously match what we fed it anyway, it
is not that interesting.

-- >8 --
Subject: [PATCH] parse_feature_request: make it easier to see feature values

We already take care to parse key/value capabilities like
"foo=bar", but the code does not provide a good way of
actually finding out what is on the right-hand side of the
"=".

A server using "parse_feature_request" could accomplish this
with some extra parsing. You must skip past the "key"
portion manually, check for "=" versus NUL or space, and
then find the length by searching for the next space (or
NUL).  But clients can't even do that, since the
"server_supports" interface does not even return the
pointer.

Instead, let's have our parser share more information by
providing a pointer to the value and its length. The
"parse_feature_value" function returns a pointer to the
feature's value portion, along with the length of the value.
If the feature is missing, NULL is returned. If it does not
have an "=", then a zero-length value is returned.

Similarly, "server_feature_value" behaves in the same way,
but always checks the static server_feature_list variable.

We can then implement "server_supports" in terms of
"server_feature_value". We cannot implement the original
"parse_feature_request" in terms of our new function,
because it returned a pointer to the beginning of the
feature. However, no callers actually cared about the value
of the returned pointer, so we can simplify it to a boolean
just as we do for "server_supports".

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h   |  4 +++-
 connect.c | 45 ++++++++++++++++++++++++++++++++++++---------
 2 files changed, 39 insertions(+), 10 deletions(-)

diff --git a/cache.h b/cache.h
index 67f28b4..95daa69 100644
--- a/cache.h
+++ b/cache.h
@@ -1038,7 +1038,9 @@ struct extra_have_objects {
 };
 extern struct ref **get_remote_heads(int in, struct ref **list, unsigned int flags, struct extra_have_objects *);
 extern int server_supports(const char *feature);
-extern const char *parse_feature_request(const char *features, const char *feature);
+extern int parse_feature_request(const char *features, const char *feature);
+extern const char *server_feature_value(const char *feature, int *len_ret);
+extern const char *parse_feature_value(const char *feature_list, const char *feature, int *len_ret);
 
 extern struct packed_git *parse_pack_index(unsigned char *sha1, const char *idx_path);
 
diff --git a/connect.c b/connect.c
index 55a85ad..49e56ba 100644
--- a/connect.c
+++ b/connect.c
@@ -115,12 +115,7 @@ struct ref **get_remote_heads(int in, struct ref **list,
 	return list;
 }
 
-int server_supports(const char *feature)
-{
-	return !!parse_feature_request(server_capabilities, feature);
-}
-
-const char *parse_feature_request(const char *feature_list, const char *feature)
+const char *parse_feature_value(const char *feature_list, const char *feature, int *lenp)
 {
 	int len;
 
@@ -132,14 +127,46 @@ const char *parse_feature_request(const char *feature_list, const char *feature)
 		const char *found = strstr(feature_list, feature);
 		if (!found)
 			return NULL;
-		if ((feature_list == found || isspace(found[-1])) &&
-		    (!found[len] || isspace(found[len]) || found[len] == '='))
-			return found;
+		if (feature_list == found || isspace(found[-1])) {
+			const char *value = found + len;
+			/* feature with no value (e.g., "thin-pack") */
+			if (!*value || isspace(*value)) {
+				if (lenp)
+					*lenp = 0;
+				return value;
+			}
+			/* feature with a value (e.g., "agent=git/1.2.3") */
+			else if (*value == '=') {
+				value++;
+				if (lenp)
+					*lenp = strcspn(value, " \t\n");
+				return value;
+			}
+			/*
+			 * otherwise we matched a substring of another feature;
+			 * keep looking
+			 */
+		}
 		feature_list = found + 1;
 	}
 	return NULL;
 }
 
+int parse_feature_request(const char *feature_list, const char *feature)
+{
+	return !!parse_feature_value(feature_list, feature, NULL);
+}
+
+const char *server_feature_value(const char *feature, int *len)
+{
+	return parse_feature_value(server_capabilities, feature, len);
+}
+
+int server_supports(const char *feature)
+{
+	return !!server_feature_value(feature, NULL);
+}
+
 enum protocol {
 	PROTO_LOCAL = 1,
 	PROTO_SSH,
-- 
1.7.12.rc2.11.gf0a1e27