Re: [PATCH 3/4] connect: learn to parse capabilities with values

On Fri, Aug 10, 2012 at 01:01:11PM -0700, Junio C Hamano wrote:

Jeff King [off-list ref] writes:

quoted

+/*
+ * Parse features of the form "feature=value".  Returns NULL if the feature
+ * does not exist, the empty string if it exists but does not have an "=", or
+ * the content to the right of the "=" until the first space (or end of
+ * string).  The cannot contain literal spaces; double-quoting or similar
+ * schemes would break compatibility, since older versions of git treat the
+ * space as a hard-delimiter without any context.
+ *
+ * The return value (if non-NULL) is newly allocated on the heap and belongs to
+ * the caller.
+ */
+char *parse_feature_request_value(const char *feature_list, const char *feature)
+{
+	const char *start = parse_feature_request(feature_list, feature);
+	const char *end;
+
+	if (!start || prefixcmp(start, feature))
+		return NULL;
+	start += strlen(feature);
+
+	if (*start == '=')
+		start++;
+	end = strchrnul(start, ' ');
+
+	return xmemdupz(start, end - start);
+}

Having to run strlen(feature) three times in this function (once in
parse_feature_request() as part of strstr() and the edge check of
the found string, once as part of prefixcmp() here, and then an
explicit strlen() to skip it) makes me feel dirty.

I thought about that, but it seems like a quite premature optimization.
It is three extra strlens per network conversation. _If_ you have turned
on double-verbosity in fetch-pack. I considered reusing the existing
parse_feature_request function more valuable from a maintenance
standpoint.

I would think the extra memory allocation would dwarf it, anyway.

It is not wrong per-se, but it is likely that the caller has a
constant string as the feature when it called this function, so
perhaps just changing the function signature of server_supports,
i.e.

    const char *server_supports(const char *feature)
    {
	return parse_feature_request(server_capabilities, feature);
    }

to return "var=val " would be more than sufficient.

That was in fact my first iteration, but...

Then the existing callers can keep doing

	if (server_supports("thin-pack"))
        if (!server_supports("quiet"))

and a new caller can do something like

	agent = server_supports("agent");
        if (!agent || !agent[5])
        	... no agent ...
	else {
        	int span = strcspn(agent + 6, " \t\n");
                printf("I found agent=<%.*s>!\n", span, agent + 6);
	}

which doesn't look too bad.

I didn't want to force callers to have to deal with ad-hoc parsing.

Anyway, do you think this is even worth doing at this point? I'm
lukewarm on the final two patches due to the existence of
GIT_TRACE_PACKET, which is much more likely to be useful.

-Peff