Re: [PATCH v7 3/5] promisor-remote: refactor how we parse advertised fields

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