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

> 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

> 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@tuxfamily.org>
> ---
>  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;
> +};

> +
> +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;
>  		}

>  		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;

> +	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, ';');

> +
>  		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);

> +
> +	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
>