From: Xing Xin Date: Mon, 23 Sep 2024 13:04:11 GMT Subject: Re:Re: Re: Re: Re: [PATCH 3/4] builtin/clone.c: recognize fetch.serverOption configuration Message-ID: <7b01a69b.bdf6.1921efb807c.Coremail.bupt_xingxin@163.com> In-Reply-To: At 2024-09-09 19:49:16, "Patrick Steinhardt" wrote: >On Mon, Sep 09, 2024 at 10:50:21AM +0800, Xing Xin wrote: >> At 2024-09-05 21:44:41, "Patrick Steinhardt" 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..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.*. 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.` or `remote.*.` 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..` and `http.`(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..serverOption` configuration. Xing Xin