Re: [PATCH v7 3/5] promisor-remote: refactor how we parse advertised fields
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 31, 2025, 16:03 UTC
- Message-ID
- <xmqqh5yss5xx.fsf@gitster.g>
- In-Reply-To
- <20250731072401.3817074-4-christian.couder@gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
Show 7 quoted lines
> +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.
Show 9 quoted lines
> + 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.
Show 9 quoted lines
> + 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 ...
Show 9 quoted lines
> 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.
Show 13 quoted lines
> + 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.