Re: [PATCH v4 1/5] promisor-remote: refactor to get rid of 'struct strvec'

From: Christian Couder <christian.couder@gmail.com>
Date: Jun 25, 2025, 12:52 UTC
Message-ID: <CAP8UFD1LxX966Cd__AUg=trOWPc6-2K55-diarEU4Z69zLNtgg@mail.gmail.com>
In-Reply-To: <2z3uanklsfnrgxmiqjt2innv2diownbpwtgcj67bi7udwjdqpg@ackusinuz7c6>

On Mon, Jun 23, 2025 at 9:43 PM Justin Tobler <jltobler@gmail.com> wrote:

>
> On 25/06/11 03:45PM, Christian Couder wrote:

Show 5 quoted lines

> > It will only store promisor remote information in its members. For now
> > it has only a 'name' member for the promisor remote name and an 'url'
> > member for its URL. We will use use a 'struct string_list' to store
>
> s/use use/use/

Fixed in v5.

[...]

Show 12 quoted lines

> > @@ -340,47 +372,36 @@ 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 string_list config_info = STRING_LIST_INIT_NODUP;
>
> nit: Ok in this context, "config_info" is specific to the list of
> promisor_info not just generic git configuration. Something like
> "promisor_info_list" would be a bit more explicit, but I don't feel
> super strongly.

Yeah, it's not generic config, but it's still config, and I think that's important to help understand what the code does when it uses it, so we should keep "config" somehow in this variable name. For now I haven't changed it.

Show 25 quoted lines

> > +     struct string_list_item *item;
> >
> >       git_config_get_bool("promisor.advertise", &advertise_promisors);
> >
> >       if (!advertise_promisors)
> >               return NULL;
> >
> > -     promisor_info_vecs(repo, &names, &urls);
> > +     promisor_config_info_list(repo, &config_info);
> >
> > -     if (!names.nr)
> > +     if (!config_info.nr)
> >               return NULL;
> >
> > -     for (size_t i = 0; i < names.nr; i++) {
> > -             if (i)
> > +     for_each_string_list_item(item, &config_info) {
> > +             struct promisor_info *p = item->util;
> > +
> > +             if (item != config_info.items)
> >                       strbuf_addch(&sb, ';');
>
> Out of curiousity, is it invalid for the trailing promisor remote entry
> to end with a ';'? It would be simpler if each entry could just end with
> a semi-colon.

It would work but it's not really valid.

In "Documentation/gitprotocol-v2.adoc" which specifies the protocol, most of the time when different items are transmitted together they are separated by SP like:

item1 SP item2 SP item3 LF

and there is no SP before the LF.

For the "promisor-remote", we need something more complex but for consistency I think it makes sense to not repeat separators at the end, in the same way as SP are not repeated before LF.

Show 8 quoted lines

> > -     if (!strcmp(urls->v[i], remote_url))
> > +     if (!p->url)
> > +             BUG("bad config_info (invalid URL) for remote '%s'",
> > +                 remote_name);
>
> Ok just to clarify, it is invalid for a promisor remote to not have a
> URL specified. If so, it might be better to say "empty URL" or something
> along those lines.

Yeah, in v5 it's now "URL is NULL" instead of "invalid URL".

Thanks.