Re: [PATCH v2 2/5] remote: introduce remote.<name>.serverOption configuration

From: Patrick Steinhardt <ps@pks.im>
Date: Oct 7, 2024, 08:22 UTC
Message-ID: <ZwOaR5OqdLaS4cu6@pks.im>
In-Reply-To: <2474b4c69d68576e901cdd6381aac9410cf21922.1727093878.git.gitgitgadget@gmail.com>

On Mon, Sep 23, 2024 at 12:17:55PM +0000, Xing Xin via GitGitGadget wrote:

Show 12 quoted lines

> diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt
> index 8efc53e836d..8ba48573fb6 100644
> --- a/Documentation/config/remote.txt
> +++ b/Documentation/config/remote.txt
> @@ -95,3 +95,13 @@ remote.<name>.partialclonefilter::
>  	Changing or clearing this value will only affect fetches for new commits.
>  	To fetch associated objects for commits already present in the local object
>  	database, use the `--refetch` option of linkgit:git-fetch[1].
> +
> +remote.<name>.serverOption::
> +	When no `--server-option=<option>` argument is given from the command
> +	line, git will use the values from this configuration as a default list of

This description feels a bit inelegant to me. How about the following:

    The default set of server options used when fetching from the
    remote. These server options are overridden by the
    `--server-option=` command line arguments.

Show 13 quoted lines

> @@ -125,6 +126,7 @@ static struct remote *make_remote(struct remote_state *remote_state,
>  	struct remote *ret;
>  	struct remotes_hash_key lookup;
>  	struct hashmap_entry lookup_entry, *e;
> +	struct string_list server_options = STRING_LIST_INIT_DUP;
>  
>  	if (!len)
>  		len = strlen(name);
> @@ -143,6 +145,7 @@ static struct remote *make_remote(struct remote_state *remote_state,
>  	ret->name = xstrndup(name, len);
>  	refspec_init(&ret->push, REFSPEC_PUSH);
>  	refspec_init(&ret->fetch, REFSPEC_FETCH);
> +	ret->server_options = server_options;

I was wondering at first where we actually populate the `server_options`, until I realized that this is only done to initialize the `ret->server_options`. I think it would be less confusing if this used `string_list_init_dup()` instead of the local variable.

Patrick