On Mon, Jun 23, 2025 at 9:43 PM Justin Tobler <jltobler@xxxxxxxxx> wrote:
>
> On 25/06/11 03:45PM, Christian Couder wrote:
> > 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.
[...]
> > @@ -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.
> > + 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.
> > - 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.
