Re: [PATCH v7 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 string_list elem_list = STRING_LIST_INIT_NODUP;
> +	struct string_list_item *item;
> +
> +	string_list_split_in_place(&elem_list, remote_info->buf, ",", -1);

This munges a strbuf pointed by a structure that was supplied by the
caller as a parameter to this function.  Most notably, all commas in
remote_info->buf are replaced with NULs.

A quick read of the sole caller of this function reveals that the
caller passes one element of an array of strbuf it obtained by
calling strbuf_split_str() for the only purpose of calling this
function, and the caller never looks at the .buf member itself, so I
think this munging with _in_place() would not hurt the caller.

> +	for_each_string_list_item(item, &elem_list) {
> +		char *elem = item->string;
> +		char *value;
> +		char *p = strchr(elem, '=');
> +
> +		if (!p) {
> +			warning(_("invalid element '%s' from remote info"), elem);
> +			continue;
> +		}

We find the first '=' and ...

> +		*p = '\0';

... replace it with NUL; the item->string here is now split into elem & value.

> +		value = url_percent_decode(p + 1);

And the value gets decoded.

> +		if (!strcmp(elem, "name"))
> +			info->name = value;
> +		else if (!strcmp(elem, "url"))
> +			info->url = value;
> +		else
> +			free(value);
> +	}
> +
> +	string_list_clear(&elem_list, 0);

And we are done with the string list that holds the pieces of remote
info split out.

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

But this use of remote_info->buf will no longer give us much useful
information.  It might say something like "name", and the warning
would not let us see what the rest of the remote_info->buf used to
have before _in_place splitting.

I think initializing elem_list with INIT_DUP and using string_list_split()
without _in_place should be sufficient to fix this.

> @@ -604,32 +651,19 @@ static void filter_promisor_remote(struct repository *repo,
>  	remotes = strbuf_split_str(info, ';', 0);

If we splitted this into a string list, then each item in it
will not have the terminating ';' at its end, therefore ...

>  	for (size_t i = 0; remotes[i]; i++) {
> -		struct strbuf **elems;
> -		const char *remote_name = NULL;
> -		const char *remote_url = NULL;
> -		char *decoded_name = NULL;
> -		char *decoded_url = NULL;
> +		struct promisor_info *advertised;
>  
>  		strbuf_strip_suffix(remotes[i], ";");

... this strip_suffix() will become unnecessary.

> +		advertised = parse_one_advertised_remote(remotes[i]);

Such a change would require this call to be made with const char *
not struct strbuf * as a parameter, but the callee we just saw above
uses its parameter (i.e. remote_info) only to look at the .buf member,
and does not take any advantage of it being a strbuf (like it can be
trimmed or its length is known without running strlen(.buf) on it),
so that should be an easy conversion.

> +		if (!advertised)
> +			continue;
>  
> +		if (should_accept_remote(accept, advertised, &config_info))
> +			strvec_push(accepted, advertised->name);
>  
> -		strbuf_list_free(elems);
> -		free(decoded_name);
> -		free(decoded_url);
> +		promisor_info_free(advertised);
>  	}
>  
>  	promisor_info_list_clear(&config_info);

Overall looking very promising.  Thanks for working on the topic.