From: Junio C Hamano <hidden> Date: 2021-08-12 04:21:20
"Mahi Kolla via GitGitGadget" [off-list ref] writes:
From: Mahi Kolla <redacted>
Currently, when running 'git clone --recurse-submodules', developers do not expect other commands such as 'pull' or 'checkout' to run recursively into active submodules. However, setting 'submodule.recurse' to true at this step could make for a simpler workflow by eliminating the '--recurse-submodules' option in subsequent commands. To collect more data on developers' preference in regards to making 'submodule.recurse=true' a default config value in the future, deploy this feature under the opt in feature.experimental flag.
Please wrap overlong lines in your proposed log message to say 70 or
so columns.
Since V1: Made this an opt in feature under the experimental flag. Updated tests to reflect this design change. Also updated commit message.
This does not belong to the commit log message proper. Noting the
difference between the version being submitted and the pervious one
this way is a way to help reviewers and is very much appreciated,
but please do so below the three-dash line below your sign-off.
Signed-off-by: Mahi Kolla <redacted>
---
clone: set submodule.recurse=true if feature.experimental flag enabled
The proposed approach misuses feature.experimental flag, which was
designed to turn on many new features at once. The features covered
by the flag share one common trait: they all have gained consensus
that in the longer term we would hopefully be able to make it on by
default, and give early adopters an easy way to turn them all on.
I do not think setting submodule.recurse=true upon "clone --recurse"
falls into that category just yet. If we were to make this opt-in,
we'd want a separate flag, so that those early adopters who are
dogfooding other features that have consensus that they are
hopefully the way of the future won't have to be forced into this
separate feature.
Perhaps a separate (and new) configuration variable (in ~/.gitconfig
perhaps) can be used as that opt-in flag (I wonder if the existing
submodule.recurse variable can be that opt-in flag, though).
On Wed, Aug 11, 2021 at 09:20:58PM -0700, Junio C Hamano wrote:
"Mahi Kolla via GitGitGadget" [off-list ref] writes:
quoted
From: Mahi Kolla <redacted>
Currently, when running 'git clone --recurse-submodules', developers do not expect other commands such as 'pull' or 'checkout' to run recursively into active submodules. However, setting 'submodule.recurse' to true at this step could make for a simpler workflow by eliminating the '--recurse-submodules' option in subsequent commands. To collect more data on developers' preference in regards to making 'submodule.recurse=true' a default config value in the future, deploy this feature under the opt in feature.experimental flag.
Please wrap overlong lines in your proposed log message to say 70 or
so columns.
quoted
Since V1: Made this an opt in feature under the experimental flag. Updated tests to reflect this design change. Also updated commit message.
This does not belong to the commit log message proper. Noting the
difference between the version being submitted and the pervious one
this way is a way to help reviewers and is very much appreciated,
but please do so below the three-dash line below your sign-off.
quoted
Signed-off-by: Mahi Kolla <redacted>
---
clone: set submodule.recurse=true if feature.experimental flag enabled
The proposed approach misuses feature.experimental flag, which was
designed to turn on many new features at once. The features covered
by the flag share one common trait: they all have gained consensus
that in the longer term we would hopefully be able to make it on by
default, and give early adopters an easy way to turn them all on.
I do not think setting submodule.recurse=true upon "clone --recurse"
falls into that category just yet. If we were to make this opt-in,
we'd want a separate flag, so that those early adopters who are
dogfooding other features that have consensus that they are
hopefully the way of the future won't have to be forced into this
separate feature.
I'd like to open discussions to get said consensus :)
It seems surprising to me that a user would want to clone with all the
submodules fetched *without* intending to then use
superproject-plus-submodules together recursively. I would like to hear
more about the use case you have in mind, Junio.
One scenario that did come to mind when I discussed this with Mahi is
that a user may provide a pathspec to --recurse-submodules (that is,
"yes, this repo has submodules a/ and b/, but I only care about the
contents of submodule a/") - and in that case, --recurse-submodules
seems to do the right thing with or without Mahi's change.
It seemed to me that trying out this change on feature.experimental flag
was the right approach, because users with that flag have already
volunteered to be testers for upcoming behavior changes; this seems like
one such that is likely to be welcome. By contrast, turning the behavior
on with a separate config variable reduces the pool of testers
essentially to "users who know about this change" - or, to be more
reductive, "a handful of users at Google who we Google Git contributors
already know want this change". I recommended to Mahi that we stick this
feature under 'feature.experimental' because I really wanted to hear
from more users than just Googlers.
Perhaps a separate (and new) configuration variable (in ~/.gitconfig
perhaps) can be used as that opt-in flag (I wonder if the existing
submodule.recurse variable can be that opt-in flag, though).
Do you mean something like "git config --global submodule.recurse
TryTheNewThingPlease"? I guess it could work - repos that use a pathspec
in that slot would still have the pathspec configured locally, repos
that have submodule.recurse intentionally unset wouldn't know what to do
with the junk string, and repos that have submodule.recurse
intentionally set to true would still have that true override the global
value.
Or else I misunderstood you...
- Emily
From: Philippe Blain <hidden> Date: 2021-08-13 03:35:47
Hi Emily,
Le 2021-08-12 à 19:54, Emily Shaffer a écrit :
On Wed, Aug 11, 2021 at 09:20:58PM -0700, Junio C Hamano wrote:
quoted
"Mahi Kolla via GitGitGadget" [off-list ref] writes:
quoted
From: Mahi Kolla <redacted>
Currently, when running 'git clone --recurse-submodules', developers do not expect other commands such as 'pull' or 'checkout' to run recursively into active submodules. However, setting 'submodule.recurse' to true at this step could make for a simpler workflow by eliminating the '--recurse-submodules' option in subsequent commands. To collect more data on developers' preference in regards to making 'submodule.recurse=true' a default config value in the future, deploy this feature under the opt in feature.experimental flag.
Please wrap overlong lines in your proposed log message to say 70 or
so columns.
quoted
Since V1: Made this an opt in feature under the experimental flag. Updated tests to reflect this design change. Also updated commit message.
This does not belong to the commit log message proper. Noting the
difference between the version being submitted and the pervious one
this way is a way to help reviewers and is very much appreciated,
but please do so below the three-dash line below your sign-off.
Mahi, since you're using Gitgitgadget, you would put this "since v1"
content in the PR description, and Gitgitgadget will append it under
the three-dash line when you /submit :) (Do keep the CC's automatically
added by GGG so that your next version is CC'ed to those that participated
in earlier rounds).
quoted
quoted
Signed-off-by: Mahi Kolla <redacted>
---
clone: set submodule.recurse=true if feature.experimental flag enabled
The proposed approach misuses feature.experimental flag, which was
designed to turn on many new features at once. The features covered
by the flag share one common trait: they all have gained consensus
that in the longer term we would hopefully be able to make it on by
default, and give early adopters an easy way to turn them all on.
I do not think setting submodule.recurse=true upon "clone --recurse"
falls into that category just yet. If we were to make this opt-in,
we'd want a separate flag, so that those early adopters who are
dogfooding other features that have consensus that they are
hopefully the way of the future won't have to be forced into this
separate feature.
I'd like to open discussions to get said consensus :)
It seems surprising to me that a user would want to clone with all the
submodules fetched *without* intending to then use
superproject-plus-submodules together recursively. I would like to hear
more about the use case you have in mind, Junio.
One scenario that did come to mind when I discussed this with Mahi is
that a user may provide a pathspec to --recurse-submodules (that is,
"yes, this repo has submodules a/ and b/, but I only care about the
contents of submodule a/") - and in that case, --recurse-submodules
seems to do the right thing with or without Mahi's change.
I'm not sure what you mean by "the right thing" here. '--recurse-submodules=a'
would set 'submodule.active' to 'a', which means "when command are asked to recurse into
submodules, I only care about submodules a", but it does not do anything to
'submodule.recurse=true', which means "I do not ever want to type '--recurse-submodules',
always use this behaviour for all commands that have that flag, except clone and ls-files.
Unless I'm missing something :)
It seemed to me that trying out this change on feature.experimental flag
was the right approach, because users with that flag have already
volunteered to be testers for upcoming behavior changes; this seems like
one such that is likely to be welcome. By contrast, turning the behavior
on with a separate config variable reduces the pool of testers
essentially to "users who know about this change" - or, to be more
reductive, "a handful of users at Google who we Google Git contributors
already know want this change". I recommended to Mahi that we stick this
feature under 'feature.experimental' because I really wanted to hear
from more users than just Googlers.
I agree that we would not want yet another config variable that users would
have to set. If people know about submodule.recurse and want to always use this
behaviour, they already have it in their ~/.gitconfig, so they do not need a new
variable. If they do not know about submodule.recurse, then they probably won't learn
about this new variable either ;) That's why I suggested to Mahi that in any case it would
be a good thing that 'git clone --recurse-submodules' would at least inform users, using
an advice, that they might want to set submodule.recurse.
Regarding feature.experimental, I do not have a strong opinion. I don't think
the population of Git users that have this flag set is representative of the total
population of Git users, unfortunately. But I agree it's better than nothing.
quoted
Perhaps a separate (and new) configuration variable (in ~/.gitconfig
perhaps) can be used as that opt-in flag (I wonder if the existing
submodule.recurse variable can be that opt-in flag, though).
Do you mean something like "git config --global submodule.recurse
TryTheNewThingPlease"? I guess it could work - repos that use a pathspec
in that slot would still have the pathspec configured locally,
Here I think you are confusing submodule.active (which takes a pathspec)
and submodule.recurse (which takes a boolean).
Cheers,
Philippe.
Hi all,
Thank you all for the great feedback! I'm learning a lot as a
first-time contributor :) I will be wrapping my internship this week
and will continue contributing externally.
quoted
quoted
quoted
Since V1: Made this an opt in feature under the experimental flag. Updated tests to reflect this design change. Also updated commit message.
This does not belong to the commit log message proper. Noting the
difference between the version being submitted and the pervious one
this way is a way to help reviewers and is very much appreciated,
but please do so below the three-dash line below your sign-off.
Mahi, since you're using Gitgitgadget, you would put this "since v1"
content in the PR description, and Gitgitgadget will append it under
the three-dash line when you /submit :) (Do keep the CC's automatically
added by GGG so that your next version is CC'ed to those that participated
in earlier rounds).
Got it, thank you!
quoted
It seemed to me that trying out this change on feature.experimental flag
was the right approach, because users with that flag have already
volunteered to be testers for upcoming behavior changes; this seems like
one such that is likely to be welcome. By contrast, turning the behavior
on with a separate config variable reduces the pool of testers
essentially to "users who know about this change" - or, to be more
reductive, "a handful of users at Google who we Google Git contributors
already know want this change". I recommended to Mahi that we stick this
feature under 'feature.experimental' because I really wanted to hear
from more users than just Googlers.
I agree that we would not want yet another config variable that users would
have to set. If people know about submodule.recurse and want to always use this
behaviour, they already have it in their ~/.gitconfig, so they do not need a new
variable. If they do not know about submodule.recurse, then they probably won't learn
about this new variable either ;) That's why I suggested to Mahi that in any case it would
be a good thing that 'git clone --recurse-submodules' would at least inform users, using
an advice, that they might want to set submodule.recurse.
When discussing with the team, we revisited the feature.experimental
design. As we have yet to gain strong consensus on making this a
default config value, we've decided to ship it under a different
config value: submodule.stickyRecursiveClone. Now, if the user sets
submodule.stickyRecursiveClone=true, when they run git clone
--recurse-submodules, we will set submodule.recurse=true. While this
may mean a smaller dataset (only people who know of this flag), we can
still collect valuable data.
As for the advice message, I agree that would be a really useful
feature. I'll submit that as a different patch.
quoted
quoted
Perhaps a separate (and new) configuration variable (in ~/.gitconfig
perhaps) can be used as that opt-in flag (I wonder if the existing
submodule.recurse variable can be that opt-in flag, though).
Unfortunately, the submodule.recurse variable can't be used as the
opt-in flag because this would cause commands to run recursively even
if developers don't have submodules in their project (aka don't run
git clone --recurse-submodules). That's why the alternate config value
seems a better choice at the moment.
Let me know what you guys think!
Best,
Mahi Kolla
From: Junio C Hamano <hidden> Date: 2021-08-13 04:34:50
Emily Shaffer [off-list ref] writes:
It seems surprising to me that a user would want to clone with all the
submodules fetched *without* intending to then use
superproject-plus-submodules together recursively. I would like to hear
more about the use case you have in mind, Junio.
You may need full forest of submodules with the superproject to
build your ware (i.e. you'd probably want to clone and fetch-update
them), but you may only be working on the sources in a small subset
of submodules and do not need your recursive grep or diff to go
outside that subset, for example. You'd need to ask the people who
recursively clone and not set submodule.recurse to true (I am not
among them).
One scenario that did come to mind when I discussed this with Mahi is
that a user may provide a pathspec to --recurse-submodules (that is,
"yes, this repo has submodules a/ and b/, but I only care about the
contents of submodule a/") - and in that case, --recurse-submodules
seems to do the right thing with or without Mahi's change.
Please be a bit more specific about "the right thing". Do you mean
"the submodules that matched the pathspec gets recursed into by
later operations"?
If so, "git clone --resurse-submodules=. $from_there" may perhaps be
the "there is no way to we make this opt-in?" I have been asking
about (not "asking for")?
It seemed to me that trying out this change on feature.experimental flag
was the right approach, because users with that flag have already
volunteered to be testers for upcoming behavior changes
Yes, if we already have a consensus that a proposed change is
something we hope to be desirable, then feature.experimental is a
good way to see if early adopters can find problems in their real
world use, as these volunteers may include audiences with different
use pattern from the original advocates of a particular feature, who
might have dogfooded the new feature to gain consensus that it may
want to become the default.
By the way, I am not fundamentally opposed to the feature being
proposed. I would imagine that such a feature would be liked by
those who want to keep things simpler. I however am hesitant to see
it pushed too hastily without considering if it harms existing users
with different preferences.
IOW, I was primarily reacting to the apparent wrong order in which
things are being done, first throwing this into feature.experimental
before we have gathered enough confidence that it may be a good
thing to do by having it in shipped version as an opt-in feature.
Thanks.
On Thu, Aug 12, 2021 at 09:34:47PM -0700, Junio C Hamano wrote:
Emily Shaffer [off-list ref] writes:
quoted
It seems surprising to me that a user would want to clone with all the
submodules fetched *without* intending to then use
superproject-plus-submodules together recursively. I would like to hear
more about the use case you have in mind, Junio.
You may need full forest of submodules with the superproject to
build your ware (i.e. you'd probably want to clone and fetch-update
them), but you may only be working on the sources in a small subset
of submodules and do not need your recursive grep or diff to go
outside that subset, for example. You'd need to ask the people who
recursively clone and not set submodule.recurse to true (I am not
among them).
quoted
One scenario that did come to mind when I discussed this with Mahi is
that a user may provide a pathspec to --recurse-submodules (that is,
"yes, this repo has submodules a/ and b/, but I only care about the
contents of submodule a/") - and in that case, --recurse-submodules
seems to do the right thing with or without Mahi's change.
Please be a bit more specific about "the right thing". Do you mean
"the submodules that matched the pathspec gets recursed into by
later operations"?
If so, "git clone --resurse-submodules=. $from_there" may perhaps be
the "there is no way to we make this opt-in?" I have been asking
about (not "asking for")?
quoted
It seemed to me that trying out this change on feature.experimental flag
was the right approach, because users with that flag have already
volunteered to be testers for upcoming behavior changes
Yes, if we already have a consensus that a proposed change is
something we hope to be desirable, then feature.experimental is a
good way to see if early adopters can find problems in their real
world use, as these volunteers may include audiences with different
use pattern from the original advocates of a particular feature, who
might have dogfooded the new feature to gain consensus that it may
want to become the default.
By the way, I am not fundamentally opposed to the feature being
proposed. I would imagine that such a feature would be liked by
those who want to keep things simpler. I however am hesitant to see
it pushed too hastily without considering if it harms existing users
with different preferences.
IOW, I was primarily reacting to the apparent wrong order in which
things are being done, first throwing this into feature.experimental
before we have gathered enough confidence that it may be a good
thing to do by having it in shipped version as an opt-in feature.
Yeah, since writing my reply I was very helpfully reinformed on the
convention around 'feature.experimental' by Jonathan N off-list. Thanks
for being patient with me.
I think the right move, then, is to explore whether your suggestion in
https://lore.kernel.org/git/xmqqeeaxw28z.fsf%40gitster.g is appropriate
- I have the sense that it is, but I want to make sure to think it
through before I say so for sure.
- Emily