Re: [PATCH v6 3/5] promisor-remote: refactor how we parse advertised fields

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

Christian Couder <christian.couder@gmail.com> writes:

> +static struct promisor_info *parse_one_advertised_remote(struct strbuf *remote_info)
> +{
> +	struct promisor_info *info = xcalloc(1, sizeof(*info));
> +	struct strbuf **elems = strbuf_split(remote_info, ',');

Unless the primary use of an array is about passing it around as a
whole "set", name such a variable singular, so that element[4] can
be naturally read as "fourth element"---"fourth elements" is not as
natural.

Also, can't we do this without strbuf_split(), which is a wrong API
to use in general [*]?  strbuf is a very good data structure to work
with when editing string data, but an array of strbuf is not---you
would not be editing many pieces of string data in parallel.

    [*] often string_list_split_in_place() is a better alternative,
    especially when you do not have to heavily edit the substrings.

> +	for (size_t i = 0; elems[i]; i++) {
> +		char *elem = elems[i]->buf;
> +		char *value;
> +		char *p = strchr(elem, '=');

	The pointer elem points at the name, and the pointer p
	points at the beginning of value, which could contain '='.

> +		strbuf_strip_suffix(elems[i], ",");

This does not even count as "editing"; split_in_place() would have
removed the trailing comma (and replaced it with NUL to terminate
the string).

> +		if (!p) {
> +			warning(_("invalid element '%s' from remote info"), elem);
> +			continue;
> +		}

	elem pointed at "foo" or "foo,"; we may have stripped the
	trailing comma, but we didn't see the equal sign to start
	the value at all.  Bad input.

> +		*p = '\0';

	Terminate the name by replacing '=' with NUL.

> +		value = url_percent_decode(p + 1);

	Can this helper function fail and signal that it saw a
	malformed data?  If not already, shouldn't it be taught to
	do so?

We are inventing the syntax for this data in this series, so if this
helper takes garbage data silently, and if we are not willing to fix
it, then we can even consider changing the syntax to something with
a helper we can use that already has a good error checking.

> +		if (!strcmp(elem, "name"))
> +			info->name = value;
> +		else if (!strcmp(elem, "url"))
> +			info->url = value;
> +		else
> +			free(value);

As url_percent_decode() always allocate a new copy of string even
when there is nothing to decode, value will always be an allocated
string, and if we are not storing it away, it will leak.  The copies
we kept in info->{name,url} are ours to own.  Makes sense.

> +	strbuf_list_free(elems);

And because [elem..p] (name) we only peeked, we can safely release
the whole thing.  If you used string_list_split_in_place(), you
would only free the string_list shell without having to free the
underlying string.

> +	if (!info->name || !info->url) {
> +		warning(_("server advertised a promisor remote without a name or URL: %s"),
> +			remote_info->buf);
> +		promisor_info_free(info);
> +		return NULL;

Nicely done.

> +	}
> +
> +	return info;
> +}