patch, 4 partsSupport server option from configuration

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

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(-)

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(-)

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

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.

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

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(-)

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

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.

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

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

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.

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?

Xing Xin

On Wed, Sep 04, 2024 at 03:49:28PM +0800, Xing Xin wrote:

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.

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

At 2024-09-05 19:05:15, "Patrick Steinhardt" <ps@pks.im> wrote:

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.

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

On Thu, Sep 05, 2024 at 08:12:58PM +0800, Xing Xin wrote:

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>

Patrick Steinhardt <ps@pks.im> writes:

;-)  Sounds sensible.

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

[snip]

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

On Mon, Sep 09, 2024 at 10:50:21AM +0800, Xing Xin wrote:

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

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

At 2024-09-09 19:49:16, "Patrick Steinhardt" <ps@pks.im> wrote:

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

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

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.

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(-)

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

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

On Mon, Sep 02, 2024 at 12:13:52PM +0000, blanet via GitGitGadget wrote:

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

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

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(-)

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

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

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.

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

At 2024-10-07 16:22:31, "Patrick Steinhardt" <ps@pks.im> wrote:

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

Make sense.

Xing Xin

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

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

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

Nice catch.

Patrick

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

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

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

On Mon, Sep 23, 2024 at 12:17:53PM +0000, blanet via GitGitGadget wrote:

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

At 2024-10-07 16:23:02, "Patrick Steinhardt" <ps@pks.im> wrote:

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

Xing Xin

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

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(-)

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

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

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

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

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.

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

On Tue, Oct 08, 2024 at 03:38:14AM +0000, blanet via GitGitGadget wrote:

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

Patrick

Patrick Steinhardt <ps@pks.im> writes:

Thanks, both of you.  Queued.