Christian Couder <christian.couder@xxxxxxxxx> 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.
