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

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

> >  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`?

> >       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()`?

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