On Fri, Aug 10, 2012 at 01:01:11PM -0700, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html