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

From: Christian Couder <christian.couder@gmail.com>
Date: May 21, 2025, 06:33 UTC
Message-ID: <CAP8UFD25fF6OdYmP54JFyY4Znpij=MR1ifdWk+cjca5Hes739A@mail.gmail.com>
In-Reply-To: <xmqqr00ji63l.fsf@gitster.g>

On Tue, May 20, 2025 at 6:45 PM Junio C Hamano <gitster@pobox.com> wrote:

>
> Christian Couder <christian.couder@gmail.com> writes:

Show 17 quoted lines

> > Junio suggested this name in a discussion of a previous patch series:
> >
> > https://lore.kernel.org/git/xmqqa5bbq0nb.fsf@gitster.g/
>
> Don't blame me for that name ;-)
>
> The name was for a hypothetical variant that took "struct strvec *"
> as its first parameter, and the name was given only because it did
> not make much sense to call the helper after "strvec".  A function
> name that signals that we are finding (something) using "nick"-name
> was a much better choice.  Since your final one finds in "struct
> promisor_info *", not a generic "struct strvec *", I wouldn't be
> surprised if a name that is about promisor-info (whatever it is)
> more clearly describes what it does.  If this is a file-local
> helper, promisor_info_find() or find_promisor_info() may be
> sufficient, if "by nickname" is the primary and only way for the
> application to find an instance of promisor_info.

OK, I think I will use promisor_info_find() or promisor_info_find_by_name() in the next reroll, as it goes well with how other functions related to "struct promisor_info *" are named.

> Is promisor_info primarily be found by their names before its other
> members are accessed?

No, there are different ways the promisor_info instances are iterated on or searched, see promisor_remote_info() and all_fields_match() where we iterate on all the instances and then either use the instance members to generate an advertisement string, or check if some of the members match advertised values.

> If so, I wonder if a strmap or string_list
> that uses the nickname as the key with a pointer to a promisor_info
> structure as the data more appropriate than a hand-crafted linear
> linked list.

I don't think it would bring a lot of benefits. Using an strmap or a sorted string list might make things faster if there are a lot of promisor remotes configured on the clients, but I don't think we are at a point where such an optimisation is worth it.