From: Patrick Steinhardt <ps@pks.im>
Date: Tue, 03 Sep 2024 10:09:45 GMT
Subject: Re: [PATCH 3/4] builtin/clone.c: recognize fetch.serverOption configuration
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:
> 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.

> 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