On 25/06/11 03:45PM, Christian Couder wrote:
> 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.
>
> 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
s/use use/use/
> 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.
>
> Signed-off-by: Christian Couder <chriscool@xxxxxxxxxxxxx>
> ---
> promisor-remote.c | 111 +++++++++++++++++++++++++++++-----------------
> 1 file changed, 70 insertions(+), 41 deletions(-)
>
> diff --git a/promisor-remote.c b/promisor-remote.c
> index 9d058586df..90a063ea53 100644
> --- a/promisor-remote.c
> +++ b/promisor-remote.c
> @@ -314,9 +314,35 @@ static int allow_unsanitized(char ch)
> return ch > 32 && ch < 127;
> }
>
> -static void promisor_info_vecs(struct repository *repo,
> - struct strvec *names,
> - struct strvec *urls)
> +/*
> + * Struct for promisor remotes involved in the "promisor-remote"
> + * protocol capability.
> + *
> + * Except for "name", each <member> in this struct and its <value>
> + * should correspond (either on the client side or on the server side)
> + * to a "remote.<name>.<member>" config variable set to <value> where
> + * "<name>" is a promisor remote name.
> + */
> +struct promisor_info {
> + const char *name;
> + const char *url;
> +};
Ok so now all promisor info for a given remote is stored in its own
struct. This will enable easier extension in the future to add
additional fields.
> +
> +static void promisor_info_list_clear(struct string_list *list)
> +{
> + for (size_t i = 0; i < list->nr; i++) {
> + struct promisor_info *p = list->items[i].util;
> + free((char *)p->name);
> + free((char *)p->url);
> + }
> + string_list_clear(list, 1);
> +}
> +
> +/*
> + * Populate 'list' with promisor remote information from the config.
> + * The 'util' pointer of each list item will hold a 'struct promisor_info'.
> + */
> +static void promisor_config_info_list(struct repository *repo, struct string_list *list)
> {
> struct promisor_remote *r;
>
> @@ -328,8 +354,14 @@ static void promisor_info_vecs(struct repository *repo,
>
> /* Only add remotes with a non empty URL */
> if (!git_config_get_string_tmp(url_key, &url) && *url) {
> - strvec_push(names, r->name);
> - strvec_push(urls, url);
> + struct promisor_info *new_info = xcalloc(1, sizeof(*new_info));
> + struct string_list_item *item;
> +
> + new_info->name = xstrdup(r->name);
> + new_info->url = xstrdup(url);
> +
> + item = string_list_append(list, new_info->name);
> + item->util = new_info;
> }
In this version, each remote is now stored as a member of a `struct
string_list` instead of a custom collection type specific to promisor
remote info. Nice
> free(url_key);
> @@ -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.
> + 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.
> +
> strbuf_addstr(&sb, "name=");
> - strbuf_addstr_urlencode(&sb, names.v[i], allow_unsanitized);
> + strbuf_addstr_urlencode(&sb, p->name, allow_unsanitized);
> strbuf_addstr(&sb, ",url=");
> - strbuf_addstr_urlencode(&sb, urls.v[i], allow_unsanitized);
> + strbuf_addstr_urlencode(&sb, p->url, allow_unsanitized);
> }
>
> - strvec_clear(&names);
> - strvec_clear(&urls);
> + promisor_info_list_clear(&config_info);
>
> return strbuf_detach(&sb, NULL);
> }
>
> -/*
> - * 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.
> - */
> -static size_t remote_nick_find(struct strvec *nicks, const char *nick)
> -{
> - for (size_t i = 0; i < nicks->nr; i++)
> - if (!strcmp(nicks->v[i], nick))
> - return i;
> - return nicks->nr;
> -}
> -
> enum accept_promisor {
> ACCEPT_NONE = 0,
> ACCEPT_KNOWN_URL,
> @@ -390,19 +411,23 @@ enum accept_promisor {
>
> static int should_accept_remote(enum accept_promisor accept,
> const char *remote_name, const char *remote_url,
> - struct strvec *names, struct strvec *urls)
> + struct string_list *config_info)
> {
> - size_t i;
> + struct promisor_info *p;
> + struct string_list_item *item;
>
> if (accept == ACCEPT_ALL)
> return 1;
>
> - i = remote_nick_find(names, remote_name);
> + /* Get config info for that promisor remote */
> + item = string_list_lookup(config_info, remote_name);
>
> - if (i >= names->nr)
> + if (!item)
> /* We don't know about that remote */
> return 0;
>
> + p = item->util;
> +
> if (accept == ACCEPT_KNOWN_NAME)
> return 1;
>
> @@ -414,11 +439,15 @@ static int should_accept_remote(enum accept_promisor accept,
> return 0;
> }
>
> - 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.
> +
> + if (!strcmp(p->url, remote_url))
> return 1;
>
> warning(_("known remote named '%s' but with URL '%s' instead of '%s'"),
> - remote_name, urls->v[i], remote_url);
> + remote_name, p->url, remote_url);
>
> return 0;
> }
> @@ -430,8 +459,7 @@ static void filter_promisor_remote(struct repository *repo,
> struct strbuf **remotes;
> const char *accept_str;
> enum accept_promisor accept = ACCEPT_NONE;
> - struct strvec names = STRVEC_INIT;
> - struct strvec urls = STRVEC_INIT;
> + struct string_list config_info = STRING_LIST_INIT_NODUP;
>
> if (!git_config_get_string_tmp("promisor.acceptfromserver", &accept_str)) {
> if (!*accept_str || !strcasecmp("None", accept_str))
> @@ -450,8 +478,10 @@ static void filter_promisor_remote(struct repository *repo,
> if (accept == ACCEPT_NONE)
> return;
>
> - if (accept != ACCEPT_ALL)
> - promisor_info_vecs(repo, &names, &urls);
> + if (accept != ACCEPT_ALL) {
> + promisor_config_info_list(repo, &config_info);
> + string_list_sort(&config_info);
> + }
>
> /* Parse remote info received */
>
> @@ -482,7 +512,7 @@ static void filter_promisor_remote(struct repository *repo,
> if (remote_url)
> decoded_url = url_percent_decode(remote_url);
>
> - if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, &names, &urls))
> + if (decoded_name && should_accept_remote(accept, decoded_name, decoded_url, &config_info))
> strvec_push(accepted, decoded_name);
>
> strbuf_list_free(elems);
> @@ -490,8 +520,7 @@ static void filter_promisor_remote(struct repository *repo,
> free(decoded_url);
> }
>
> - strvec_clear(&names);
> - strvec_clear(&urls);
> + promisor_info_list_clear(&config_info);
> strbuf_list_free(remotes);
> }
>
> --
> 2.50.0.rc2.5.ge8efe62b7f
>
