Re: [PATCH 3/7] bundle-uri: create "key=value" line parsing

[Junio C Hamano <gitster@pobox.com>]

"Ævar Arnfjörð Bjarmason via GitGitGadget"  <gitgitgadget@gmail.com>
writes:

> +/**
> + * General API for {transport,connect}.c etc.
> + */
> +int bundle_uri_parse_line(struct bundle_list *list, const char *line)
> +{
> +	int result;
> +	const char *equals;
> +	struct strbuf key = STRBUF_INIT;
> +
> +	if (!strlen(line))
> +		return error(_("bundle-uri: got an empty line"));
> +
> +	equals = strchr(line, '=');
> +
> +	if (!equals)
> +		return error(_("bundle-uri: line is not of the form 'key=value'"));
> +	if (line == equals || !*(equals + 1))
> +		return error(_("bundle-uri: line has empty key or value"));

The suggestions implied by my asking fall strictly into the "it does
not have to exist here at this step and we can later extend it", but
for something whose equivalent can be stored in our configuration
file, it is curious why we _insist_ to refuse an empty string as the
value.

I do not miss the "key alone without even '=' means 'true'"
convention, personally, so insisting to have '=' is OK, but the
inability to have an empty string as a value looks a bit disturbing.

This depends on how the helper gets called, but most likely the
caller has a single line of pkt-line that it GAVE us to process, so
it sounds a bit wasteful to insist that "line" to be const to us and
force us to use a separate strbuf, instead of just stuffing NUL at
where we found '=' and pass the two halves to bundle_list_update().

Not a huge deal, it is just something I found funny in the "back in
the days we coded together, Linus would never have written like
this" way.

Other than that small detail, the code looks OK to me.

> +	strbuf_add(&key, line, equals - line);
> +	result = bundle_list_update(key.buf, equals + 1, list);
> +	strbuf_release(&key);
> +
> +	return result;
> +}
> diff --git a/bundle-uri.h b/bundle-uri.h
> index 6692aa4b170..f725c9796f7 100644
> --- a/bundle-uri.h
> +++ b/bundle-uri.h
> @@ -76,4 +76,16 @@ int for_all_bundles_in_list(struct bundle_list *list,
>   */
>  int fetch_bundle_uri(struct repository *r, const char *uri);
>  
> -#endif
> +/**
> + * General API for {transport,connect}.c etc.
> + */
> +
> +/**
> + * Parse a "key=value" packet line from the bundle-uri verb.
> + *
> + * Returns 0 on success and non-zero on error.
> + */
> +int bundle_uri_parse_line(struct bundle_list *list,
> +			  const char *line);
> +
> +#endif /* BUNDLE_URI_H */