Re: [PATCH 3/4] connect: learn to parse capabilities with values
- From
Jeff King <peff@peff.net>
- Date
- Aug 10, 2012, 21:15 UTC
- Message-ID
- <20120810211509.GB888@sigill.intra.peff.net>
- In-Reply-To
- <7v7gt6jz3s.fsf@alter.siamese.dyndns.org>
On Fri, Aug 10, 2012 at 01:01:11PM -0700, Junio C Hamano wrote:
Show 33 quoted lines
> Jeff King <peff@peff.net> writes:
>
> > +/*
> > + * 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.
Show 11 quoted lines
> 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...
Show 16 quoted lines
> 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