Re: [PATCH 2/4] promisor-remote: refactor to get rid of 'struct strvec'

[Christian Couder <christian.couder@gmail.com>]

On Tue, Apr 22, 2025 at 12:13 PM Patrick Steinhardt <ps@pks.im> wrote:
>
> On Mon, Apr 14, 2025 at 06:03:41PM +0200, Christian Couder wrote:

> > +/*
> > + * Linked list for promisor remotes.
> > + *
> > + * 'fields' should not be sorted, as we will rely on the order we put
> > + * things into it. So, for example, 'string_list_append()' should be
> > + * used instead of 'string_list_insert()'.
> > + */
> > +struct promisor_info {
> > +     struct promisor_info *next;
> > +     struct string_list fields;
> > +};
> > +
> > +static void free_info_list(struct promisor_info *p)
>
> Nit: nowadays we would call this something like
> `promisor_info_list_free()`, with the name of the subsystem coming
> first.

Fine, I have changed it to `promisor_info_list_free()` in the next version.

> >  char *promisor_remote_info(struct repository *repo)
> >  {
> >       struct strbuf sb = STRBUF_INIT;
> >       int advertise_promisors = 0;
> > -     struct strvec names = STRVEC_INIT;
> > -     struct strvec urls = STRVEC_INIT;
> > +     struct promisor_info *info_list;
> > +     struct promisor_info *r, *p;
> >
> >       git_config_get_bool("promisor.advertise", &advertise_promisors);
> >
> >       if (!advertise_promisors)
> >               return NULL;
> >
> > -     promisor_info_vecs(repo, &names, &urls);
> > +     info_list = promisor_info_list(repo);
> >
> > -     if (!names.nr)
> > +     if (!info_list)
> >               return NULL;
> >
> > -     for (size_t i = 0; i < names.nr; i++) {
> > -             if (i)
> > +     for (p = NULL, r = info_list; r; p = r, r = r->next) {
> > +             struct string_list_item *item;
> > +             int first = 1;
> > +
> > +             if (r != info_list)
> >                       strbuf_addch(&sb, ';');
> > -             strbuf_addstr(&sb, "name=");
> > -             strbuf_addstr_urlencode(&sb, names.v[i], allow_unsanitized);
> > -             strbuf_addstr(&sb, ",url=");
> > -             strbuf_addstr_urlencode(&sb, urls.v[i], allow_unsanitized);
> > +
> > +             for_each_string_list_item(item, &r->fields) {
> > +                     if (first)
> > +                             first = 0;
> > +                     else
> > +                             strbuf_addch(&sb, ',');
> > +                     strbuf_addf(&sb, "%s=", item->string);
> > +                     strbuf_addstr_urlencode(&sb, (char *)item->util, allow_unsanitized);
> > +             }
> >       }
> >
> > -     strvec_clear(&names);
> > -     strvec_clear(&urls);
> > +     free_info_list(p);
>
> I don't quite follow the usage pattern of `info_list` here. My
> expectation is that we'd:
>
>   1. Acquire the promisor info list.
>
>   2. Iterate through each of its items.
>
>   3. Free the complete list.
>
> But why do we free `p` here? Shouldn't we free `info_list`? And if we
> did so, can't we drop `p` completely and just iterate through the list
> via `r`?

Yeah, it's a bug that is fixed in the next version.

> >       return strbuf_detach(&sb, NULL);
> >  }
> >
> >  /*
> > - * Find first index of 'nicks' where there is 'nick'. 'nick' is
> > - * compared case sensitively to the strings in 'nicks'. If not found
> > - * 'nicks->nr' is returned.
> > + * Find first element of 'p' where the 'name' field is 'nick'. 'nick'
> > + * is compared case sensitively to the strings in 'p'. If not found
> > + * NULL is returned.
> >   */
> > -static size_t remote_nick_find(struct strvec *nicks, const char *nick)
> > +static struct promisor_info *remote_nick_find(struct promisor_info *p, const char *nick)
> >  {
> > -     for (size_t i = 0; i < nicks->nr; i++)
> > -             if (!strcmp(nicks->v[i], nick))
> > -                     return i;
> > -     return nicks->nr;
> > +     for (; p; p = p->next) {
> > +             assert(!strcmp(p->fields.items[0].string, "name"));
>
> Why do we add this assert now? And if we want to keep it, shouldn't it
> rather be `BUG()`?

Yeah, it's now BUG() in the next version.

> > @@ -414,11 +461,16 @@ static int should_accept_remote(enum accept_promisor accept,
> >               return 0;
> >       }
> >
> > -     if (!strcmp(urls->v[i], remote_url))
> > +     if (strcmp(p->fields.items[1].string, "url"))
> > +             BUG("Bad info_list for remote '%s'", remote_name);
>
> It feels somewhat fragile to assume hardcoded locations of each of the
> keys in `fields.items`. Would it be preferable to instead have a
> function that looks up the index by key?

On the other hand we always require a 'name' and a 'url' fields, so
putting them first can simplify and speed things up.

If the checks look too ugly maybe they can be abstracted out in
functions dedicated to get the name or the url?