From: Christian Couder Date: Mon, 08 Sep 2025 05:31:32 GMT Subject: Re: [PATCH v7 3/5] promisor-remote: refactor how we parse advertised fields Message-ID: In-Reply-To: On Thu, Jul 31, 2025 at 6:04 PM Junio C Hamano wrote: > > Christian Couder 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. Yeah, this is bad. This is fixed in the v8 by using string_list_split() instead of string_list_split_in_place(). See below. > 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. Yeah, right, this solution is used in v8. Thanks. > > @@ -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. Yeah, right, all these suggestions (splitting into a string list, removing the strbuf_strip_suffix() call and passing a const char * to parse_one_advertised_remote()) are implemented in v8. Thanks. While at it, in v8, all the calls to strbuf_split*() functions left in "promisor-remote.c" were replaced with calls to string_list_split*() functions.