Re: [PATCH v4 1/5] promisor-remote: refactor to get rid of 'struct strvec'
Christian Couder <christian.couder@gmail.com> writes:
Show 12 quoted lines
> In a following commit, we will use the new 'promisor-remote' protocol
> capability introduced by d460267613 (Add 'promisor-remote' capability
> to protocol v2, 2025-02-18) to pass and process more information
> about promisor remotes than just their name and url.
>
> For that purpose, we will need to store information about other
> fields, especially information that might or might not be available
> for different promisor remotes. Unfortunately using 'struct strvec',
> as we currently do, to store information about the promisor remotes
> with one 'struct strvec' for each field like "name" or "url" does not
> scale easily in that case.
>
Nit: It would be nice to mention _why_ it doesn't scale easily here.
Show 16 quoted lines
> Let's refactor this and introduce a new 'struct promisor_info'.
>
> 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
> the instances of 'struct promisor_info'. For each 'item' in the
> string_list, 'item->string' will point to the promisor remote name and
> 'item->util' will point to the corresponding 'struct promisor_info'
> instance.
>
> Explicit members are used within 'struct promisor_info' for type
> safety and clarity regarding the specific information being handled,
> rather than a generic key-value store. We want to specify and document
> each field and its content, so adding new members to the struct as
> more fields are supported is fine.
>
The rest of the patch looks good to me.
[snip]
Karthik Nayak <karthik.188@gmail.com>