On Thu, Jul 31, 2025 at 6:04 PM Junio C Hamano [off-list ref] wrote:

Christian Couder [off-list ref] writes:

quoted

+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.

quoted

+     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 ...

quoted

+             *p = '\0';

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

quoted

+             value = url_percent_decode(p + 1);

And the value gets decoded.

quoted

+             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.

quoted

+     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.

quoted

@@ -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 ...

quoted

      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.

quoted

+             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.