Re: [PATCH 3/4] builtin/clone.c: recognize fetch.serverOption configuration

From: Patrick Steinhardt <ps@pks.im>
Date: Sep 3, 2024, 10:09 UTC
Message-ID: <ZtbgafDQwbU9tBoq@pks.im>
In-Reply-To: <7c3ebda513d872a2ab2aa0cff5887757de4cde0a.1725279236.git.gitgitgadget@gmail.com>

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

Show 14 quoted lines

> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index 8e925db7e9c..105645ed685 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -149,6 +149,9 @@ objects from the source repository into a pack in the cloned repository.
>  	unknown ones, is server-specific.
>  	When multiple ++--server-option=++__<option>__ are given, they are all
>  	sent to the other side in the order listed on the command line.
> +	When no ++--server-option=++__<option>__ is given from the command
> +	line, the values of configuration variable `fetch.serverOption`
> +	are used instead.
>  
>  `-n`::
>  `--no-checkout`::

I'm not a 100% sure, but I don't think that `fetch.*` configs typically impact git-clone(1). So this here is a tad surprising to me.

It makes me wonder whether it is actually sensible to implement this as part of the `fetch` namespace in the first place. I'm not yet quite sure where this whole series slots in, that is why one would want to set up default server options in the first place. So what I'm wondering right now is whether the server options are something that you want to apply globally for all remotes, or whether you'd rather want to set them up per remote.

In the latter case I could see that it may make sense to instead make this `remote.<name>.serverOption`. This would also remove the unclean design that a fetch-related config now impacts clones, even though it now works differently than our push options.

I guess this depends on the actual usecase.

Show 17 quoted lines

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 269b6e18a4e..5a1e2e769af 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -847,6 +848,12 @@ static int git_clone_config(const char *k, const char *v,
>  		config_reject_shallow = git_config_bool(k, v);
>  	if (!strcmp(k, "clone.filtersubmodules"))
>  		config_filter_submodules = git_config_bool(k, v);
> +	if (!strcmp(k, "fetch.serveroption")) {
> +		if (!v)
> +			return config_error_nonbool(k);
> +		parse_transport_option(v, &config_server_options);
> +		return 0;
> +	}
>  
>  	return git_default_config(k, v, ctx, cb);
>  }

Seeing that we always have the `config_error_nonbool()` call, would it make sense to also move that into `parse_transport_option()` and have it return an error code for us?

Patrick