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

[Junio C Hamano <gitster@pobox.com>]

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

> On Tue, May 20, 2025 at 11:37 AM Karthik Nayak <karthik.188@gmail.com> wrote:
>>
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>> [snip]
>>
>> >
>> >  /*
>> > - * 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' member 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)
>>
>> Nit: while we're here wouldn't be nicer to rename this to
>> `promiser_info_list_find_name` or similar?
>
> 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.

Is promisor_info primarily be found by their names before its other
members are accessed?  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.

Thanks.