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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 10, 2012, 20:01 UTC
Message-ID: <7v7gt6jz3s.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20120810075816.GC8399@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 26 quoted lines

> +/*
> + * 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.

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.

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.