| Message ID | 7c3ebda513d872a2ab2aa0cff5887757de4cde0a.1725279236.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Support server option from configuration | expand |
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
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
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
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
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>
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.
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
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
"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.
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
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' '