| Message ID | pull.1006.v4.git.1628536305810.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] clone: update submodule.recurse in config when using --recurse-submodule | expand |
"Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Mahi Kolla <mahikolla@google.com> > > When running 'git clone --recurse-submodules', developers might expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules. Set 'submodule.recurse' to true when 'git clone' is run with '--recurse-submodules'. Please wrap overlong lines in your proposed log message to say 70 or so columns. Some developers might expect, but wouldn't some others want to see this not set to true, but want to recurse only into some but not all submodules? Is it possible to avoid changing the behaviour unconditionally and potentially breaking existing users by making it an opt-in feature, e.g. "git clone --recurse-submodules" would work as the current users would expect, while "git clone --recurse-submodules=sticky" would set submodule.recurse to true, or something?
Hi Junio, Thank you for your feedback! On Mon, Aug 9, 2021 at 2:15 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Mahi Kolla <mahikolla@google.com> > > > > When running 'git clone --recurse-submodules', developers might expect various other commands such as 'pull' and 'checkout' to also run recursively into submodules. Set 'submodule.recurse' to true when 'git clone' is run with '--recurse-submodules'. > > Please wrap overlong lines in your proposed log message to say 70 or > so columns. > Ah, my bad, will do so going forward. > Some developers might expect, but wouldn't some others want to see > this not set to true, but want to recurse only into some but not all > submodules? > I definitely agree with this. Currently, the `--recurse-submodules` option takes in 1 parameter, a pathspec to the submodule they would like to initialize. `submodule.active` stores this path or `.` if no path is specified. Accordingly, `submodule.recurse=true` will only apply to the submodules specified by the user in `submodule.active`. This way users can ensure commands are run recursively only in submodules they have listed as active. > Is it possible to avoid changing the behaviour unconditionally and > potentially breaking existing users by making it an opt-in feature, > e.g. "git clone --recurse-submodules" would work as the current > users would expect, while "git clone --recurse-submodules=sticky" > would set submodule.recurse to true, or something? As mentioned, the `submodule.recurse=true` will only apply to active submodules specified by the user. Setting this config value when the user runs their initial `git clone` minimizes the number of times a developer must use the `--recurse-submodule` option on other commands. However, this is a behavior change that may be surprising for developers. To ensure a smooth rollout and easy adoption, I think adding a message using an `advice.*` config setting would be useful. When a user runs `git clone --recurse-submodules` an advice message will pop up alerting them `submodule.recurse=true`, what this means for other commands' functionality, and how to change it back (`git -C dst-dir config submodule.recurse false`). This also gives the user the option to turn off the advice message if they don't need it. I'll also update the appropriate documentation to better explain how setting submodule.recurse=true effects workflow. Let me know what you think! Best, Mahi Kolla
Mahi Kolla <mahikolla@google.com> writes: >> Is it possible to avoid changing the behaviour unconditionally and >> potentially breaking existing users by making it an opt-in feature, >> e.g. "git clone --recurse-submodules" would work as the current >> users would expect, while "git clone --recurse-submodules=sticky" >> would set submodule.recurse to true, or something? > > As mentioned, the `submodule.recurse=true` will only apply to active > submodules specified by the user. Setting this config value when the > user runs their initial `git clone` minimizes the number of times a > developer must use the `--recurse-submodule` option on other commands. > > However, this is a behavior change that may be surprising for > developers. To ensure a smooth rollout and easy adoption, I think > adding a message using an `advice.*` config setting would be useful. It may be better than nothing, but that still is a unilateral behaviour change. Can't we come up with a way to make it an opt-in feature? I've already suggested to allow the "--recurse-submodules" option of "git clone" to take an optional parameter (e.g. "sticky") so that the user can request configuration variable to be set, but you seem to be ignoring or skirting it. Even though I am not married to the "give optional parameter to --recurse-submodules" design, unconditionally setting the variable, with or without advice or warning, is a regression we'd want to avoid.
Hi Junio, Le 2021-08-10 à 14:36, Junio C Hamano a écrit : > Mahi Kolla <mahikolla@google.com> writes: > >>> Is it possible to avoid changing the behaviour unconditionally and >>> potentially breaking existing users by making it an opt-in feature, >>> e.g. "git clone --recurse-submodules" would work as the current >>> users would expect, while "git clone --recurse-submodules=sticky" >>> would set submodule.recurse to true, or something? >> >> As mentioned, the `submodule.recurse=true` will only apply to active >> submodules specified by the user. Setting this config value when the >> user runs their initial `git clone` minimizes the number of times a >> developer must use the `--recurse-submodule` option on other commands. >> >> However, this is a behavior change that may be surprising for >> developers. To ensure a smooth rollout and easy adoption, I think >> adding a message using an `advice.*` config setting would be useful. > > It may be better than nothing, but that still is a unilateral > behaviour change. Can't we come up with a way to make it an opt-in > feature? I've already suggested to allow the "--recurse-submodules" > option of "git clone" to take an optional parameter (e.g. "sticky") > so that the user can request configuration variable to be set, but > you seem to be ignoring or skirting it. The '--recures-submodule' option in 'git clone' already takes an optional argument, which is a pathspec and if given, only submodules matching the given pathspec will be initialized (as opposed to all submodules if the flag is given without an argument). So, it does not seem to be possible to use this flag as a way to also set 'submodule.recurse'. When Emily (CC'ed) sent her roadmap for submodule enhancements in [1], the enhancement that Mahi is suggesting was explicitely mentioned: > - git clone ... > What doesn't already work: > > * --recurse-submodules should turn on submodule.recurse=true I don't know if Mahi is part of this effort or just came up with the same idea, but in any case maybe Emily would be able to add more justification for this change. > Even though I am not > married to the "give optional parameter to --recurse-submodules" > design, unconditionally setting the variable, with or without advice > or warning, is a regression we'd want to avoid. > In my opinion, it would not be a regression; it would a behaviour change that would be a *vast improvement* for the majority of projects that use submodules, at least those that use non-optional submodules (which, I believe, is the vast majority of projects that use submodules, judging by what I've read on the web over the past 3 years of my interest in the subject.) As soon as you use submodules in a non-optional way, you really *want* submodule.recurse=true, because if not: 1. 'git checkout' does not recursively check out your submodules, which probably breaks your build. You have to remember to always run 'git checkout --recurse-submodules' or run 'git submdule update' after each checkout, and teach your team to do the same. 2. 'git pull' fetches submodules commits, but does not recursively check out your submodules, which also probably breaks your build. You have to remember to always run 'git pull --recurse-submodules', or run 'git submodule update' after each pull, and also teach your team to do so. 3. If you forget to do 1. or 2., and then use 'git commit -am "some message" (as a lot of Git beginners unfortunately do), you regress the submodule commit, creating a lot of problems down the line. These are the main reasons that I think Git should recurse by default. Setting 'submodule.recurse' also brings other niceties, like 'git grep' recursing into submodules. If we can agree that the behaviour *should* change eventually, then at least 'git clone --recurse-submodules' could be changed *right now* to suggest setting 'submodule.recurse' using the advice API, and stating that this will be the default some day. Even if we don't agree that the behaviour should enventually change, I think having this advice would be a strict improvement because it would help user discover the setting, which would already go a long way. Thanks, Philippe. [1] https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com/
Hi Phillippe and Junio, On Tue, Aug 10, 2021 at 4:04 PM Philippe Blain <levraiphilippeblain@gmail.com> wrote: > > Hi Junio, > > Le 2021-08-10 à 14:36, Junio C Hamano a écrit : > > Mahi Kolla <mahikolla@google.com> writes: > > > >>> Is it possible to avoid changing the behaviour unconditionally and > >>> potentially breaking existing users by making it an opt-in feature, > >>> e.g. "git clone --recurse-submodules" would work as the current > >>> users would expect, while "git clone --recurse-submodules=sticky" > >>> would set submodule.recurse to true, or something? > >> > >> As mentioned, the `submodule.recurse=true` will only apply to active > >> submodules specified by the user. Setting this config value when the > >> user runs their initial `git clone` minimizes the number of times a > >> developer must use the `--recurse-submodule` option on other commands. > >> > >> However, this is a behavior change that may be surprising for > >> developers. To ensure a smooth rollout and easy adoption, I think > >> adding a message using an `advice.*` config setting would be useful. > > > > It may be better than nothing, but that still is a unilateral > > behaviour change. Can't we come up with a way to make it an opt-in > > feature? I've already suggested to allow the "--recurse-submodules" > > option of "git clone" to take an optional parameter (e.g. "sticky") > > so that the user can request configuration variable to be set, but > > you seem to be ignoring or skirting it. > > The '--recures-submodule' option in 'git clone' already takes an optional > argument, which is a pathspec and if given, only submodules matching the given > pathspec will be initialized (as opposed to all submodules if the flag is given without > an argument). So, it does not seem to be possible to use this > flag as a way to also set 'submodule.recurse'. > Because of the optional pathspec argument, adding a `=sticky` argument to the option may be hard to implement. That was my initial hesitation to the opt in design. > When Emily (CC'ed) sent her roadmap for submodule enhancements in [1], the enhancement > that Mahi is suggesting was explicitely mentioned: > > > - git clone > ... > > What doesn't already work: > > > > * --recurse-submodules should turn on submodule.recurse=true > > I don't know if Mahi is part of this effort or just came up with the same idea, > but in any case maybe Emily would be able to add more justification for this change. > I am part of the team and am implementing that exact feature from the roadmap :) > > Even though I am not > > married to the "give optional parameter to --recurse-submodules" > > design, unconditionally setting the variable, with or without advice > > or warning, is a regression we'd want to avoid. > > > > In my opinion, it would not be a regression; it would a behaviour change that > would be a *vast improvement* for the majority of projects that use submodules, at > least those that use non-optional submodules (which, I believe, is the vast majority > of projects that use submodules, judging by what I've read on the web over the past 3 > years of my interest in the subject.) > > As soon as you use submodules in a non-optional way, you really *want* submodule.recurse=true, > because if not: > > 1. 'git checkout' does not recursively check out your submodules, which probably breaks your build. > You have to remember to always run 'git checkout --recurse-submodules' or run 'git submdule update' > after each checkout, and teach your team to do the same. > 2. 'git pull' fetches submodules commits, but does not recursively check out your submodules, > which also probably breaks your build. You have to remember to always run 'git pull --recurse-submodules', > or run 'git submodule update' after each pull, and also teach your team to do so. > 3. If you forget to do 1. or 2., and then use 'git commit -am "some message" (as a lot > of Git beginners unfortunately do), you regress the submodule commit, creating a lot > of problems down the line. > > These are the main reasons that I think Git should recurse by default. Setting 'submodule.recurse' > also brings other niceties, like 'git grep' recursing into submodules. > I completely agree with this! These are a lot of the reasons why the feature was initially suggested. An alternative path forward the team discussed was testing `submodule.recurse=true` under `feature.experimental`. This way we can collect feedback from developers before making this the default config value. > If we can agree that the behaviour *should* change eventually, then at least > 'git clone --recurse-submodules' could be changed *right now* to suggest setting > 'submodule.recurse' using the advice API, and stating that this will be the default > some day. > > Even if we don't agree that the behaviour should enventually change, I think > having this advice would be a strict improvement because > it would help user discover the setting, which would already go a long way. > > Thanks, > > Philippe. > > > [1] https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com/ I agree that adding an advice message when a user runs `git clone --recurse-submodules` would at least alert users of their options, giving them the choice to set `submodule.recurse` accordingly. Thanks! Best, Mahi Kolla
Junio C Hamano <gitster@pobox.com> writes: > Mahi Kolla <mahikolla@google.com> writes: > >>> Is it possible to avoid changing the behaviour unconditionally and >>> potentially breaking existing users by making it an opt-in feature, >>> e.g. "git clone --recurse-submodules" would work as the current >>> users would expect, while "git clone --recurse-submodules=sticky" >>> would set submodule.recurse to true, or something? >> >> As mentioned, the `submodule.recurse=true` will only apply to active >> submodules specified by the user. Setting this config value when the >> user runs their initial `git clone` minimizes the number of times a >> developer must use the `--recurse-submodule` option on other commands. >> >> However, this is a behavior change that may be surprising for >> developers. To ensure a smooth rollout and easy adoption, I think >> adding a message using an `advice.*` config setting would be useful. Let me outline some general rules on changing the behaviour of the system used around here. First of all, if a proposed change of behaviour is a bugfix, the following does not apply [*1*]. When a new behaviour is made available to those who want to use it, it starts as an opt-in feature. - Existing users will not be surprised by a familiar command suddenly changing its behaviour. If users keep using the system the same way as they used it before, the system will behave the same way, without changing the behaviour. - Those who want to use the new behaviour need to do something to explicitly trigger it (with a command line option, configuration variable, a new command, etc.) Over time, a behaviour that used to be a "new way" may just become "one of the two ways available", and it may even turn out to be a more desirable one between the two. At that point, we may propose to flip the default, with a migration plan that is carefully designed to avoid breaking existing users. Even if it were an *improvement* to set the configuration variable, it is not an excuse to suddenly change the behaviour of the command for users who do not ask. It needs to start as an optional feature, and if we really like it and manage to convince majority users to also like the new way, we may even consider making it the default, but it is way too premature to do so. Unless we can argue that the current behaviour *is* buggy, that is. Thanks. [Footnote] *1* A change that we have to say "not all users may be happy with this new behaviour" or "developers would be surprised by the new behaviour" cannot be a bugfix.
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt index 3fe3810f1ce..1d6aeb9e367 100644 --- a/Documentation/git-clone.txt +++ b/Documentation/git-clone.txt @@ -276,7 +276,7 @@ branch of some repository for search indexing. This option can be given multiple times for pathspecs consisting of multiple entries. The resulting clone has `submodule.active` set to the provided pathspec, or "." (meaning all submodules) if no - pathspec is provided. + pathspec is provided. In addition, `submodule.recurse` is set to true. + Submodules are initialized and cloned using their default settings. This is equivalent to running diff --git a/builtin/clone.c b/builtin/clone.c index 66fe66679c8..c6bb38d2fde 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -1130,6 +1130,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) strbuf_detach(&sb, NULL)); } + string_list_append(&option_config, "submodule.recurse=true"); if (option_required_reference.nr && option_optional_reference.nr) die(_("clone --recursive is not compatible with " diff --git a/t/t5606-clone-options.sh b/t/t5606-clone-options.sh index 3a595c0f82c..055b74069b3 100755 --- a/t/t5606-clone-options.sh +++ b/t/t5606-clone-options.sh @@ -16,6 +16,13 @@ test_expect_success 'setup' ' ' +test_expect_success 'clone --recurse-submodules sets submodule.recurse=true' ' + + git clone --recurse-submodules parent clone-rec-submodule && + test_cmp_config -C clone-rec-submodule true submodule.recurse + +' + test_expect_success 'clone -o' ' git clone -o foo parent clone-o &&