Mahi Kolla [off-list ref] writes:
quoted
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 [off-list ref] writes:
quoted
quoted
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
[off-list ref] wrote:
Hi Junio,
Le 2021-08-10 à 14:36, Junio C Hamano a écrit :
quoted
Mahi Kolla [off-list ref] writes:
quoted
quoted
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:
quoted
- git clone
...
quoted
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 :)
quoted
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 [off-list ref] writes:
Mahi Kolla [off-list ref] writes:
quoted
quoted
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.