[PATCH 0/4] Support server option from configuration

[PATCH 0/4] Support server option from configuration

[blanet via GitGitGadget]

Currently, server options for Git protocol v2 can only be specified via the
command line option "--server-option ", which is inconvenient for users who
want to specify a list of default options.

This patch series introduces a new multi-valued configuration,
fetch.serverOption, to specify default server options. Designed similarly to
push.pushOption:

 1. Server options set in lower-priority configuration files (e.g.,
    /etc/gitconfig or $HOME/.gitconfig) can be overridden or unset in more
    specific repository configurations using an empty string.
 2. Command-line specified server options take precedence over those from
    the configuration.

All commands involving server options, including git-fetch, git-clone,
git-ls-remote, and git-pull, have been updated to recognize the new
configuration.

Xing Xin (4):
  transport: add parse_transport_option() method
  builtin/fetch.c: add fetch.serverOption configuration
  builtin/clone.c: recognize fetch.serverOption configuration
  builtin/ls-remote.c: recognize fetch.serverOption configuration

 Documentation/config/fetch.txt  | 29 ++++++++++
 Documentation/fetch-options.txt |  3 ++
 Documentation/git-clone.txt     |  3 ++
 Documentation/git-ls-remote.txt |  3 ++
 builtin/clone.c                 | 22 +++++---
 builtin/fetch.c                 | 31 ++++++++---
 builtin/ls-remote.c             | 32 ++++++++++--
 builtin/push.c                  |  6 +--
 t/t5702-protocol-v2.sh          | 93 ++++++++++++++++++++++++++++-----
 transport.c                     |  8 +++
 transport.h                     |  3 ++
 11 files changed, 196 insertions(+), 37 deletions(-)


base-commit: 4590f2e9412378c61eac95966709c78766d326ba
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1776%2Fblanet%2Fxx%2Fadd-server-option-from-config-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1776/blanet/xx/add-server-option-from-config-v1
Pull-Request: https://github.com/git/git/pull/1776
-- 
gitgitgadget

[PATCH 1/4] transport: add parse_transport_option() method

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Introduce the `parse_transport_option()` method used to parse
`push.pushOption` configuration values. This method will be further
utilized in subsequent commits to parse a newly added
`fetch.serverOption` configuration for fetches, which aligns with the
design pattern of `push.pushOption`.

Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 builtin/push.c | 6 +-----
 transport.c    | 8 ++++++++
 transport.h    | 3 +++
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 7a67398124f..63d19acfb27 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -519,11 +519,7 @@ static int git_push_config(const char *k, const char *v,
 	} else if (!strcmp(k, "push.pushoption")) {
 		if (!v)
 			return config_error_nonbool(k);
-		else
-			if (!*v)
-				string_list_clear(&push_options_config, 0);
-			else
-				string_list_append(&push_options_config, v);
+		parse_transport_option(v, &push_options_config);
 		return 0;
 	} else if (!strcmp(k, "color.push")) {
 		push_use_color = git_config_colorbool(k, v);
diff --git a/transport.c b/transport.c
index bab28965f96..ca6cd5b3436 100644
--- a/transport.c
+++ b/transport.c
@@ -1091,6 +1091,14 @@ int is_transport_allowed(const char *type, int from_user)
 	BUG("invalid protocol_allow_config type");
 }
 
+void parse_transport_option(const char *option, struct string_list *transport_options)
+{
+	if (!*option)
+		string_list_clear(transport_options, 0);
+	else
+		string_list_append(transport_options, option);
+}
+
 void transport_check_allowed(const char *type)
 {
 	if (!is_transport_allowed(type, -1))
diff --git a/transport.h b/transport.h
index 6393cd9823c..1b8735e2ca4 100644
--- a/transport.h
+++ b/transport.h
@@ -342,4 +342,7 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 /* common method used by transport-helper.c and send-pack.c */
 void reject_atomic_push(struct ref *refs, int mirror_mode);
 
+/* common method to parse push-option for pushes or server-option for fetches */
+void parse_transport_option(const char *option, struct string_list *transport_options);
+
 #endif
-- 
gitgitgadget

[PATCH 2/4] builtin/fetch.c: add fetch.serverOption configuration

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Currently, server options for Git protocol v2 can only be passed via the
command line using "--server-option <option>", which is inconvenient
when we want to specify a list of default options to send.

This commit introduces a new multi-valued configuration
`fetch.serverOption`, which is designed similar to `push.pushOption`. It
allows specifying default server options to be sent automatically if not
explicitly set. Subsequent commits will extend this configuration to
other commands that involve server options, such as git-ls-remote and
git-clone.

When server options are set in lower-priority configuration files (e.g.,
/etc/gitconfig or $HOME/.gitconfig), they can be overridden or unset in
more specific repository configurations using an empty string.

Tests and documentation have been updated accordingly.

Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 Documentation/config/fetch.txt  | 29 +++++++++++++++++++++++
 Documentation/fetch-options.txt |  3 +++
 builtin/fetch.c                 | 31 +++++++++++++++++-------
 t/t5702-protocol-v2.sh          | 42 +++++++++++++++++++++++++++------
 4 files changed, 90 insertions(+), 15 deletions(-)

diff --git a/Documentation/config/fetch.txt b/Documentation/config/fetch.txt
index d7dc461bd16..9987d2a7425 100644
--- a/Documentation/config/fetch.txt
+++ b/Documentation/config/fetch.txt
@@ -126,3 +126,32 @@ fetch.bundleCreationToken::
 The creation token values are chosen by the provider serving the specific
 bundle URI. If you modify the URI at `fetch.bundleURI`, then be sure to
 remove the value for the `fetch.bundleCreationToken` value before fetching.
+
+fetch.serverOption::
+	When no `--server-option=<option>` argument is given from the
+	command line, fetch behaves as if each <value> of this variable is
+	given as `--server-option=<value>`.
++
+This is a multi-valued variable, and an empty value can be used in a higher
+priority configuration file (e.g. `.git/config` in a repository) to clear
+the values inherited from a lower priority configuration files (e.g.
+`$HOME/.gitconfig`).
++
+----
+
+Example:
+
+/etc/gitconfig
+  fetch.serverOption = a
+  fetch.serverOption = b
+
+~/.gitconfig
+  fetch.serverOption = c
+
+repo/.git/config
+  fetch.serverOption =
+  fetch.serverOption = b
+
+This will result in only b (a and c are cleared).
+
+----
diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index e22b217fba9..39969aeb326 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -304,6 +304,9 @@ endif::git-pull[]
 	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.
 
 --show-forced-updates::
 	By default, git checks if a branch is force-updated during
diff --git a/builtin/fetch.c b/builtin/fetch.c
index c297569a473..d49dff8a76a 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -95,7 +95,7 @@ static struct transport *gtransport;
 static struct transport *gsecondary;
 static struct refspec refmap = REFSPEC_INIT_FETCH;
 static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;
-static struct string_list server_options = STRING_LIST_INIT_DUP;
+static struct string_list config_server_options = STRING_LIST_INIT_DUP;
 static struct string_list negotiation_tip = STRING_LIST_INIT_NODUP;
 
 struct fetch_config {
@@ -170,6 +170,13 @@ static int git_fetch_config(const char *k, const char *v,
 			    "fetch.output", 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);
 }
 
@@ -2063,7 +2070,8 @@ static inline void fetch_one_setup_partial(struct remote *remote)
 
 static int fetch_one(struct remote *remote, int argc, const char **argv,
 		     int prune_tags_ok, int use_stdin_refspecs,
-		     const struct fetch_config *config)
+		     const struct fetch_config *config,
+		     struct string_list *server_options)
 {
 	struct refspec rs = REFSPEC_INIT_FETCH;
 	int i;
@@ -2124,8 +2132,8 @@ static int fetch_one(struct remote *remote, int argc, const char **argv,
 		strbuf_release(&line);
 	}
 
-	if (server_options.nr)
-		gtransport->server_options = &server_options;
+	if (server_options && server_options->nr)
+		gtransport->server_options = server_options;
 
 	sigchain_push_common(unlock_pack_on_signal);
 	atexit(unlock_pack_atexit);
@@ -2152,6 +2160,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	const char *submodule_prefix = "";
 	const char *bundle_uri;
 	struct string_list list = STRING_LIST_INIT_DUP;
+	struct string_list option_server_options = STRING_LIST_INIT_DUP;
+	struct string_list *server_options = NULL;
 	struct remote *remote = NULL;
 	int all = -1, multiple = 0;
 	int result = 0;
@@ -2231,7 +2241,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			 N_("accept refs that update .git/shallow")),
 		OPT_CALLBACK_F(0, "refmap", &refmap, N_("refmap"),
 			       N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
-		OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
+		OPT_STRING_LIST('o', "server-option", &option_server_options, N_("server-specific"),
+				N_("option to transmit")),
 		OPT_IPVERSION(&family),
 		OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
 				N_("report that we have only objects reachable from this object")),
@@ -2272,6 +2283,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 	argc = parse_options(argc, argv, prefix,
 			     builtin_fetch_options, builtin_fetch_usage, 0);
 
+	server_options = option_server_options.nr ?
+			 &option_server_options : &config_server_options;
+
 	if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
 		config.recurse_submodules = recurse_submodules_cli;
 
@@ -2418,8 +2432,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 			result = 1;
 			goto cleanup;
 		}
-		if (server_options.nr)
-			gtransport->server_options = &server_options;
+		if (server_options && server_options->nr)
+			gtransport->server_options = server_options;
 		result = transport_fetch_refs(gtransport, NULL);
 
 		oidset_iter_init(&acked_commits, &iter);
@@ -2430,7 +2444,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 		if (filter_options.choice || repo_has_promisor_remote(the_repository))
 			fetch_one_setup_partial(remote);
 		result = fetch_one(remote, argc, argv, prune_tags_ok, stdin_refspecs,
-				   &config);
+				   &config, server_options);
 	} else {
 		int max_children = max_jobs;
 
@@ -2529,5 +2543,6 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
 
  cleanup:
 	string_list_clear(&list, 0);
+	string_list_clear(&option_server_options, 0);
 	return result;
 }
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 1ef540f73d3..ae25400010e 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -368,17 +368,45 @@ test_expect_success 'ref advertisement is filtered during fetch using protocol v
 test_expect_success 'server-options are sent when fetching' '
 	test_when_finished "rm -f log" &&
 
-	test_commit -C file_parent four &&
-
+	# Specify server options from command line
 	GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
 		fetch -o hello -o world origin main &&
+	test_grep "server-option=hello" log &&
+	test_grep "server-option=world" log &&
+	rm -f log &&
 
-	git -C file_child log -1 --format=%s origin/main >actual &&
-	git -C file_parent log -1 --format=%s >expect &&
-	test_cmp expect actual &&
+	# Specify server options from fetch.serverOption config
+	GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
+		-c fetch.serverOption=hello -c fetch.serverOption=world \
+		fetch origin main &&
+	test_grep "server-option=hello" log &&
+	test_grep "server-option=world" log &&
+	rm -f log &&
 
-	grep "server-option=hello" log &&
-	grep "server-option=world" log
+	# Cmdline server options take a higher priority
+	GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
+		-c fetch.serverOption=hello -c fetch.serverOption=world \
+		fetch -o foo=bar origin main &&
+	test_grep ! "server-option=hello" log &&
+	test_grep ! "server-option=world" log &&
+	test_grep "server-option=foo=bar" log
+'
+
+test_expect_success 'empty value of fetch.serverOption in config clears the list' '
+	test_when_finished "rm -f log" &&
+	GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
+		-c fetch.serverOption=hello -c fetch.serverOption=world \
+		-c fetch.serverOption= -c fetch.serverOption=foo=bar \
+		fetch origin main &&
+	test_grep ! "server-option=hello" log &&
+	test_grep ! "server-option=world" log &&
+	test_grep "server-option=foo=bar" log
+'
+
+test_expect_success 'invalid fetch.serverOption in config' '
+	test_when_finished "rm -f log" &&
+	test_must_fail git -C file_child -c protocol.version=2 \
+		-c fetch.serverOption fetch origin main
 '
 
 test_expect_success 'warn if using server-option with fetch with legacy protocol' '
-- 
gitgitgadget

Re: [PATCH 2/4] builtin/fetch.c: add fetch.serverOption configuration

[Patrick Steinhardt]

On Mon, Sep 02, 2024 at 12:13:54PM +0000, Xing Xin via GitGitGadget wrote:
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index c297569a473..d49dff8a76a 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -2231,7 +2241,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  			 N_("accept refs that update .git/shallow")),
>  		OPT_CALLBACK_F(0, "refmap", &refmap, N_("refmap"),
>  			       N_("specify fetch refmap"), PARSE_OPT_NONEG, parse_refmap_arg),
> -		OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
> +		OPT_STRING_LIST('o', "server-option", &option_server_options, N_("server-specific"),
> +				N_("option to transmit")),
>  		OPT_IPVERSION(&family),
>  		OPT_STRING_LIST(0, "negotiation-tip", &negotiation_tip, N_("revision"),
>  				N_("report that we have only objects reachable from this object")),
> @@ -2272,6 +2283,9 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  	argc = parse_options(argc, argv, prefix,
>  			     builtin_fetch_options, builtin_fetch_usage, 0);
>  
> +	server_options = option_server_options.nr ?
> +			 &option_server_options : &config_server_options;
> +
>  	if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
>  		config.recurse_submodules = recurse_submodules_cli;
>  

Doesn't this mean that `server_options` will never be `NULL`? Why do we
have the new checks for whether or not `server_options` is set, like
e.g. in the next hunk.

> @@ -2418,8 +2432,8 @@ int cmd_fetch(int argc, const char **argv, const char *prefix)
>  			result = 1;
>  			goto cleanup;
>  		}
> -		if (server_options.nr)
> -			gtransport->server_options = &server_options;
> +		if (server_options && server_options->nr)
> +			gtransport->server_options = server_options;
>  		result = transport_fetch_refs(gtransport, NULL);
>  
>  		oidset_iter_init(&acked_commits, &iter);
> diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
> index 1ef540f73d3..ae25400010e 100755
> --- a/t/t5702-protocol-v2.sh
> +++ b/t/t5702-protocol-v2.sh
> @@ -368,17 +368,45 @@ test_expect_success 'ref advertisement is filtered during fetch using protocol v
>  test_expect_success 'server-options are sent when fetching' '
>  	test_when_finished "rm -f log" &&
>  
> -	test_commit -C file_parent four &&
> -
> +	# Specify server options from command line
>  	GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
>  		fetch -o hello -o world origin main &&
> +	test_grep "server-option=hello" log &&
> +	test_grep "server-option=world" log &&
> +	rm -f log &&
>  
> -	git -C file_child log -1 --format=%s origin/main >actual &&
> -	git -C file_parent log -1 --format=%s >expect &&
> -	test_cmp expect actual &&
> +	# Specify server options from fetch.serverOption config
> +	GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
> +		-c fetch.serverOption=hello -c fetch.serverOption=world \
> +		fetch origin main &&
> +	test_grep "server-option=hello" log &&
> +	test_grep "server-option=world" log &&
> +	rm -f log &&
>  
> -	grep "server-option=hello" log &&
> -	grep "server-option=world" log
> +	# Cmdline server options take a higher priority
> +	GIT_TRACE_PACKET="$(pwd)/log" git -C file_child -c protocol.version=2 \
> +		-c fetch.serverOption=hello -c fetch.serverOption=world \
> +		fetch -o foo=bar origin main &&
> +	test_grep ! "server-option=hello" log &&
> +	test_grep ! "server-option=world" log &&
> +	test_grep "server-option=foo=bar" log
> +'

I think it makes more sense to check for the new behaviour in new tests
exclusively. Otherwise one is left wondering whether any of the old
behaviour changed.

Patrick

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

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Teach git-clone to recognize the `fetch.serverOption` configuration as a
default list of server options to send for Git protocol v2, if server
options are not explicitly set via the command line.

Note that `builtin/clone.c:cmd_clone` originally read the git config
twice via `builtin/clone.c:git_clone_config`, which would duplicate
server options if parsing logic were added there. Upon investigation, it
was found that the first config read is unnecessary since all the global
variables it sets are actually used after the second config read.
Therefore, the first config read is replaced with a simple
`config.c:git_default_config`.

Tests and documentation have been updated accordingly.

Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 Documentation/git-clone.txt |  3 +++
 builtin/clone.c             | 22 ++++++++++++++++------
 t/t5702-protocol-v2.sh      | 22 ++++++++++++++++++++--
 3 files changed, 39 insertions(+), 8 deletions(-)

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`::
diff --git a/builtin/clone.c b/builtin/clone.c
index 269b6e18a4e..5a1e2e769af 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -85,7 +85,8 @@ static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP;
 static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;
 static int option_filter_submodules = -1;    /* unspecified */
 static int config_filter_submodules = -1;    /* unspecified */
-static struct string_list server_options = STRING_LIST_INIT_NODUP;
+static struct string_list config_server_options = STRING_LIST_INIT_DUP;
+static struct string_list option_server_options = STRING_LIST_INIT_DUP;
 static int option_remote_submodules;
 static const char *bundle_uri;
 
@@ -160,7 +161,7 @@ static struct option builtin_clone_options[] = {
 		   N_("specify the reference format to use")),
 	OPT_STRING_LIST('c', "config", &option_config, N_("key=value"),
 			N_("set config inside the new repository")),
-	OPT_STRING_LIST(0, "server-option", &server_options,
+	OPT_STRING_LIST(0, "server-option", &option_server_options,
 			N_("server-specific"), N_("option to transmit")),
 	OPT_IPVERSION(&family),
 	OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
@@ -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);
 }
@@ -982,17 +989,20 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 	int hash_algo;
 	enum ref_storage_format ref_storage_format = REF_STORAGE_FORMAT_UNKNOWN;
 	const int do_not_override_repo_unix_permissions = -1;
-
+	struct string_list *server_options = NULL;
 	struct transport_ls_refs_options transport_ls_refs_options =
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
 
 	packet_trace_identity("clone");
 
-	git_config(git_clone_config, NULL);
+	git_config(git_default_config, NULL);
 
 	argc = parse_options(argc, argv, prefix, builtin_clone_options,
 			     builtin_clone_usage, 0);
 
+	server_options = option_server_options.nr ?
+			 &option_server_options : &config_server_options;
+
 	if (argc > 2)
 		usage_msg_opt(_("Too many arguments."),
 			builtin_clone_usage, builtin_clone_options);
@@ -1359,8 +1369,8 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK,
 				     option_upload_pack);
 
-	if (server_options.nr)
-		transport->server_options = &server_options;
+	if (server_options && server_options->nr)
+		transport->server_options = server_options;
 
 	if (filter_options.choice) {
 		const char *spec =
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index ae25400010e..3bf31fb570d 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -424,12 +424,30 @@ test_expect_success 'warn if using server-option with fetch with legacy protocol
 test_expect_success 'server-options are sent when cloning' '
 	test_when_finished "rm -rf log myclone" &&
 
+	# Specify server options from command line
 	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
 		clone --server-option=hello --server-option=world \
 		"file://$(pwd)/file_parent" myclone &&
+	test_grep "server-option=hello" log &&
+	test_grep "server-option=world" log &&
+	rm -rf log myclone &&
 
-	grep "server-option=hello" log &&
-	grep "server-option=world" log
+	# Specify server options from fetch.serverOption config
+	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+		-c fetch.serverOption=hello -c fetch.serverOption=world \
+		clone "file://$(pwd)/file_parent" myclone &&
+	test_grep "server-option=hello" log &&
+	test_grep "server-option=world" log &&
+	rm -rf log myclone &&
+
+	# Cmdline server options take a higher priority
+	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+		-c fetch.serverOption=hello -c fetch.serverOption=world \
+		clone --server-option=foo=bar \
+		"file://$(pwd)/file_parent" myclone &&
+	test_grep ! "server-option=hello" log &&
+	test_grep ! "server-option=world" log &&
+	test_grep "server-option=foo=bar" log
 '
 
 test_expect_success 'warn if using server-option with clone with legacy protocol' '
-- 
gitgitgadget

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

[Patrick Steinhardt]

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

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

[Xing Xin]

At 2024-09-03 18:09:45, "Patrick Steinhardt" <ps@pks.im> wrote:
>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.

Sorry for not explaining our use case clearly. We have several internal
repositories configured with numerous CI tasks, each requiring code
preparation (sometimes via clone, sometimes via fetch). These CI tasks
are ususally triggered by post-receive hook, so the concurrent tasks are
actually fetching the same copy of code.

On git server, we want to deploy a special pack-objects-hook to mitigate
the performance impacts caused by these CI tasks (so the packfile
produced by git-pack-objects can be reused).  Since not all clone/fetch
operations can benefit from this caching mechanism (e.g. pulls from
users' dev environment), we need the client to pass a special identifier
to inform the server whether caching support should be enabled for that
clone/fetch. Clearly, using server options is a good choice.

To achieve our design, we need to add two patch series to git:

1. Support injecting server options to identify environments via
   configuration, because adding the --server-option parameter would
   require too many script modifications, making it difficult to deploy.
   This is what this patch series does.
2. Git server should pass the received server options as environment
   variables (similar to push options) to the pack-objects-hook.

>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

I named the new configuration `fetch.serverOption` mainly to follow the
`push.pushOption` pattern.  Since which server options to support is
actually server-specific, using `remote.<name>.serverOption` is a good
idea for git-fetch. However, how should we design the configuration for
git-ls-remote or git-clone, if we wanna provide all of them with a
default list of server options to send?

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

Xing Xin

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

[Patrick Steinhardt]

On Wed, Sep 04, 2024 at 03:49:28PM +0800, Xing Xin wrote:
> At 2024-09-03 18:09:45, "Patrick Steinhardt" <ps@pks.im> wrote:
> >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.
> 
> Sorry for not explaining our use case clearly. We have several internal
> repositories configured with numerous CI tasks, each requiring code
> preparation (sometimes via clone, sometimes via fetch). These CI tasks
> are ususally triggered by post-receive hook, so the concurrent tasks are
> actually fetching the same copy of code.
> 
> On git server, we want to deploy a special pack-objects-hook to mitigate
> the performance impacts caused by these CI tasks (so the packfile
> produced by git-pack-objects can be reused).  Since not all clone/fetch
> operations can benefit from this caching mechanism (e.g. pulls from
> users' dev environment), we need the client to pass a special identifier
> to inform the server whether caching support should be enabled for that
> clone/fetch. Clearly, using server options is a good choice.
> 
> To achieve our design, we need to add two patch series to git:
> 
> 1. Support injecting server options to identify environments via
>    configuration, because adding the --server-option parameter would
>    require too many script modifications, making it difficult to deploy.
>    This is what this patch series does.
> 2. Git server should pass the received server options as environment
>    variables (similar to push options) to the pack-objects-hook.

When you talk about client, do you mean that the actual users will have
to configure this? That sounds somewhat unmaintainable on your side from
the surface. I guess I ain't got enough knowledge around the environment
you operate in though, so I probably shouldn't judge.

> >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
> 
> I named the new configuration `fetch.serverOption` mainly to follow the
> `push.pushOption` pattern.  Since which server options to support is
> actually server-specific, using `remote.<name>.serverOption` is a good
> idea for git-fetch. However, how should we design the configuration for
> git-ls-remote or git-clone, if we wanna provide all of them with a
> default list of server options to send?

As mentioned in another reply, I think that putting this into the remote
configuration "remote.*.serverOption" might be a better solution, as it
also brings you the ability to set this per remote by default.

Patrick

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

[Xing Xin]

At 2024-09-05 19:05:15, "Patrick Steinhardt" <ps@pks.im> wrote:
>On Wed, Sep 04, 2024 at 03:49:28PM +0800, Xing Xin wrote:
>> At 2024-09-03 18:09:45, "Patrick Steinhardt" <ps@pks.im> wrote:
>> >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.
>> 
>> Sorry for not explaining our use case clearly. We have several internal
>> repositories configured with numerous CI tasks, each requiring code
>> preparation (sometimes via clone, sometimes via fetch). These CI tasks
>> are ususally triggered by post-receive hook, so the concurrent tasks are
>> actually fetching the same copy of code.
>> 
>> On git server, we want to deploy a special pack-objects-hook to mitigate
>> the performance impacts caused by these CI tasks (so the packfile
>> produced by git-pack-objects can be reused).  Since not all clone/fetch
>> operations can benefit from this caching mechanism (e.g. pulls from
>> users' dev environment), we need the client to pass a special identifier
>> to inform the server whether caching support should be enabled for that
>> clone/fetch. Clearly, using server options is a good choice.
>> 
>> To achieve our design, we need to add two patch series to git:
>> 
>> 1. Support injecting server options to identify environments via
>>    configuration, because adding the --server-option parameter would
>>    require too many script modifications, making it difficult to deploy.
>>    This is what this patch series does.
>> 2. Git server should pass the received server options as environment
>>    variables (similar to push options) to the pack-objects-hook.
>
>When you talk about client, do you mean that the actual users will have
>to configure this? That sounds somewhat unmaintainable on your side from
>the surface. I guess I ain't got enough knowledge around the environment
>you operate in though, so I probably shouldn't judge.

Yes, that is indeed our design goal. We want specially configured git
clients to benefit from caching acceleration when cloning code, while
clients without special configurations follow the regular logic. This is
because code fetching in CI environments is often homogeneous, making it
worthwhile to implement caching logic to speed up the process and reduce
server load. In contrast, code fetching from users is usually diverse,
making caching less valuable.

By linking pack-objects-hook with server options, we believe we can
effectively differentiate between various clone behaviors. In fact, I
think a similar design can also be extended to GitHub Actions or GitLab
CI to save CPU on git servers, although I'm not sure whether a similar
mechanism is already available.

CI environments are usually provided by the infrastructure team, making this
solution easier to maintain, and we do not expect any changes on real users.

>> >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
>> 
>> I named the new configuration `fetch.serverOption` mainly to follow the
>> `push.pushOption` pattern.  Since which server options to support is
>> actually server-specific, using `remote.<name>.serverOption` is a good
>> idea for git-fetch. However, how should we design the configuration for
>> git-ls-remote or git-clone, if we wanna provide all of them with a
>> default list of server options to send?
>
>As mentioned in another reply, I think that putting this into the remote
>configuration "remote.*.serverOption" might be a better solution, as it
>also brings you the ability to set this per remote by default.

I agree that using "remote.*.serverOption" is better. In fact, I also
think "push.pushOption" would be better as "remote.*.pushOption". What I'm
contemplating is whether we need to add a configuration for a default
list of server options, so that when "remote.origin.serverOption" is not
present, we can fall back to use that as default.


Xing Xin

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

[Patrick Steinhardt]

On Thu, Sep 05, 2024 at 08:12:58PM +0800, Xing Xin wrote:
> At 2024-09-05 19:05:15, "Patrick Steinhardt" <ps@pks.im> wrote:
> >On Wed, Sep 04, 2024 at 03:49:28PM +0800, Xing Xin wrote:
> >> At 2024-09-03 18:09:45, "Patrick Steinhardt" <ps@pks.im> wrote:
> >> >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
> >> 
> >> I named the new configuration `fetch.serverOption` mainly to follow the
> >> `push.pushOption` pattern.  Since which server options to support is
> >> actually server-specific, using `remote.<name>.serverOption` is a good
> >> idea for git-fetch. However, how should we design the configuration for
> >> git-ls-remote or git-clone, if we wanna provide all of them with a
> >> default list of server options to send?
> >
> >As mentioned in another reply, I think that putting this into the remote
> >configuration "remote.*.serverOption" might be a better solution, as it
> >also brings you the ability to set this per remote by default.
> 
> I agree that using "remote.*.serverOption" is better. In fact, I also
> think "push.pushOption" would be better as "remote.*.pushOption". What I'm
> contemplating is whether we need to add a configuration for a default
> list of server options, so that when "remote.origin.serverOption" is not
> present, we can fall back to use that as default.

Junio proposed in [1] to introduce `[remote "*"]` syntax to supply
default values to all remotes. You could pick up that proposal and
implement it as part of your patch series.

I also have to agree that "push.pushOption" would be way more sensible
if it was configured per remote. I think it would be sensible to also
introduce "remote.*.pushOption" in the same way and have it override the
default value of "push.pushOption" if present. So the precedence order
would become (from high to low):

  - remote.someRemote.pushOption
  - remote."*".pushOption
  - push.PushOption

This should be backwards compatible, too.

Patrick

[1]: <xmqq5xrcn2k1.fsf@gitster.g>

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

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> I also have to agree that "push.pushOption" would be way more sensible
> if it was configured per remote. I think it would be sensible to also
> introduce "remote.*.pushOption" in the same way and have it override the
> default value of "push.pushOption" if present. So the precedence order
> would become (from high to low):
>
>   - remote.someRemote.pushOption
>   - remote."*".pushOption
>   - push.PushOption
>
> This should be backwards compatible, too.

;-)  Sounds sensible.

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

[Xing Xin]

At 2024-09-05 21:44:41, "Patrick Steinhardt" <ps@pks.im> wrote

[snip]

>> >> I named the new configuration `fetch.serverOption` mainly to follow the
>> >> `push.pushOption` pattern.  Since which server options to support is
>> >> actually server-specific, using `remote.<name>.serverOption` is a good
>> >> idea for git-fetch. However, how should we design the configuration for
>> >> git-ls-remote or git-clone, if we wanna provide all of them with a
>> >> default list of server options to send?
>> >
>> >As mentioned in another reply, I think that putting this into the remote
>> >configuration "remote.*.serverOption" might be a better solution, as it
>> >also brings you the ability to set this per remote by default.
>> 
>> I agree that using "remote.*.serverOption" is better. In fact, I also
>> think "push.pushOption" would be better as "remote.*.pushOption". What I'm
>> contemplating is whether we need to add a configuration for a default
>> list of server options, so that when "remote.origin.serverOption" is not
>> present, we can fall back to use that as default.
>
>Junio proposed in [1] to introduce `[remote "*"]` syntax to supply
>default values to all remotes. You could pick up that proposal and
>implement it as part of your patch series.

Given the addition of a remote.*.<subkey> configuration in Git's global
settings, such as:

    git config --global remote."*".demoConfigKey demoConfigValue

The current versions of Git may produce errors with certain commands.
For example, running `git fetch --all` will result in:

    $ git fetch --all
    Fetching *
    fatal: '*' does not appear to be a git repository
    fatal: Could not read from remote repository.

    Please make sure you have the correct access rights
    and the repository exists.
    error: could not fetch *
    Fetching origin

This raises the question of whether this can be defined as an
incompatibility or whether this is acceptable. If it isn't, I prefer
using a `remote.defaultServerOption` instead, as it aligns with our
existing use of `remote.pushDefault` anyway.

[snip]

Xing Xin

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

[Patrick Steinhardt]

On Mon, Sep 09, 2024 at 10:50:21AM +0800, Xing Xin wrote:
> At 2024-09-05 21:44:41, "Patrick Steinhardt" <ps@pks.im> wrote
> 
> [snip]
> 
> >> >> I named the new configuration `fetch.serverOption` mainly to follow the
> >> >> `push.pushOption` pattern.  Since which server options to support is
> >> >> actually server-specific, using `remote.<name>.serverOption` is a good
> >> >> idea for git-fetch. However, how should we design the configuration for
> >> >> git-ls-remote or git-clone, if we wanna provide all of them with a
> >> >> default list of server options to send?
> >> >
> >> >As mentioned in another reply, I think that putting this into the remote
> >> >configuration "remote.*.serverOption" might be a better solution, as it
> >> >also brings you the ability to set this per remote by default.
> >> 
> >> I agree that using "remote.*.serverOption" is better. In fact, I also
> >> think "push.pushOption" would be better as "remote.*.pushOption". What I'm
> >> contemplating is whether we need to add a configuration for a default
> >> list of server options, so that when "remote.origin.serverOption" is not
> >> present, we can fall back to use that as default.
> >
> >Junio proposed in [1] to introduce `[remote "*"]` syntax to supply
> >default values to all remotes. You could pick up that proposal and
> >implement it as part of your patch series.
> 
> Given the addition of a remote.*.<subkey> configuration in Git's global
> settings, such as:
> 
>     git config --global remote."*".demoConfigKey demoConfigValue
> 
> The current versions of Git may produce errors with certain commands.
> For example, running `git fetch --all` will result in:
> 
>     $ git fetch --all
>     Fetching *
>     fatal: '*' does not appear to be a git repository
>     fatal: Could not read from remote repository.
> 
>     Please make sure you have the correct access rights
>     and the repository exists.
>     error: could not fetch *
>     Fetching origin

We do not currently handle "remote.*.something", so we'd first have to
add support for this syntax.

> This raises the question of whether this can be defined as an
> incompatibility or whether this is acceptable. If it isn't, I prefer
> using a `remote.defaultServerOption` instead, as it aligns with our
> existing use of `remote.pushDefault` anyway.

While for this config key it may work alright, I think supporting
"remote.*.something" is preferable. Ideally, it would be generic enough
to apply to all per-remote settings such that we don't have to add
support for every single config key that applies to remotes.

Right now we only care about push options and server options. But
somebody may want to globally configure proxies, tag options, partial
clone filters or something else. And if we got all of that without
having to introduce `remote.defaultPruneTags`,
`remote.defaultPartialCloneFilter` and similar options I'd consider it a
win.

Patrick

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

[Xing Xin]

At 2024-09-09 19:49:16, "Patrick Steinhardt" <ps@pks.im> wrote:
>On Mon, Sep 09, 2024 at 10:50:21AM +0800, Xing Xin wrote:
>> At 2024-09-05 21:44:41, "Patrick Steinhardt" <ps@pks.im> wrote
>> 
>> [snip]
>> 
>> >> >> I named the new configuration `fetch.serverOption` mainly to follow the
>> >> >> `push.pushOption` pattern.  Since which server options to support is
>> >> >> actually server-specific, using `remote.<name>.serverOption` is a good
>> >> >> idea for git-fetch. However, how should we design the configuration for
>> >> >> git-ls-remote or git-clone, if we wanna provide all of them with a
>> >> >> default list of server options to send?
>> >> >
>> >> >As mentioned in another reply, I think that putting this into the remote
>> >> >configuration "remote.*.serverOption" might be a better solution, as it
>> >> >also brings you the ability to set this per remote by default.
>> >> 
>> >> I agree that using "remote.*.serverOption" is better. In fact, I also
>> >> think "push.pushOption" would be better as "remote.*.pushOption". What I'm
>> >> contemplating is whether we need to add a configuration for a default
>> >> list of server options, so that when "remote.origin.serverOption" is not
>> >> present, we can fall back to use that as default.
>> >
>> >Junio proposed in [1] to introduce `[remote "*"]` syntax to supply
>> >default values to all remotes. You could pick up that proposal and
>> >implement it as part of your patch series.
>> 
>> Given the addition of a remote.*.<subkey> configuration in Git's global
>> settings, such as:
>> 
>>     git config --global remote."*".demoConfigKey demoConfigValue
>> 
>> The current versions of Git may produce errors with certain commands.
>> For example, running `git fetch --all` will result in:
>> 
>>     $ git fetch --all
>>     Fetching *
>>     fatal: '*' does not appear to be a git repository
>>     fatal: Could not read from remote repository.
>> 
>>     Please make sure you have the correct access rights
>>     and the repository exists.
>>     error: could not fetch *
>>     Fetching origin
>
>We do not currently handle "remote.*.something", so we'd first have to
>add support for this syntax.
>
>> This raises the question of whether this can be defined as an
>> incompatibility or whether this is acceptable. If it isn't, I prefer
>> using a `remote.defaultServerOption` instead, as it aligns with our
>> existing use of `remote.pushDefault` anyway.
>
>While for this config key it may work alright, I think supporting
>"remote.*.something" is preferable. Ideally, it would be generic enough
>to apply to all per-remote settings such that we don't have to add
>support for every single config key that applies to remotes.
>
>Right now we only care about push options and server options. But
>somebody may want to globally configure proxies, tag options, partial
>clone filters or something else. And if we got all of that without
>having to introduce `remote.defaultPruneTags`,
>`remote.defaultPartialCloneFilter` and similar options I'd consider it a
>win.

After some consideration, I tend not to implement a `remote.<subkey>`
or `remote.*.<subkey>` as a fallback default in this patch series.

In fact I attempted to use `remote.serverOption` and extended a new
`server_options` field in `remote.h:remote_state` to store its parsing
result. However, I think this approach isn't general enough (it only
differs in name from the `remote.defaultServerOption`).

Implementing a mechanism similar to `http.<url>.<subkey>` and
`http.<subkey>`(the fallback default) seems a better approach, but it
involves more refactoring, which is too much to do at this time.

I have just submitted my v2 series, which only adds the
`remote.<name>.serverOption` configuration.


Xing Xin

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

[Junio C Hamano]

"Xing Xin" <bupt_xingxin@163.com> writes:

> Given the addition of a remote.*.<subkey> configuration in Git's global
> settings, such as:
>
>     git config --global remote."*".demoConfigKey demoConfigValue
>
> The current versions of Git may produce errors with certain commands.
> For example, running `git fetch --all` will result in:
>
>     $ git fetch --all
>     Fetching *
>     fatal: '*' does not appear to be a git repository
>     fatal: Could not read from remote repository.

Ah, good point.  Anything that wants to enumerate the subkeys under
remote. hierarchy MUST be aware of the new convention.

So such a code must of course need to be updated to treat "*" as a
virtual thing and exclude from enumeration (I suspect "git remote"
has the same property), and delivered to the end-users at the same
time.

An alternative is to use remote.<key> as a fallback default for
remote.<name>.<key>, which has been done successfully for other
hierarchies like http.<url>.<key> would override http.<key> for any
defined <key>s.  So if we have remote.<name>.skipFetchAll per
remote, we could use remote.skipFetchAll as its fallback default
value.

[PATCH 4/4] builtin/ls-remote.c: recognize fetch.serverOption configuration

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Teach git-ls-remote to recognize the `fetch.serverOption` configuration
as a default list of server options to send for Git protocol v2, if
server options are not explicitly set via the command line.

Tests and documentation have been updated accordingly.

Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 Documentation/git-ls-remote.txt |  3 +++
 builtin/ls-remote.c             | 32 ++++++++++++++++++++++++++++----
 t/t5702-protocol-v2.sh          | 29 ++++++++++++++++++++++++-----
 3 files changed, 55 insertions(+), 9 deletions(-)

diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 76c86c3ce4f..fc9930193f8 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -81,6 +81,9 @@ OPTIONS
 	character.
 	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.
 
 <repository>::
 	The "remote" repository to query.  This parameter can be
diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 0a491595ca8..87afd69828c 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -1,4 +1,5 @@
 #include "builtin.h"
+#include "config.h"
 #include "gettext.h"
 #include "hex.h"
 #include "transport.h"
@@ -8,6 +9,8 @@
 #include "parse-options.h"
 #include "wildmatch.h"
 
+static struct string_list config_server_options = STRING_LIST_INIT_DUP;
+
 static const char * const ls_remote_usage[] = {
 	N_("git ls-remote [--branches] [--tags] [--refs] [--upload-pack=<exec>]\n"
 	   "              [-q | --quiet] [--exit-code] [--get-url] [--sort=<key>]\n"
@@ -37,6 +40,18 @@ static int tail_match(const struct strvec *pattern, const char *path)
 	return 0;
 }
 
+static int git_ls_remote_config(const char *k, const char *v,
+			    const struct config_context *ctx, void *cb)
+{
+	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);
+}
+
 int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 {
 	const char *dest = NULL;
@@ -49,8 +64,9 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	struct strvec pattern = STRVEC_INIT;
 	struct transport_ls_refs_options transport_options =
 		TRANSPORT_LS_REFS_OPTIONS_INIT;
+	struct string_list option_server_options = STRING_LIST_INIT_DUP;
+	struct string_list *server_options = NULL;
 	int i;
-	struct string_list server_options = STRING_LIST_INIT_DUP;
 
 	struct remote *remote;
 	struct transport *transport;
@@ -80,16 +96,23 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 			      2, PARSE_OPT_NOCOMPLETE),
 		OPT_BOOL(0, "symref", &show_symref_target,
 			 N_("show underlying ref in addition to the object pointed by it")),
-		OPT_STRING_LIST('o', "server-option", &server_options, N_("server-specific"), N_("option to transmit")),
+		OPT_STRING_LIST('o', "server-option", &option_server_options,
+				N_("server-specific"),
+				N_("option to transmit")),
 		OPT_END()
 	};
 
+	git_config(git_ls_remote_config, NULL);
+
 	memset(&ref_array, 0, sizeof(ref_array));
 
 	argc = parse_options(argc, argv, prefix, options, ls_remote_usage,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	dest = argv[0];
 
+	server_options = option_server_options.nr ?
+			 &option_server_options : &config_server_options;
+
 	/*
 	 * TODO: This is buggy, but required for transport helpers. When a
 	 * transport helper advertises a "refspec", then we'd add that to a
@@ -130,8 +153,8 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	transport = transport_get(remote, NULL);
 	if (uploadpack)
 		transport_set_option(transport, TRANS_OPT_UPLOADPACK, uploadpack);
-	if (server_options.nr)
-		transport->server_options = &server_options;
+	if (server_options && server_options->nr)
+		transport->server_options = server_options;
 
 	ref = transport_get_remote_refs(transport, &transport_options);
 	if (ref) {
@@ -169,5 +192,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	transport_ls_refs_options_release(&transport_options);
 
 	strvec_clear(&pattern);
+	string_list_clear(&option_server_options, 0);
 	return status;
 }
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 3bf31fb570d..da6241afa22 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -173,16 +173,35 @@ test_expect_success 'ref advertisement is filtered with ls-remote using protocol
 test_expect_success 'server-options are sent when using ls-remote' '
 	test_when_finished "rm -f log" &&
 
-	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
-		ls-remote -o hello -o world "file://$(pwd)/file_parent" main >actual &&
-
 	cat >expect <<-EOF &&
 	$(git -C file_parent rev-parse refs/heads/main)$(printf "\t")refs/heads/main
 	EOF
 
+	# Specify server options from command line
+	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+		ls-remote -o hello -o world "file://$(pwd)/file_parent" main >actual &&
+	test_cmp expect actual &&
+	test_grep "server-option=hello" log &&
+	test_grep "server-option=world" log &&
+	rm -f log &&
+
+	# Specify server options from fetch.serverOption config
+	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+		-c fetch.serverOption=hello -c fetch.serverOption=world \
+		ls-remote "file://$(pwd)/file_parent" main >actual &&
+	test_cmp expect actual &&
+	test_grep "server-option=hello" log &&
+	test_grep "server-option=world" log &&
+	rm -f log &&
+
+	# Cmdline server options take a higher priority
+	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+		-c fetch.serverOption=hello -c fetch.serverOption=world \
+		ls-remote -o foo=bar "file://$(pwd)/file_parent" main >actual &&
 	test_cmp expect actual &&
-	grep "server-option=hello" log &&
-	grep "server-option=world" log
+	test_grep ! "server-option=hello" log &&
+	test_grep ! "server-option=world" log &&
+	test_grep "server-option=foo=bar" log
 '
 
 test_expect_success 'warn if using server-option with ls-remote with legacy protocol' '
-- 
gitgitgadget

Re: [PATCH 4/4] builtin/ls-remote.c: recognize fetch.serverOption configuration

[Patrick Steinhardt]

On Mon, Sep 02, 2024 at 12:13:56PM +0000, Xing Xin via GitGitGadget wrote:
> diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
> index 76c86c3ce4f..fc9930193f8 100644
> --- a/Documentation/git-ls-remote.txt
> +++ b/Documentation/git-ls-remote.txt
> @@ -81,6 +81,9 @@ OPTIONS
>  	character.
>  	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.
>  
>  <repository>::
>  	The "remote" repository to query.  This parameter can be

Hm. So `fetch.serverOption` now applies to git-fetch(1), git-clone(1)
and git-ls-remote(1). That feels somewhat unexpected to me given its
name, so I'm further leaning into the direction by now that it should be
a per-remote setting `remote.<name>.serverOption`.

Patrick

Re: [PATCH 0/4] Support server option from configuration

[Patrick Steinhardt]

On Mon, Sep 02, 2024 at 12:13:52PM +0000, blanet via GitGitGadget wrote:
> Currently, server options for Git protocol v2 can only be specified via the
> command line option "--server-option ", which is inconvenient for users who
> want to specify a list of default options.
> 
> This patch series introduces a new multi-valued configuration,
> fetch.serverOption, to specify default server options. Designed similarly to
> push.pushOption:
> 
>  1. Server options set in lower-priority configuration files (e.g.,
>     /etc/gitconfig or $HOME/.gitconfig) can be overridden or unset in more
>     specific repository configurations using an empty string.
>  2. Command-line specified server options take precedence over those from
>     the configuration.
> 
> All commands involving server options, including git-fetch, git-clone,
> git-ls-remote, and git-pull, have been updated to recognize the new
> configuration.

It would be nice to learn about the context this comes from. In which
scenario does it make sense to specify options by default? What is the
intended usecase? I'm sure that this feature proposal comes from a
specific usecase that you have at your employer, so learning a bit about
it would help to decide whether it makes sense to have or not.

Patrick

[PATCH v2 0/5] Support server option from configuration

[blanet via GitGitGadget]

We have multiple internal repositories configured with a huge number of CI
tasks, each requiring code preparation (either via clone or fetch). These CI
tasks, triggered by post-receive hooks, often fetch the same code (usually
using --depth=1) concurrently.

To reduce performance impacts on the git server caused by these tasks, we
want to deploy a special designed pack-objects-hook. This hook would allow
the packs generated by git-pack-objects to be reused. Since not all
clone/fetch operations will benefit from this caching (e.g., pulls from
developers' environments), clients need to pass a special identifier to
indicate whether caching should be enabled for a particular operation. Using
server options is a suitable method for this.

However, server options can only be specified via the command line option
(via --server-option or -o), which is inconvenient and requires CI script
modifications. A configuration-based approach is easier to propagate (by
changing the global configuration ~/.gitconfig) and avoids issues with older
Git versions that don't support --server-option.

This patch series introduces a new multi-valued configuration,
remote.<name>.serverOption, similar to push.pushOption, to specify default
server options for the corresponding remote.

 * Patch 1~3 is the main change for introducing the new configuration.
 * Patch 4 fixes a issue for git-fetch not sending server-options when
   fetching multiple remotes.
 * Patch 5 is a minor fix for a server options-related memory leak.

Xing Xin (5):
  transport: introduce parse_transport_option() method
  remote: introduce remote.<name>.serverOption configuration
  transport.c::handshake: make use of server options from remote
  fetch: respect --server-option when fetching multiple remotes
  ls-remote: leakfix for not clearing server_options

 Documentation/config/remote.txt |  10 +++
 Documentation/fetch-options.txt |   3 +
 Documentation/git-clone.txt     |   3 +
 Documentation/git-ls-remote.txt |   3 +
 builtin/fetch.c                 |   2 +
 builtin/ls-remote.c             |   1 +
 builtin/push.c                  |   9 +--
 remote.c                        |   7 ++
 remote.h                        |   3 +
 t/t5702-protocol-v2.sh          | 133 ++++++++++++++++++++++++++++++++
 transport.c                     |  15 ++++
 transport.h                     |   4 +
 12 files changed, 185 insertions(+), 8 deletions(-)


base-commit: 4590f2e9412378c61eac95966709c78766d326ba
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1776%2Fblanet%2Fxx%2Fadd-server-option-from-config-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1776/blanet/xx/add-server-option-from-config-v2
Pull-Request: https://github.com/git/git/pull/1776

Range-diff vs v1:

 1:  5c8f3c166a5 ! 1:  c95ed5e0dd5 transport: add parse_transport_option() method
     @@ Metadata
      Author: Xing Xin <xingxin.xx@bytedance.com>
      
       ## Commit message ##
     -    transport: add parse_transport_option() method
     +    transport: introduce parse_transport_option() method
      
     -    Introduce the `parse_transport_option()` method used to parse
     -    `push.pushOption` configuration values. This method will be further
     -    utilized in subsequent commits to parse a newly added
     -    `fetch.serverOption` configuration for fetches, which aligns with the
     -    design pattern of `push.pushOption`.
     +    Add the `parse_transport_option()` method to parse the `push.pushOption`
     +    configuration. This method will also be used in the next commit to
     +    handle the new `remote.<name>.serverOption` configuration for setting
     +    server options in Git protocol v2.
      
          Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
      
       ## builtin/push.c ##
      @@ builtin/push.c: static int git_push_config(const char *k, const char *v,
     + 			RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
     + 		recurse_submodules = val;
       	} else if (!strcmp(k, "push.pushoption")) {
     - 		if (!v)
     - 			return config_error_nonbool(k);
     +-		if (!v)
     +-			return config_error_nonbool(k);
      -		else
      -			if (!*v)
      -				string_list_clear(&push_options_config, 0);
      -			else
      -				string_list_append(&push_options_config, v);
     -+		parse_transport_option(v, &push_options_config);
     - 		return 0;
     +-		return 0;
     ++		return parse_transport_option(k, v, &push_options_config);
       	} else if (!strcmp(k, "color.push")) {
       		push_use_color = git_config_colorbool(k, v);
     + 		return 0;
      
       ## transport.c ##
      @@ transport.c: int is_transport_allowed(const char *type, int from_user)
       	BUG("invalid protocol_allow_config type");
       }
       
     -+void parse_transport_option(const char *option, struct string_list *transport_options)
     ++int parse_transport_option(const char *var, const char *value,
     ++			   struct string_list *transport_options)
      +{
     -+	if (!*option)
     ++	if (!value)
     ++		return config_error_nonbool(var);
     ++	if (!*value)
      +		string_list_clear(transport_options, 0);
      +	else
     -+		string_list_append(transport_options, option);
     ++		string_list_append(transport_options, value);
     ++	return 0;
      +}
      +
       void transport_check_allowed(const char *type)
     @@ transport.h: void transport_print_push_status(const char *dest, struct ref *refs
       /* common method used by transport-helper.c and send-pack.c */
       void reject_atomic_push(struct ref *refs, int mirror_mode);
       
     -+/* common method to parse push-option for pushes or server-option for fetches */
     -+void parse_transport_option(const char *option, struct string_list *transport_options);
     ++/* common method to parse push-option or server-option from config */
     ++int parse_transport_option(const char *var, const char *value,
     ++			   struct string_list *transport_options);
      +
       #endif
 2:  7e2d5c504d7 < -:  ----------- builtin/fetch.c: add fetch.serverOption configuration
 -:  ----------- > 2:  2474b4c69d6 remote: introduce remote.<name>.serverOption configuration
 3:  7c3ebda513d ! 3:  a7f3e458501 builtin/clone.c: recognize fetch.serverOption configuration
     @@ Metadata
      Author: Xing Xin <xingxin.xx@bytedance.com>
      
       ## Commit message ##
     -    builtin/clone.c: recognize fetch.serverOption configuration
     +    transport.c::handshake: make use of server options from remote
      
     -    Teach git-clone to recognize the `fetch.serverOption` configuration as a
     -    default list of server options to send for Git protocol v2, if server
     -    options are not explicitly set via the command line.
     +    Utilize the `server_options` from the corresponding remote during the
     +    handshake in `transport.c` when Git protocol v2 is detected. This helps
     +    initialize the `server_options` in `transport.h:transport` if no server
     +    options are set for the transport (typically via `--server-option` or
     +    `-o`).
      
     -    Note that `builtin/clone.c:cmd_clone` originally read the git config
     -    twice via `builtin/clone.c:git_clone_config`, which would duplicate
     -    server options if parsing logic were added there. Upon investigation, it
     -    was found that the first config read is unnecessary since all the global
     -    variables it sets are actually used after the second config read.
     -    Therefore, the first config read is replaced with a simple
     -    `config.c:git_default_config`.
     +    While another potential place to incorporate server options from the
     +    remote is in `transport.c:transport_get`, setting server options for a
     +    transport using a protocol other than v2 could lead to unexpected errors
     +    (see `transport.c:die_if_server_options`).
      
     -    Tests and documentation have been updated accordingly.
     +    Relevant tests and documentation have been updated accordingly.
      
          Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
      
     + ## Documentation/fetch-options.txt ##
     +@@ Documentation/fetch-options.txt: endif::git-pull[]
     + 	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 `remote.<name>.serverOption`
     ++	are used instead.
     + 
     + --show-forced-updates::
     + 	By default, git checks if a branch is force-updated during
     +
       ## Documentation/git-clone.txt ##
      @@ Documentation/git-clone.txt: 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`
     ++	line, the values of configuration variable `remote.<name>.serverOption`
      +	are used instead.
       
       `-n`::
       `--no-checkout`::
      
     - ## builtin/clone.c ##
     -@@ builtin/clone.c: static struct string_list option_recurse_submodules = STRING_LIST_INIT_NODUP;
     - static struct list_objects_filter_options filter_options = LIST_OBJECTS_FILTER_INIT;
     - static int option_filter_submodules = -1;    /* unspecified */
     - static int config_filter_submodules = -1;    /* unspecified */
     --static struct string_list server_options = STRING_LIST_INIT_NODUP;
     -+static struct string_list config_server_options = STRING_LIST_INIT_DUP;
     -+static struct string_list option_server_options = STRING_LIST_INIT_DUP;
     - static int option_remote_submodules;
     - static const char *bundle_uri;
     - 
     -@@ builtin/clone.c: static struct option builtin_clone_options[] = {
     - 		   N_("specify the reference format to use")),
     - 	OPT_STRING_LIST('c', "config", &option_config, N_("key=value"),
     - 			N_("set config inside the new repository")),
     --	OPT_STRING_LIST(0, "server-option", &server_options,
     -+	OPT_STRING_LIST(0, "server-option", &option_server_options,
     - 			N_("server-specific"), N_("option to transmit")),
     - 	OPT_IPVERSION(&family),
     - 	OPT_PARSE_LIST_OBJECTS_FILTER(&filter_options),
     -@@ builtin/clone.c: 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);
     - }
     -@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     - 	int hash_algo;
     - 	enum ref_storage_format ref_storage_format = REF_STORAGE_FORMAT_UNKNOWN;
     - 	const int do_not_override_repo_unix_permissions = -1;
     --
     -+	struct string_list *server_options = NULL;
     - 	struct transport_ls_refs_options transport_ls_refs_options =
     - 		TRANSPORT_LS_REFS_OPTIONS_INIT;
     - 
     - 	packet_trace_identity("clone");
     - 
     --	git_config(git_clone_config, NULL);
     -+	git_config(git_default_config, NULL);
     + ## Documentation/git-ls-remote.txt ##
     +@@ Documentation/git-ls-remote.txt: OPTIONS
     + 	character.
     + 	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 `remote.<name>.serverOption`
     ++	are used instead.
       
     - 	argc = parse_options(argc, argv, prefix, builtin_clone_options,
     - 			     builtin_clone_usage, 0);
     + <repository>::
     + 	The "remote" repository to query.  This parameter can be
     +
     + ## t/t5702-protocol-v2.sh ##
     +@@ t/t5702-protocol-v2.sh: test_expect_success 'server-options are sent when using ls-remote' '
     + 	grep "server-option=world" log
     + '
       
     -+	server_options = option_server_options.nr ?
     -+			 &option_server_options : &config_server_options;
     -+
     - 	if (argc > 2)
     - 		usage_msg_opt(_("Too many arguments."),
     - 			builtin_clone_usage, builtin_clone_options);
     -@@ builtin/clone.c: int cmd_clone(int argc, const char **argv, const char *prefix)
     - 		transport_set_option(transport, TRANS_OPT_UPLOADPACK,
     - 				     option_upload_pack);
     ++test_expect_success 'server-options from configuration are used by ls-remote' '
     ++	test_when_finished "rm -rf log myclone" &&
     ++	git clone "file://$(pwd)/file_parent" myclone &&
     ++	cat >expect <<-EOF &&
     ++	$(git -C file_parent rev-parse refs/heads/main)$(printf "\t")refs/heads/main
     ++	EOF
     ++
     ++	# Default server options from configuration are used
     ++	git -C myclone config --add remote.origin.serverOption foo &&
     ++	git -C myclone config --add remote.origin.serverOption bar &&
     ++	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
     ++		ls-remote origin main >actual &&
     ++	test_cmp expect actual &&
     ++	test_grep "ls-remote> server-option=foo" log &&
     ++	test_grep "ls-remote> server-option=bar" log &&
     ++	rm -f log &&
     ++
     ++	# Empty value of remote.<name>.serverOption clears the list
     ++	git -C myclone config --add remote.origin.serverOption "" &&
     ++	git -C myclone config --add remote.origin.serverOption tar &&
     ++	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
     ++		ls-remote origin main >actual &&
     ++	test_cmp expect actual &&
     ++	test_grep "ls-remote> server-option=tar" log &&
     ++	test_grep ! "ls-remote> server-option=foo" log &&
     ++	test_grep ! "ls-remote> server-option=bar" log &&
     ++	rm -f log &&
     ++
     ++	# Server option from command line overrides those from configuration
     ++	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
     ++		ls-remote -o hello -o world origin main >actual &&
     ++	test_cmp expect actual &&
     ++	test_grep "ls-remote> server-option=hello" log &&
     ++	test_grep "ls-remote> server-option=world" log &&
     ++	test_grep ! "ls-remote> server-option=tar" log
     ++'
     ++
     + test_expect_success 'warn if using server-option with ls-remote with legacy protocol' '
     + 	test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
     + 		ls-remote -o hello -o world "file://$(pwd)/file_parent" main 2>err &&
     +@@ t/t5702-protocol-v2.sh: test_expect_success 'server-options are sent when fetching' '
     + 	grep "server-option=world" log
     + '
       
     --	if (server_options.nr)
     --		transport->server_options = &server_options;
     -+	if (server_options && server_options->nr)
     -+		transport->server_options = server_options;
     ++test_expect_success 'server-options from configuration are used by git-fetch' '
     ++	test_when_finished "rm -rf log myclone" &&
     ++	git clone "file://$(pwd)/file_parent" myclone &&
     ++	git -C file_parent log -1 --format=%s >expect &&
     ++
     ++	# Default server options from configuration are used
     ++	git -C myclone config --add remote.origin.serverOption foo &&
     ++	git -C myclone config --add remote.origin.serverOption bar &&
     ++	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
     ++		fetch origin main &&
     ++	git -C myclone log -1 --format=%s origin/main >actual &&
     ++	test_cmp expect actual &&
     ++	test_grep "fetch> server-option=foo" log &&
     ++	test_grep "fetch> server-option=bar" log &&
     ++	rm -f log &&
     ++
     ++	# Empty value of remote.<name>.serverOption clears the list
     ++	git -C myclone config --add remote.origin.serverOption "" &&
     ++	git -C myclone config --add remote.origin.serverOption tar &&
     ++	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
     ++		fetch origin main &&
     ++	git -C myclone log -1 --format=%s origin/main >actual &&
     ++	test_cmp expect actual &&
     ++	test_grep "fetch> server-option=tar" log &&
     ++	test_grep ! "fetch> server-option=foo" log &&
     ++	test_grep ! "fetch> server-option=bar" log &&
     ++	rm -f log &&
     ++
     ++	# Server option from command line overrides those from configuration
     ++	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
     ++		fetch -o hello -o world origin main &&
     ++	git -C myclone log -1 --format=%s origin/main >actual &&
     ++	test_cmp expect actual &&
     ++	test_grep "fetch> server-option=hello" log &&
     ++	test_grep "fetch> server-option=world" log &&
     ++	test_grep ! "fetch> server-option=tar" log
     ++'
     ++
     + test_expect_success 'warn if using server-option with fetch with legacy protocol' '
     + 	test_when_finished "rm -rf temp_child" &&
       
     - 	if (filter_options.choice) {
     - 		const char *spec =
     -
     - ## t/t5702-protocol-v2.sh ##
     -@@ t/t5702-protocol-v2.sh: test_expect_success 'warn if using server-option with fetch with legacy protocol
     - test_expect_success 'server-options are sent when cloning' '
     - 	test_when_finished "rm -rf log myclone" &&
     +@@ t/t5702-protocol-v2.sh: test_expect_success 'server-options are sent when cloning' '
     + 	grep "server-option=world" log
     + '
       
     -+	# Specify server options from command line
     - 	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
     - 		clone --server-option=hello --server-option=world \
     - 		"file://$(pwd)/file_parent" myclone &&
     -+	test_grep "server-option=hello" log &&
     -+	test_grep "server-option=world" log &&
     ++test_expect_success 'server-options from configuration are used by git-clone' '
     ++	test_when_finished "rm -rf log myclone" &&
     ++
     ++	# Default server options from configuration are used
     ++	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
     ++		-c remote.origin.serverOption=foo -c remote.origin.serverOption=bar \
     ++		clone "file://$(pwd)/file_parent" myclone &&
     ++	test_grep "clone> server-option=foo" log &&
     ++	test_grep "clone> server-option=bar" log &&
      +	rm -rf log myclone &&
     - 
     --	grep "server-option=hello" log &&
     --	grep "server-option=world" log
     -+	# Specify server options from fetch.serverOption config
     ++
     ++	# Empty value of remote.<name>.serverOption clears the list
      +	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
     -+		-c fetch.serverOption=hello -c fetch.serverOption=world \
     ++		-c remote.origin.serverOption=foo -c remote.origin.serverOption=bar \
     ++		-c remote.origin.serverOption= -c remote.origin.serverOption=tar \
      +		clone "file://$(pwd)/file_parent" myclone &&
     -+	test_grep "server-option=hello" log &&
     -+	test_grep "server-option=world" log &&
     ++	test_grep "clone> server-option=tar" log &&
     ++	test_grep ! "clone> server-option=foo" log &&
     ++	test_grep ! "clone> server-option=bar" log &&
      +	rm -rf log myclone &&
      +
     -+	# Cmdline server options take a higher priority
     ++	# Server option from command line overrides those from configuration
      +	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
     -+		-c fetch.serverOption=hello -c fetch.serverOption=world \
     -+		clone --server-option=foo=bar \
     ++		-c remote.origin.serverOption=tar \
     ++		clone --server-option=hello --server-option=world \
      +		"file://$(pwd)/file_parent" myclone &&
     -+	test_grep ! "server-option=hello" log &&
     -+	test_grep ! "server-option=world" log &&
     -+	test_grep "server-option=foo=bar" log
     ++	test_grep "clone> server-option=hello" log &&
     ++	test_grep "clone> server-option=world" log &&
     ++	test_grep ! "clone> server-option=tar" log
     ++'
     ++
     + test_expect_success 'warn if using server-option with clone with legacy protocol' '
     + 	test_when_finished "rm -rf myclone" &&
     + 
     +@@ t/t5702-protocol-v2.sh: test_expect_success 'warn if using server-option with clone with legacy protocol
     + 	test_grep "server options require protocol version 2 or later" err
       '
       
     - test_expect_success 'warn if using server-option with clone with legacy protocol' '
     ++test_expect_success 'server-option configuration with legacy protocol is ok' '
     ++	test_when_finished "rm -rf myclone" &&
     ++
     ++	env GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
     ++		-c remote.origin.serverOption=foo -c remote.origin.serverOption=bar \
     ++		clone "file://$(pwd)/file_parent" myclone
     ++'
     ++
     ++test_expect_success 'invalid server-option configuration' '
     ++	test_when_finished "rm -rf myclone" &&
     ++
     ++	test_must_fail git -c protocol.version=2 \
     ++		-c remote.origin.serverOption \
     ++		clone "file://$(pwd)/file_parent" myclone 2>err &&
     ++	test_grep "error: missing value for '\''remote.origin.serveroption'\''" err
     ++'
     ++
     + test_expect_success 'upload-pack respects config using protocol v2' '
     + 	git init server &&
     + 	write_script server/.git/hook <<-\EOF &&
     +
     + ## transport.c ##
     +@@ transport.c: static struct ref *handshake(struct transport *transport, int for_push,
     + 	data->version = discover_version(&reader);
     + 	switch (data->version) {
     + 	case protocol_v2:
     ++		if ((!transport->server_options || !transport->server_options->nr) &&
     ++		    transport->remote->server_options.nr)
     ++			transport->server_options = &transport->remote->server_options;
     + 		if (server_feature_v2("session-id", &server_sid))
     + 			trace2_data_string("transfer", NULL, "server-sid", server_sid);
     + 		if (must_list_refs)
 4:  8962031f999 < -:  ----------- builtin/ls-remote.c: recognize fetch.serverOption configuration
 -:  ----------- > 4:  39ee8dbef78 fetch: respect --server-option when fetching multiple remotes
 -:  ----------- > 5:  39c07a6c8ee ls-remote: leakfix for not clearing server_options

-- 
gitgitgadget

[PATCH v2 1/5] transport: introduce parse_transport_option() method

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Add the `parse_transport_option()` method to parse the `push.pushOption`
configuration. This method will also be used in the next commit to
handle the new `remote.<name>.serverOption` configuration for setting
server options in Git protocol v2.

Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 builtin/push.c |  9 +--------
 transport.c    | 12 ++++++++++++
 transport.h    |  4 ++++
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 7a67398124f..290a08f6ef2 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -517,14 +517,7 @@ static int git_push_config(const char *k, const char *v,
 			RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
 		recurse_submodules = val;
 	} else if (!strcmp(k, "push.pushoption")) {
-		if (!v)
-			return config_error_nonbool(k);
-		else
-			if (!*v)
-				string_list_clear(&push_options_config, 0);
-			else
-				string_list_append(&push_options_config, v);
-		return 0;
+		return parse_transport_option(k, v, &push_options_config);
 	} else if (!strcmp(k, "color.push")) {
 		push_use_color = git_config_colorbool(k, v);
 		return 0;
diff --git a/transport.c b/transport.c
index bab28965f96..c8947415eec 100644
--- a/transport.c
+++ b/transport.c
@@ -1091,6 +1091,18 @@ int is_transport_allowed(const char *type, int from_user)
 	BUG("invalid protocol_allow_config type");
 }
 
+int parse_transport_option(const char *var, const char *value,
+			   struct string_list *transport_options)
+{
+	if (!value)
+		return config_error_nonbool(var);
+	if (!*value)
+		string_list_clear(transport_options, 0);
+	else
+		string_list_append(transport_options, value);
+	return 0;
+}
+
 void transport_check_allowed(const char *type)
 {
 	if (!is_transport_allowed(type, -1))
diff --git a/transport.h b/transport.h
index 6393cd9823c..44100fa9b7f 100644
--- a/transport.h
+++ b/transport.h
@@ -342,4 +342,8 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 /* common method used by transport-helper.c and send-pack.c */
 void reject_atomic_push(struct ref *refs, int mirror_mode);
 
+/* common method to parse push-option or server-option from config */
+int parse_transport_option(const char *var, const char *value,
+			   struct string_list *transport_options);
+
 #endif
-- 
gitgitgadget

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

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Currently, server options for Git protocol v2 can only be specified via
the command line option "--server-option" or "-o", which is inconvenient
when users want to specify a list of default options to send. Therefore,
we are introducing a new configuration to hold a list of default server
options, akin to the `push.pushOption` configuration for push options.

Initially, I named the new configuration `fetch.serverOption` to align
with `push.pushOption`. However, after discussing with Patrick, it was
renamed to `remote.<name>.serverOption` as suggested, because:

1. Server options are designed to be server-specific, making it more
   logical to use a per-remote configuration.
2. Using "fetch." prefixed configurations in git-clone or git-ls-remote
   seems out of place and inconsistent in design.

The parsing logic for `remote.<name>.serverOption` also relies on
`transport.c:parse_transport_option`, similar to `push.pushOption`, and
they follow the same priority design:

1. Server options set in lower-priority configuration files (e.g.,
   /etc/gitconfig or $HOME/.gitconfig) can be overridden or unset in
   more specific repository configurations using an empty string.
2. Command-line specified server options take precedence over those from
   the configuration.

Server options from configuration are stored to the corresponding
`remote.h:remote` as a new field `server_options`.  The field will be
utilized in the subsequent commit to help initialize the
`server_options` of `transport.h:transport`.

And documentation have been updated accordingly.

Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Junio C Hamano <gitster@pobox.com>
Reported-by: Liu Zhongbo <liuzhongbo.6666@bytedance.com>
Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 Documentation/config/remote.txt | 10 ++++++++++
 remote.c                        |  7 +++++++
 remote.h                        |  3 +++
 3 files changed, 20 insertions(+)

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
+	server options for this remote.
++
+This is a multi-valued variable, and an empty value can be used in a higher
+priority configuration file (e.g. `.git/config` in a repository) to clear
+the values inherited from a lower priority configuration files (e.g.
+`$HOME/.gitconfig`).
diff --git a/remote.c b/remote.c
index 7d5b8f750d8..fe11ead27ad 100644
--- a/remote.c
+++ b/remote.c
@@ -24,6 +24,7 @@
 #include "advice.h"
 #include "connect.h"
 #include "parse-options.h"
+#include "transport.h"
 
 enum map_direction { FROM_SRC, FROM_DST };
 
@@ -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;
 
 	ALLOC_GROW(remote_state->remotes, remote_state->remotes_nr + 1,
 		   remote_state->remotes_alloc);
@@ -166,6 +169,7 @@ static void remote_clear(struct remote *remote)
 	free((char *)remote->uploadpack);
 	FREE_AND_NULL(remote->http_proxy);
 	FREE_AND_NULL(remote->http_proxy_authmethod);
+	string_list_clear(&remote->server_options, 0);
 }
 
 static void add_merge(struct branch *branch, const char *name)
@@ -482,6 +486,9 @@ static int handle_config(const char *key, const char *value,
 					 key, value);
 	} else if (!strcmp(subkey, "vcs")) {
 		return git_config_string(&remote->foreign_vcs, key, value);
+	} else if (!strcmp(subkey, "serveroption")) {
+		return parse_transport_option(key, value,
+					      &remote->server_options);
 	}
 	return 0;
 }
diff --git a/remote.h b/remote.h
index b901b56746d..d9926a76d23 100644
--- a/remote.h
+++ b/remote.h
@@ -4,6 +4,7 @@
 #include "hash.h"
 #include "hashmap.h"
 #include "refspec.h"
+#include "string-list.h"
 #include "strvec.h"
 
 struct option;
@@ -104,6 +105,8 @@ struct remote {
 
 	/* The method used for authenticating against `http_proxy`. */
 	char *http_proxy_authmethod;
+
+	struct string_list server_options;
 };
 
 /**
-- 
gitgitgadget

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

[Patrick Steinhardt]

On Mon, Sep 23, 2024 at 12:17:55PM +0000, Xing Xin via GitGitGadget wrote:
> 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.

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

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

[Xing Xin]

At 2024-10-07 16:22:31, "Patrick Steinhardt" <ps@pks.im> wrote:
>On Mon, Sep 23, 2024 at 12:17:55PM +0000, Xing Xin via GitGitGadget wrote:
>> 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.

Thanks for the help! Applied to the new patch (with some minor modifications).

>> @@ -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.

Make sense.

Xing Xin

[PATCH v2 3/5] transport.c::handshake: make use of server options from remote

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Utilize the `server_options` from the corresponding remote during the
handshake in `transport.c` when Git protocol v2 is detected. This helps
initialize the `server_options` in `transport.h:transport` if no server
options are set for the transport (typically via `--server-option` or
`-o`).

While another potential place to incorporate server options from the
remote is in `transport.c:transport_get`, setting server options for a
transport using a protocol other than v2 could lead to unexpected errors
(see `transport.c:die_if_server_options`).

Relevant tests and documentation have been updated accordingly.

Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 Documentation/fetch-options.txt |   3 +
 Documentation/git-clone.txt     |   3 +
 Documentation/git-ls-remote.txt |   3 +
 t/t5702-protocol-v2.sh          | 123 ++++++++++++++++++++++++++++++++
 transport.c                     |   3 +
 5 files changed, 135 insertions(+)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index e22b217fba9..424cdeafcdd 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -304,6 +304,9 @@ endif::git-pull[]
 	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 `remote.<name>.serverOption`
+	are used instead.
 
 --show-forced-updates::
 	By default, git checks if a branch is force-updated during
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 8e925db7e9c..409fb3a3af6 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 `remote.<name>.serverOption`
+	are used instead.
 
 `-n`::
 `--no-checkout`::
diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 76c86c3ce4f..d71c4ab3e29 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -81,6 +81,9 @@ OPTIONS
 	character.
 	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 `remote.<name>.serverOption`
+	are used instead.
 
 <repository>::
 	The "remote" repository to query.  This parameter can be
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 1ef540f73d3..5cec2061d28 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -185,6 +185,43 @@ test_expect_success 'server-options are sent when using ls-remote' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'server-options from configuration are used by ls-remote' '
+	test_when_finished "rm -rf log myclone" &&
+	git clone "file://$(pwd)/file_parent" myclone &&
+	cat >expect <<-EOF &&
+	$(git -C file_parent rev-parse refs/heads/main)$(printf "\t")refs/heads/main
+	EOF
+
+	# Default server options from configuration are used
+	git -C myclone config --add remote.origin.serverOption foo &&
+	git -C myclone config --add remote.origin.serverOption bar &&
+	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
+		ls-remote origin main >actual &&
+	test_cmp expect actual &&
+	test_grep "ls-remote> server-option=foo" log &&
+	test_grep "ls-remote> server-option=bar" log &&
+	rm -f log &&
+
+	# Empty value of remote.<name>.serverOption clears the list
+	git -C myclone config --add remote.origin.serverOption "" &&
+	git -C myclone config --add remote.origin.serverOption tar &&
+	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
+		ls-remote origin main >actual &&
+	test_cmp expect actual &&
+	test_grep "ls-remote> server-option=tar" log &&
+	test_grep ! "ls-remote> server-option=foo" log &&
+	test_grep ! "ls-remote> server-option=bar" log &&
+	rm -f log &&
+
+	# Server option from command line overrides those from configuration
+	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
+		ls-remote -o hello -o world origin main >actual &&
+	test_cmp expect actual &&
+	test_grep "ls-remote> server-option=hello" log &&
+	test_grep "ls-remote> server-option=world" log &&
+	test_grep ! "ls-remote> server-option=tar" log
+'
+
 test_expect_success 'warn if using server-option with ls-remote with legacy protocol' '
 	test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
 		ls-remote -o hello -o world "file://$(pwd)/file_parent" main 2>err &&
@@ -381,6 +418,44 @@ test_expect_success 'server-options are sent when fetching' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'server-options from configuration are used by git-fetch' '
+	test_when_finished "rm -rf log myclone" &&
+	git clone "file://$(pwd)/file_parent" myclone &&
+	git -C file_parent log -1 --format=%s >expect &&
+
+	# Default server options from configuration are used
+	git -C myclone config --add remote.origin.serverOption foo &&
+	git -C myclone config --add remote.origin.serverOption bar &&
+	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
+		fetch origin main &&
+	git -C myclone log -1 --format=%s origin/main >actual &&
+	test_cmp expect actual &&
+	test_grep "fetch> server-option=foo" log &&
+	test_grep "fetch> server-option=bar" log &&
+	rm -f log &&
+
+	# Empty value of remote.<name>.serverOption clears the list
+	git -C myclone config --add remote.origin.serverOption "" &&
+	git -C myclone config --add remote.origin.serverOption tar &&
+	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
+		fetch origin main &&
+	git -C myclone log -1 --format=%s origin/main >actual &&
+	test_cmp expect actual &&
+	test_grep "fetch> server-option=tar" log &&
+	test_grep ! "fetch> server-option=foo" log &&
+	test_grep ! "fetch> server-option=bar" log &&
+	rm -f log &&
+
+	# Server option from command line overrides those from configuration
+	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
+		fetch -o hello -o world origin main &&
+	git -C myclone log -1 --format=%s origin/main >actual &&
+	test_cmp expect actual &&
+	test_grep "fetch> server-option=hello" log &&
+	test_grep "fetch> server-option=world" log &&
+	test_grep ! "fetch> server-option=tar" log
+'
+
 test_expect_success 'warn if using server-option with fetch with legacy protocol' '
 	test_when_finished "rm -rf temp_child" &&
 
@@ -404,6 +479,37 @@ test_expect_success 'server-options are sent when cloning' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'server-options from configuration are used by git-clone' '
+	test_when_finished "rm -rf log myclone" &&
+
+	# Default server options from configuration are used
+	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+		-c remote.origin.serverOption=foo -c remote.origin.serverOption=bar \
+		clone "file://$(pwd)/file_parent" myclone &&
+	test_grep "clone> server-option=foo" log &&
+	test_grep "clone> server-option=bar" log &&
+	rm -rf log myclone &&
+
+	# Empty value of remote.<name>.serverOption clears the list
+	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+		-c remote.origin.serverOption=foo -c remote.origin.serverOption=bar \
+		-c remote.origin.serverOption= -c remote.origin.serverOption=tar \
+		clone "file://$(pwd)/file_parent" myclone &&
+	test_grep "clone> server-option=tar" log &&
+	test_grep ! "clone> server-option=foo" log &&
+	test_grep ! "clone> server-option=bar" log &&
+	rm -rf log myclone &&
+
+	# Server option from command line overrides those from configuration
+	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+		-c remote.origin.serverOption=tar \
+		clone --server-option=hello --server-option=world \
+		"file://$(pwd)/file_parent" myclone &&
+	test_grep "clone> server-option=hello" log &&
+	test_grep "clone> server-option=world" log &&
+	test_grep ! "clone> server-option=tar" log
+'
+
 test_expect_success 'warn if using server-option with clone with legacy protocol' '
 	test_when_finished "rm -rf myclone" &&
 
@@ -415,6 +521,23 @@ test_expect_success 'warn if using server-option with clone with legacy protocol
 	test_grep "server options require protocol version 2 or later" err
 '
 
+test_expect_success 'server-option configuration with legacy protocol is ok' '
+	test_when_finished "rm -rf myclone" &&
+
+	env GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
+		-c remote.origin.serverOption=foo -c remote.origin.serverOption=bar \
+		clone "file://$(pwd)/file_parent" myclone
+'
+
+test_expect_success 'invalid server-option configuration' '
+	test_when_finished "rm -rf myclone" &&
+
+	test_must_fail git -c protocol.version=2 \
+		-c remote.origin.serverOption \
+		clone "file://$(pwd)/file_parent" myclone 2>err &&
+	test_grep "error: missing value for '\''remote.origin.serveroption'\''" err
+'
+
 test_expect_success 'upload-pack respects config using protocol v2' '
 	git init server &&
 	write_script server/.git/hook <<-\EOF &&
diff --git a/transport.c b/transport.c
index c8947415eec..bcfd4199c87 100644
--- a/transport.c
+++ b/transport.c
@@ -332,6 +332,9 @@ static struct ref *handshake(struct transport *transport, int for_push,
 	data->version = discover_version(&reader);
 	switch (data->version) {
 	case protocol_v2:
+		if ((!transport->server_options || !transport->server_options->nr) &&
+		    transport->remote->server_options.nr)
+			transport->server_options = &transport->remote->server_options;
 		if (server_feature_v2("session-id", &server_sid))
 			trace2_data_string("transfer", NULL, "server-sid", server_sid);
 		if (must_list_refs)
-- 
gitgitgadget

[PATCH v2 4/5] fetch: respect --server-option when fetching multiple remotes

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Fix an issue where server options specified via the command line
(`--server-option` or `-o`) were not sent when fetching from multiple
remotes using Git protocol v2.

To reproduce the issue with a repository containing multiple remotes:

  GIT_TRACE_PACKET=1 git -c protocol.version=2 fetch --server-option=demo --all

Observe that no server options are sent to any remote.

The root cause was identified in `builtin/fetch.c:fetch_multiple`, which
is invoked when fetching from more than one remote. This function forks
a `git-fetch` subprocess for each remote but did not include the
specified server options in the subprocess arguments.

This commit ensures that command-line specified server options are
properly passed to each subprocess. Relevant tests have been added.

Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 builtin/fetch.c        |  2 ++
 t/t5702-protocol-v2.sh | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index c297569a473..c1b3aea7745 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1980,6 +1980,8 @@ static int fetch_multiple(struct string_list *list, int max_children,
 	strvec_pushl(&argv, "-c", "fetch.bundleURI=",
 		     "fetch", "--append", "--no-auto-gc",
 		     "--no-write-commit-graph", NULL);
+	for (i = 0; i < server_options.nr; i++)
+		strvec_pushf(&argv, "--server-option=%s", server_options.items[i].string);
 	add_options_to_argv(&argv, config);
 
 	if (max_children != 1 && list->nr != 1) {
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 5cec2061d28..d3df81e7852 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -418,6 +418,16 @@ test_expect_success 'server-options are sent when fetching' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'server-options are sent when fetch multiple remotes' '
+	test_when_finished "rm -f log server_options_sent" &&
+	git clone "file://$(pwd)/file_parent" child_multi_remotes &&
+	git -C child_multi_remotes remote add another "file://$(pwd)/file_parent" &&
+	GIT_TRACE_PACKET="$(pwd)/log" git -C child_multi_remotes -c protocol.version=2 \
+		fetch -o hello --all &&
+	grep "fetch> server-option=hello" log >server_options_sent &&
+	test_line_count = 2 server_options_sent
+'
+
 test_expect_success 'server-options from configuration are used by git-fetch' '
 	test_when_finished "rm -rf log myclone" &&
 	git clone "file://$(pwd)/file_parent" myclone &&
-- 
gitgitgadget

Re: [PATCH v2 4/5] fetch: respect --server-option when fetching multiple remotes

[Patrick Steinhardt]

On Mon, Sep 23, 2024 at 12:17:57PM +0000, Xing Xin via GitGitGadget wrote:
> From: Xing Xin <xingxin.xx@bytedance.com>
> 
> Fix an issue where server options specified via the command line
> (`--server-option` or `-o`) were not sent when fetching from multiple
> remotes using Git protocol v2.
> 
> To reproduce the issue with a repository containing multiple remotes:
> 
>   GIT_TRACE_PACKET=1 git -c protocol.version=2 fetch --server-option=demo --all
> 
> Observe that no server options are sent to any remote.
> 
> The root cause was identified in `builtin/fetch.c:fetch_multiple`, which
> is invoked when fetching from more than one remote. This function forks
> a `git-fetch` subprocess for each remote but did not include the
> specified server options in the subprocess arguments.
> 
> This commit ensures that command-line specified server options are
> properly passed to each subprocess. Relevant tests have been added.

Nice catch.

Patrick

[PATCH v2 5/5] ls-remote: leakfix for not clearing server_options

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Ensure `server_options` is properly cleared using `string_list_clear()`
in `builtin/ls-remote.c:cmd_ls_remote`.

Although we cannot yet enable `TEST_PASSES_SANITIZE_LEAK=true` for
`t/t5702-protocol-v2.sh` due to other existing leaks, this fix ensures
that "git-ls-remote" related server options tests pass the sanitize leak
check:

  ...
  ok 12 - server-options are sent when using ls-remote
  ok 13 - server-options from configuration are used by ls-remote
  ...

Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 builtin/ls-remote.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 0a491595ca8..c3fdda08409 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -169,5 +169,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
 	transport_ls_refs_options_release(&transport_options);
 
 	strvec_clear(&pattern);
+	string_list_clear(&server_options, 0);
 	return status;
 }
-- 
gitgitgadget

Re: [PATCH v2 5/5] ls-remote: leakfix for not clearing server_options

[Patrick Steinhardt]

On Mon, Sep 23, 2024 at 12:17:58PM +0000, Xing Xin via GitGitGadget wrote:
> From: Xing Xin <xingxin.xx@bytedance.com>
> 
> Ensure `server_options` is properly cleared using `string_list_clear()`
> in `builtin/ls-remote.c:cmd_ls_remote`.
> 
> Although we cannot yet enable `TEST_PASSES_SANITIZE_LEAK=true` for
> `t/t5702-protocol-v2.sh` due to other existing leaks, this fix ensures
> that "git-ls-remote" related server options tests pass the sanitize leak
> check:
> 
>   ...
>   ok 12 - server-options are sent when using ls-remote
>   ok 13 - server-options from configuration are used by ls-remote
>   ...
> 
> Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
> ---
>  builtin/ls-remote.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
> index 0a491595ca8..c3fdda08409 100644
> --- a/builtin/ls-remote.c
> +++ b/builtin/ls-remote.c
> @@ -169,5 +169,6 @@ int cmd_ls_remote(int argc, const char **argv, const char *prefix)
>  	transport_ls_refs_options_release(&transport_options);
>  
>  	strvec_clear(&pattern);
> +	string_list_clear(&server_options, 0);
>  	return status;
>  }

Indeed. I've also got a patch for this pending as part of my memory leak
series, but didn't yet send it out. So I'm happy when this lands
independently so that I have to carry one patch less.

Patrick

Re: [PATCH v2 0/5] Support server option from configuration

[Patrick Steinhardt]

On Mon, Sep 23, 2024 at 12:17:53PM +0000, blanet via GitGitGadget wrote:
> We have multiple internal repositories configured with a huge number of CI
> tasks, each requiring code preparation (either via clone or fetch). These CI
> tasks, triggered by post-receive hooks, often fetch the same code (usually
> using --depth=1) concurrently.
> 
> To reduce performance impacts on the git server caused by these tasks, we
> want to deploy a special designed pack-objects-hook. This hook would allow
> the packs generated by git-pack-objects to be reused. Since not all
> clone/fetch operations will benefit from this caching (e.g., pulls from
> developers' environments), clients need to pass a special identifier to
> indicate whether caching should be enabled for a particular operation. Using
> server options is a suitable method for this.
> 
> However, server options can only be specified via the command line option
> (via --server-option or -o), which is inconvenient and requires CI script
> modifications. A configuration-based approach is easier to propagate (by
> changing the global configuration ~/.gitconfig) and avoids issues with older
> Git versions that don't support --server-option.
> 
> This patch series introduces a new multi-valued configuration,
> remote.<name>.serverOption, similar to push.pushOption, to specify default
> server options for the corresponding remote.
> 
>  * Patch 1~3 is the main change for introducing the new configuration.
>  * Patch 4 fixes a issue for git-fetch not sending server-options when
>    fetching multiple remotes.
>  * Patch 5 is a minor fix for a server options-related memory leak.

Sorry for taking this long to get back to this patch series. I've given
it another careful read and have a single proposal for improved docs
that you may want to address. But other than those nits this version
looks great to me, thanks!

Patrick

Re:Re: [PATCH v2 0/5] Support server option from configuration

[Xing Xin]

At 2024-10-07 16:23:02, "Patrick Steinhardt" <ps@pks.im> wrote:
>On Mon, Sep 23, 2024 at 12:17:53PM +0000, blanet via GitGitGadget wrote:
>> We have multiple internal repositories configured with a huge number of CI
>> tasks, each requiring code preparation (either via clone or fetch). These CI
>> tasks, triggered by post-receive hooks, often fetch the same code (usually
>> using --depth=1) concurrently.
>> 
>> To reduce performance impacts on the git server caused by these tasks, we
>> want to deploy a special designed pack-objects-hook. This hook would allow
>> the packs generated by git-pack-objects to be reused. Since not all
>> clone/fetch operations will benefit from this caching (e.g., pulls from
>> developers' environments), clients need to pass a special identifier to
>> indicate whether caching should be enabled for a particular operation. Using
>> server options is a suitable method for this.
>> 
>> However, server options can only be specified via the command line option
>> (via --server-option or -o), which is inconvenient and requires CI script
>> modifications. A configuration-based approach is easier to propagate (by
>> changing the global configuration ~/.gitconfig) and avoids issues with older
>> Git versions that don't support --server-option.
>> 
>> This patch series introduces a new multi-valued configuration,
>> remote.<name>.serverOption, similar to push.pushOption, to specify default
>> server options for the corresponding remote.
>> 
>>  * Patch 1~3 is the main change for introducing the new configuration.
>>  * Patch 4 fixes a issue for git-fetch not sending server-options when
>>    fetching multiple remotes.
>>  * Patch 5 is a minor fix for a server options-related memory leak.
>
>Sorry for taking this long to get back to this patch series. I've given
>it another careful read and have a single proposal for improved docs
>that you may want to address. But other than those nits this version
>looks great to me, thanks!


Thanks for your kind help! I just got back from holiday, so your reply was
perfectly timed. :-)

Xing Xin

[PATCH v3 0/5] Support server option from configuration

[blanet via GitGitGadget]

We manage some internal repositories with numerous CI tasks, each requiring
code preparation through git-clone or git-fetch. These tasks, triggered by
post-receive hooks, often fetch the same copy of code concurrently using
--depth=1, causing extremely high load spikes on our Git servers.

To reduce performance impacts caused by these tasks, we plan to deploy a
specially designed pack-objects-hook [1]. This hook would allow the packs
generated by git-pack-objects(during git-clone or git-fetch) to be reused.
Since not all clone/fetch operations will benefit from this caching (e.g.,
pulls from developer environments), clients need to pass a special
identifier to indicate whether caching should be enabled. Using server
options [2] is suitable for this purpose.

However, server options can only be specified via the command line option
(via --server-option or -o), which is inconvenient and requires
modifications to CI scripts. A configuration-based approach is preferable,
as it can be propagated through global configuration (e.g. ~/.gitconfig) and
avoids compatibility issues with older Git versions that don't support
--server-option.

This patch series introduces a new multi-valued configuration,
remote.<name>.serverOption, similar to push.pushOption, to specify default
server options for the corresponding remote.

 * Patches 1~3 contain the main changes for introducing the new
   configuration.
 * Patch 4 fixes a issue for git-fetch not sending server-options when
   fetching from multiple remotes.
 * Patch 5 is a minor fix for a server options-related memory leak.

 1. https://git-scm.com/docs/git-config#Documentation/git-config.txt-uploadpackpackObjectsHook
 2. https://git-scm.com/docs/gitprotocol-v2#_server_option

Xing Xin (5):
  transport: introduce parse_transport_option() method
  remote: introduce remote.<name>.serverOption configuration
  transport.c::handshake: make use of server options from remote
  fetch: respect --server-option when fetching multiple remotes
  ls-remote: leakfix for not clearing server_options

 Documentation/config/remote.txt |  10 +++
 Documentation/fetch-options.txt |   3 +
 Documentation/git-clone.txt     |   3 +
 Documentation/git-ls-remote.txt |   3 +
 builtin/fetch.c                 |   2 +
 builtin/ls-remote.c             |   1 +
 builtin/push.c                  |   9 +--
 remote.c                        |   6 ++
 remote.h                        |   3 +
 t/t5702-protocol-v2.sh          | 133 ++++++++++++++++++++++++++++++++
 transport.c                     |  15 ++++
 transport.h                     |   4 +
 12 files changed, 184 insertions(+), 8 deletions(-)


base-commit: 6258f68c3c1092c901337895c864073dcdea9213
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1776%2Fblanet%2Fxx%2Fadd-server-option-from-config-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1776/blanet/xx/add-server-option-from-config-v3
Pull-Request: https://github.com/git/git/pull/1776

Range-diff vs v2:

 1:  c95ed5e0dd5 = 1:  b44face42e1 transport: introduce parse_transport_option() method
 2:  2474b4c69d6 ! 2:  3c6b129d368 remote: introduce remote.<name>.serverOption configuration
     @@ Documentation/config/remote.txt: remote.<name>.partialclonefilter::
       	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
     -+	server options for this remote.
     ++	The default set of server options used when fetching from this remote.
     ++	These server options can be overridden by the `--server-option=` command
     ++	line arguments.
      ++
      +This is a multi-valued variable, and an empty value can be used in a higher
      +priority configuration file (e.g. `.git/config` in a repository) to clear
     @@ remote.c
       
       enum map_direction { FROM_SRC, FROM_DST };
       
     -@@ remote.c: 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);
      @@ remote.c: 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;
     ++	string_list_init_dup(&ret->server_options);
       
       	ALLOC_GROW(remote_state->remotes, remote_state->remotes_nr + 1,
       		   remote_state->remotes_alloc);
     @@ remote.c: static void remote_clear(struct remote *remote)
       
       static void add_merge(struct branch *branch, const char *name)
      @@ remote.c: static int handle_config(const char *key, const char *value,
     - 					 key, value);
       	} else if (!strcmp(subkey, "vcs")) {
     + 		FREE_AND_NULL(remote->foreign_vcs);
       		return git_config_string(&remote->foreign_vcs, key, value);
      +	} else if (!strcmp(subkey, "serveroption")) {
      +		return parse_transport_option(key, value,
 3:  a7f3e458501 = 3:  f0835259b06 transport.c::handshake: make use of server options from remote
 4:  39ee8dbef78 = 4:  420b15d9f37 fetch: respect --server-option when fetching multiple remotes
 5:  39c07a6c8ee ! 5:  2528d929c7e ls-remote: leakfix for not clearing server_options
     @@ Commit message
          Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
      
       ## builtin/ls-remote.c ##
     -@@ builtin/ls-remote.c: int cmd_ls_remote(int argc, const char **argv, const char *prefix)
     +@@ builtin/ls-remote.c: int cmd_ls_remote(int argc,
       	transport_ls_refs_options_release(&transport_options);
       
       	strvec_clear(&pattern);

-- 
gitgitgadget

[PATCH v3 1/5] transport: introduce parse_transport_option() method

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Add the `parse_transport_option()` method to parse the `push.pushOption`
configuration. This method will also be used in the next commit to
handle the new `remote.<name>.serverOption` configuration for setting
server options in Git protocol v2.

Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 builtin/push.c |  9 +--------
 transport.c    | 12 ++++++++++++
 transport.h    |  4 ++++
 3 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index e6f48969b81..e0ff60d48e5 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -519,14 +519,7 @@ static int git_push_config(const char *k, const char *v,
 			RECURSE_SUBMODULES_ON_DEMAND : RECURSE_SUBMODULES_OFF;
 		recurse_submodules = val;
 	} else if (!strcmp(k, "push.pushoption")) {
-		if (!v)
-			return config_error_nonbool(k);
-		else
-			if (!*v)
-				string_list_clear(&push_options_config, 0);
-			else
-				string_list_append(&push_options_config, v);
-		return 0;
+		return parse_transport_option(k, v, &push_options_config);
 	} else if (!strcmp(k, "color.push")) {
 		push_use_color = git_config_colorbool(k, v);
 		return 0;
diff --git a/transport.c b/transport.c
index 3c4714581f5..4d9e605b273 100644
--- a/transport.c
+++ b/transport.c
@@ -1099,6 +1099,18 @@ int is_transport_allowed(const char *type, int from_user)
 	BUG("invalid protocol_allow_config type");
 }
 
+int parse_transport_option(const char *var, const char *value,
+			   struct string_list *transport_options)
+{
+	if (!value)
+		return config_error_nonbool(var);
+	if (!*value)
+		string_list_clear(transport_options, 0);
+	else
+		string_list_append(transport_options, value);
+	return 0;
+}
+
 void transport_check_allowed(const char *type)
 {
 	if (!is_transport_allowed(type, -1))
diff --git a/transport.h b/transport.h
index 6393cd9823c..44100fa9b7f 100644
--- a/transport.h
+++ b/transport.h
@@ -342,4 +342,8 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 /* common method used by transport-helper.c and send-pack.c */
 void reject_atomic_push(struct ref *refs, int mirror_mode);
 
+/* common method to parse push-option or server-option from config */
+int parse_transport_option(const char *var, const char *value,
+			   struct string_list *transport_options);
+
 #endif
-- 
gitgitgadget

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

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Currently, server options for Git protocol v2 can only be specified via
the command line option "--server-option" or "-o", which is inconvenient
when users want to specify a list of default options to send. Therefore,
we are introducing a new configuration to hold a list of default server
options, akin to the `push.pushOption` configuration for push options.

Initially, I named the new configuration `fetch.serverOption` to align
with `push.pushOption`. However, after discussing with Patrick, it was
renamed to `remote.<name>.serverOption` as suggested, because:

1. Server options are designed to be server-specific, making it more
   logical to use a per-remote configuration.
2. Using "fetch." prefixed configurations in git-clone or git-ls-remote
   seems out of place and inconsistent in design.

The parsing logic for `remote.<name>.serverOption` also relies on
`transport.c:parse_transport_option`, similar to `push.pushOption`, and
they follow the same priority design:

1. Server options set in lower-priority configuration files (e.g.,
   /etc/gitconfig or $HOME/.gitconfig) can be overridden or unset in
   more specific repository configurations using an empty string.
2. Command-line specified server options take precedence over those from
   the configuration.

Server options from configuration are stored to the corresponding
`remote.h:remote` as a new field `server_options`.  The field will be
utilized in the subsequent commit to help initialize the
`server_options` of `transport.h:transport`.

And documentation have been updated accordingly.

Helped-by: Patrick Steinhardt <ps@pks.im>
Helped-by: Junio C Hamano <gitster@pobox.com>
Reported-by: Liu Zhongbo <liuzhongbo.6666@bytedance.com>
Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 Documentation/config/remote.txt | 10 ++++++++++
 remote.c                        |  6 ++++++
 remote.h                        |  3 +++
 3 files changed, 19 insertions(+)

diff --git a/Documentation/config/remote.txt b/Documentation/config/remote.txt
index 36e771556c6..b194dff07b9 100644
--- a/Documentation/config/remote.txt
+++ b/Documentation/config/remote.txt
@@ -96,3 +96,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::
+	The default set of server options used when fetching from this remote.
+	These server options can be overridden by the `--server-option=` command
+	line arguments.
++
+This is a multi-valued variable, and an empty value can be used in a higher
+priority configuration file (e.g. `.git/config` in a repository) to clear
+the values inherited from a lower priority configuration files (e.g.
+`$HOME/.gitconfig`).
diff --git a/remote.c b/remote.c
index 390a03c2643..fa65549aa44 100644
--- a/remote.c
+++ b/remote.c
@@ -24,6 +24,7 @@
 #include "advice.h"
 #include "connect.h"
 #include "parse-options.h"
+#include "transport.h"
 
 enum map_direction { FROM_SRC, FROM_DST };
 
@@ -143,6 +144,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);
+	string_list_init_dup(&ret->server_options);
 
 	ALLOC_GROW(remote_state->remotes, remote_state->remotes_nr + 1,
 		   remote_state->remotes_alloc);
@@ -166,6 +168,7 @@ static void remote_clear(struct remote *remote)
 	free((char *)remote->uploadpack);
 	FREE_AND_NULL(remote->http_proxy);
 	FREE_AND_NULL(remote->http_proxy_authmethod);
+	string_list_clear(&remote->server_options, 0);
 }
 
 static void add_merge(struct branch *branch, const char *name)
@@ -508,6 +511,9 @@ static int handle_config(const char *key, const char *value,
 	} else if (!strcmp(subkey, "vcs")) {
 		FREE_AND_NULL(remote->foreign_vcs);
 		return git_config_string(&remote->foreign_vcs, key, value);
+	} else if (!strcmp(subkey, "serveroption")) {
+		return parse_transport_option(key, value,
+					      &remote->server_options);
 	}
 	return 0;
 }
diff --git a/remote.h b/remote.h
index a58713f20ad..e95d30946c0 100644
--- a/remote.h
+++ b/remote.h
@@ -4,6 +4,7 @@
 #include "hash.h"
 #include "hashmap.h"
 #include "refspec.h"
+#include "string-list.h"
 #include "strvec.h"
 
 struct option;
@@ -104,6 +105,8 @@ struct remote {
 
 	/* The method used for authenticating against `http_proxy`. */
 	char *http_proxy_authmethod;
+
+	struct string_list server_options;
 };
 
 /**
-- 
gitgitgadget

[PATCH v3 3/5] transport.c::handshake: make use of server options from remote

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Utilize the `server_options` from the corresponding remote during the
handshake in `transport.c` when Git protocol v2 is detected. This helps
initialize the `server_options` in `transport.h:transport` if no server
options are set for the transport (typically via `--server-option` or
`-o`).

While another potential place to incorporate server options from the
remote is in `transport.c:transport_get`, setting server options for a
transport using a protocol other than v2 could lead to unexpected errors
(see `transport.c:die_if_server_options`).

Relevant tests and documentation have been updated accordingly.

Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 Documentation/fetch-options.txt |   3 +
 Documentation/git-clone.txt     |   3 +
 Documentation/git-ls-remote.txt |   3 +
 t/t5702-protocol-v2.sh          | 123 ++++++++++++++++++++++++++++++++
 transport.c                     |   3 +
 5 files changed, 135 insertions(+)

diff --git a/Documentation/fetch-options.txt b/Documentation/fetch-options.txt
index 80838fe37ef..9dc7ac8dbdc 100644
--- a/Documentation/fetch-options.txt
+++ b/Documentation/fetch-options.txt
@@ -305,6 +305,9 @@ endif::git-pull[]
 	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 `remote.<name>.serverOption`
+	are used instead.
 
 --show-forced-updates::
 	By default, git checks if a branch is force-updated during
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index 8e925db7e9c..409fb3a3af6 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 `remote.<name>.serverOption`
+	are used instead.
 
 `-n`::
 `--no-checkout`::
diff --git a/Documentation/git-ls-remote.txt b/Documentation/git-ls-remote.txt
index 76c86c3ce4f..d71c4ab3e29 100644
--- a/Documentation/git-ls-remote.txt
+++ b/Documentation/git-ls-remote.txt
@@ -81,6 +81,9 @@ OPTIONS
 	character.
 	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 `remote.<name>.serverOption`
+	are used instead.
 
 <repository>::
 	The "remote" repository to query.  This parameter can be
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 1ef540f73d3..5cec2061d28 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -185,6 +185,43 @@ test_expect_success 'server-options are sent when using ls-remote' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'server-options from configuration are used by ls-remote' '
+	test_when_finished "rm -rf log myclone" &&
+	git clone "file://$(pwd)/file_parent" myclone &&
+	cat >expect <<-EOF &&
+	$(git -C file_parent rev-parse refs/heads/main)$(printf "\t")refs/heads/main
+	EOF
+
+	# Default server options from configuration are used
+	git -C myclone config --add remote.origin.serverOption foo &&
+	git -C myclone config --add remote.origin.serverOption bar &&
+	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
+		ls-remote origin main >actual &&
+	test_cmp expect actual &&
+	test_grep "ls-remote> server-option=foo" log &&
+	test_grep "ls-remote> server-option=bar" log &&
+	rm -f log &&
+
+	# Empty value of remote.<name>.serverOption clears the list
+	git -C myclone config --add remote.origin.serverOption "" &&
+	git -C myclone config --add remote.origin.serverOption tar &&
+	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
+		ls-remote origin main >actual &&
+	test_cmp expect actual &&
+	test_grep "ls-remote> server-option=tar" log &&
+	test_grep ! "ls-remote> server-option=foo" log &&
+	test_grep ! "ls-remote> server-option=bar" log &&
+	rm -f log &&
+
+	# Server option from command line overrides those from configuration
+	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
+		ls-remote -o hello -o world origin main >actual &&
+	test_cmp expect actual &&
+	test_grep "ls-remote> server-option=hello" log &&
+	test_grep "ls-remote> server-option=world" log &&
+	test_grep ! "ls-remote> server-option=tar" log
+'
+
 test_expect_success 'warn if using server-option with ls-remote with legacy protocol' '
 	test_must_fail env GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
 		ls-remote -o hello -o world "file://$(pwd)/file_parent" main 2>err &&
@@ -381,6 +418,44 @@ test_expect_success 'server-options are sent when fetching' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'server-options from configuration are used by git-fetch' '
+	test_when_finished "rm -rf log myclone" &&
+	git clone "file://$(pwd)/file_parent" myclone &&
+	git -C file_parent log -1 --format=%s >expect &&
+
+	# Default server options from configuration are used
+	git -C myclone config --add remote.origin.serverOption foo &&
+	git -C myclone config --add remote.origin.serverOption bar &&
+	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
+		fetch origin main &&
+	git -C myclone log -1 --format=%s origin/main >actual &&
+	test_cmp expect actual &&
+	test_grep "fetch> server-option=foo" log &&
+	test_grep "fetch> server-option=bar" log &&
+	rm -f log &&
+
+	# Empty value of remote.<name>.serverOption clears the list
+	git -C myclone config --add remote.origin.serverOption "" &&
+	git -C myclone config --add remote.origin.serverOption tar &&
+	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
+		fetch origin main &&
+	git -C myclone log -1 --format=%s origin/main >actual &&
+	test_cmp expect actual &&
+	test_grep "fetch> server-option=tar" log &&
+	test_grep ! "fetch> server-option=foo" log &&
+	test_grep ! "fetch> server-option=bar" log &&
+	rm -f log &&
+
+	# Server option from command line overrides those from configuration
+	GIT_TRACE_PACKET="$(pwd)/log" git -C myclone -c protocol.version=2 \
+		fetch -o hello -o world origin main &&
+	git -C myclone log -1 --format=%s origin/main >actual &&
+	test_cmp expect actual &&
+	test_grep "fetch> server-option=hello" log &&
+	test_grep "fetch> server-option=world" log &&
+	test_grep ! "fetch> server-option=tar" log
+'
+
 test_expect_success 'warn if using server-option with fetch with legacy protocol' '
 	test_when_finished "rm -rf temp_child" &&
 
@@ -404,6 +479,37 @@ test_expect_success 'server-options are sent when cloning' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'server-options from configuration are used by git-clone' '
+	test_when_finished "rm -rf log myclone" &&
+
+	# Default server options from configuration are used
+	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+		-c remote.origin.serverOption=foo -c remote.origin.serverOption=bar \
+		clone "file://$(pwd)/file_parent" myclone &&
+	test_grep "clone> server-option=foo" log &&
+	test_grep "clone> server-option=bar" log &&
+	rm -rf log myclone &&
+
+	# Empty value of remote.<name>.serverOption clears the list
+	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+		-c remote.origin.serverOption=foo -c remote.origin.serverOption=bar \
+		-c remote.origin.serverOption= -c remote.origin.serverOption=tar \
+		clone "file://$(pwd)/file_parent" myclone &&
+	test_grep "clone> server-option=tar" log &&
+	test_grep ! "clone> server-option=foo" log &&
+	test_grep ! "clone> server-option=bar" log &&
+	rm -rf log myclone &&
+
+	# Server option from command line overrides those from configuration
+	GIT_TRACE_PACKET="$(pwd)/log" git -c protocol.version=2 \
+		-c remote.origin.serverOption=tar \
+		clone --server-option=hello --server-option=world \
+		"file://$(pwd)/file_parent" myclone &&
+	test_grep "clone> server-option=hello" log &&
+	test_grep "clone> server-option=world" log &&
+	test_grep ! "clone> server-option=tar" log
+'
+
 test_expect_success 'warn if using server-option with clone with legacy protocol' '
 	test_when_finished "rm -rf myclone" &&
 
@@ -415,6 +521,23 @@ test_expect_success 'warn if using server-option with clone with legacy protocol
 	test_grep "server options require protocol version 2 or later" err
 '
 
+test_expect_success 'server-option configuration with legacy protocol is ok' '
+	test_when_finished "rm -rf myclone" &&
+
+	env GIT_TEST_PROTOCOL_VERSION=0 git -c protocol.version=0 \
+		-c remote.origin.serverOption=foo -c remote.origin.serverOption=bar \
+		clone "file://$(pwd)/file_parent" myclone
+'
+
+test_expect_success 'invalid server-option configuration' '
+	test_when_finished "rm -rf myclone" &&
+
+	test_must_fail git -c protocol.version=2 \
+		-c remote.origin.serverOption \
+		clone "file://$(pwd)/file_parent" myclone 2>err &&
+	test_grep "error: missing value for '\''remote.origin.serveroption'\''" err
+'
+
 test_expect_success 'upload-pack respects config using protocol v2' '
 	git init server &&
 	write_script server/.git/hook <<-\EOF &&
diff --git a/transport.c b/transport.c
index 4d9e605b273..b416fc84567 100644
--- a/transport.c
+++ b/transport.c
@@ -334,6 +334,9 @@ static struct ref *handshake(struct transport *transport, int for_push,
 	data->version = discover_version(&reader);
 	switch (data->version) {
 	case protocol_v2:
+		if ((!transport->server_options || !transport->server_options->nr) &&
+		    transport->remote->server_options.nr)
+			transport->server_options = &transport->remote->server_options;
 		if (server_feature_v2("session-id", &server_sid))
 			trace2_data_string("transfer", NULL, "server-sid", server_sid);
 		if (must_list_refs)
-- 
gitgitgadget

[PATCH v3 4/5] fetch: respect --server-option when fetching multiple remotes

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Fix an issue where server options specified via the command line
(`--server-option` or `-o`) were not sent when fetching from multiple
remotes using Git protocol v2.

To reproduce the issue with a repository containing multiple remotes:

  GIT_TRACE_PACKET=1 git -c protocol.version=2 fetch --server-option=demo --all

Observe that no server options are sent to any remote.

The root cause was identified in `builtin/fetch.c:fetch_multiple`, which
is invoked when fetching from more than one remote. This function forks
a `git-fetch` subprocess for each remote but did not include the
specified server options in the subprocess arguments.

This commit ensures that command-line specified server options are
properly passed to each subprocess. Relevant tests have been added.

Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 builtin/fetch.c        |  2 ++
 t/t5702-protocol-v2.sh | 10 ++++++++++
 2 files changed, 12 insertions(+)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index c900f577219..54be2e7ca9f 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -1980,6 +1980,8 @@ static int fetch_multiple(struct string_list *list, int max_children,
 	strvec_pushl(&argv, "-c", "fetch.bundleURI=",
 		     "fetch", "--append", "--no-auto-gc",
 		     "--no-write-commit-graph", NULL);
+	for (i = 0; i < server_options.nr; i++)
+		strvec_pushf(&argv, "--server-option=%s", server_options.items[i].string);
 	add_options_to_argv(&argv, config);
 
 	if (max_children != 1 && list->nr != 1) {
diff --git a/t/t5702-protocol-v2.sh b/t/t5702-protocol-v2.sh
index 5cec2061d28..d3df81e7852 100755
--- a/t/t5702-protocol-v2.sh
+++ b/t/t5702-protocol-v2.sh
@@ -418,6 +418,16 @@ test_expect_success 'server-options are sent when fetching' '
 	grep "server-option=world" log
 '
 
+test_expect_success 'server-options are sent when fetch multiple remotes' '
+	test_when_finished "rm -f log server_options_sent" &&
+	git clone "file://$(pwd)/file_parent" child_multi_remotes &&
+	git -C child_multi_remotes remote add another "file://$(pwd)/file_parent" &&
+	GIT_TRACE_PACKET="$(pwd)/log" git -C child_multi_remotes -c protocol.version=2 \
+		fetch -o hello --all &&
+	grep "fetch> server-option=hello" log >server_options_sent &&
+	test_line_count = 2 server_options_sent
+'
+
 test_expect_success 'server-options from configuration are used by git-fetch' '
 	test_when_finished "rm -rf log myclone" &&
 	git clone "file://$(pwd)/file_parent" myclone &&
-- 
gitgitgadget

Re: [PATCH v3 4/5] fetch: respect --server-option when fetching multiple remotes

[Junio C Hamano]

"Xing Xin via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Xing Xin <xingxin.xx@bytedance.com>
>
> Fix an issue where server options specified via the command line
> (`--server-option` or `-o`) were not sent when fetching from multiple
> remotes using Git protocol v2.
>
> To reproduce the issue with a repository containing multiple remotes:
>
>   GIT_TRACE_PACKET=1 git -c protocol.version=2 fetch --server-option=demo --all
>
> Observe that no server options are sent to any remote.
>
> The root cause was identified in `builtin/fetch.c:fetch_multiple`, which
> is invoked when fetching from more than one remote. This function forks
> a `git-fetch` subprocess for each remote but did not include the
> specified server options in the subprocess arguments.
>
> This commit ensures that command-line specified server options are
> properly passed to each subprocess. Relevant tests have been added.

A more interesting use case, as there is no reason to expect that a
single server-option command line option may apply to all remotes,
would be to configure different server options for different remotes
via the new serverOption configuration, so that different server
options are used for different remotes.

If it happens that the same server option is applicable for all
remotes, it is reasonable to give --server-option from the command
line and expect it to be propagated down to subfetches, so
regardless of the "what happens when different remotes have
different options configured?" above, the change in this step looks
reasonable to me.

[PATCH v3 5/5] ls-remote: leakfix for not clearing server_options

[Xing Xin via GitGitGadget]

From: Xing Xin <xingxin.xx@bytedance.com>

Ensure `server_options` is properly cleared using `string_list_clear()`
in `builtin/ls-remote.c:cmd_ls_remote`.

Although we cannot yet enable `TEST_PASSES_SANITIZE_LEAK=true` for
`t/t5702-protocol-v2.sh` due to other existing leaks, this fix ensures
that "git-ls-remote" related server options tests pass the sanitize leak
check:

  ...
  ok 12 - server-options are sent when using ls-remote
  ok 13 - server-options from configuration are used by ls-remote
  ...

Signed-off-by: Xing Xin <xingxin.xx@bytedance.com>
---
 builtin/ls-remote.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index f723b3bf3bb..423318f87ec 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -173,5 +173,6 @@ int cmd_ls_remote(int argc,
 	transport_ls_refs_options_release(&transport_options);
 
 	strvec_clear(&pattern);
+	string_list_clear(&server_options, 0);
 	return status;
 }
-- 
gitgitgadget

Re: [PATCH v3 0/5] Support server option from configuration

[Patrick Steinhardt]

On Tue, Oct 08, 2024 at 03:38:14AM +0000, blanet via GitGitGadget wrote:
> We manage some internal repositories with numerous CI tasks, each requiring
> code preparation through git-clone or git-fetch. These tasks, triggered by
> post-receive hooks, often fetch the same copy of code concurrently using
> --depth=1, causing extremely high load spikes on our Git servers.
> 
> To reduce performance impacts caused by these tasks, we plan to deploy a
> specially designed pack-objects-hook [1]. This hook would allow the packs
> generated by git-pack-objects(during git-clone or git-fetch) to be reused.
> Since not all clone/fetch operations will benefit from this caching (e.g.,
> pulls from developer environments), clients need to pass a special
> identifier to indicate whether caching should be enabled. Using server
> options [2] is suitable for this purpose.
> 
> However, server options can only be specified via the command line option
> (via --server-option or -o), which is inconvenient and requires
> modifications to CI scripts. A configuration-based approach is preferable,
> as it can be propagated through global configuration (e.g. ~/.gitconfig) and
> avoids compatibility issues with older Git versions that don't support
> --server-option.
> 
> This patch series introduces a new multi-valued configuration,
> remote.<name>.serverOption, similar to push.pushOption, to specify default
> server options for the corresponding remote.
> 
>  * Patches 1~3 contain the main changes for introducing the new
>    configuration.
>  * Patch 4 fixes a issue for git-fetch not sending server-options when
>    fetching from multiple remotes.
>  * Patch 5 is a minor fix for a server options-related memory leak.
> 
>  1. https://git-scm.com/docs/git-config#Documentation/git-config.txt-uploadpackpackObjectsHook
>  2. https://git-scm.com/docs/gitprotocol-v2#_server_option

The range-diff looks as expected to me, so this should be ready
to go from my point of view. Thanks!

Patrick

Re: [PATCH v3 0/5] Support server option from configuration

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Oct 08, 2024 at 03:38:14AM +0000, blanet via GitGitGadget wrote:
>> We manage some internal repositories with numerous CI tasks, each requiring
>> code preparation through git-clone or git-fetch. These tasks, triggered by
>> post-receive hooks, often fetch the same copy of code concurrently using
>> --depth=1, causing extremely high load spikes on our Git servers.
>> ...
>> This patch series introduces a new multi-valued configuration,
>> remote.<name>.serverOption, similar to push.pushOption, to specify default
>> server options for the corresponding remote.
>> 
>>  * Patches 1~3 contain the main changes for introducing the new
>>    configuration.
>>  * Patch 4 fixes a issue for git-fetch not sending server-options when
>>    fetching from multiple remotes.
>>  * Patch 5 is a minor fix for a server options-related memory leak.
>> 
>>  1. https://git-scm.com/docs/git-config#Documentation/git-config.txt-uploadpackpackObjectsHook
>>  2. https://git-scm.com/docs/gitprotocol-v2#_server_option
>
> The range-diff looks as expected to me, so this should be ready
> to go from my point of view. Thanks!

Thanks, both of you.  Queued.