| Message ID | pull.1006.v5.git.1628736366133.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v5] clone: set submodule.recurse=true if user enables feature.experimental flag | expand |
"Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Mahi Kolla <mahikolla@google.com> > > 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 <mahikolla@google.com> > --- > 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" <gitgitgadget@gmail.com> writes: > > > From: Mahi Kolla <mahikolla@google.com> > > > > 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 <mahikolla@google.com> > > --- > > 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
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: >> >> "Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> From: Mahi Kolla <mahikolla@google.com> >>> >>> 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. 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). >> >>> Signed-off-by: Mahi Kolla <mahikolla@google.com> >>> --- >>> 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. > >> >> 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. > >> > >>> > >>> 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! > > > > 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. > >> > >> 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
Emily Shaffer <emilyshaffer@google.com> 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.
Philippe Blain <levraiphilippeblain@gmail.com> writes: > Here I think you are confusing submodule.active (which takes a pathspec) > and submodule.recurse (which takes a boolean). Sigh, but I have to agree with you. I really hoped that in a recursive clone of a repository with two submodules A and B, when made with --recurse-submodules=A, "git grep ." would look for things in the superproject and submodule A as Emily seemed to have meant by her "the right thing", but you are correct. We only set .active but we do not set .recurse, so "git grep ." in the superproject does not descend into neither submodules without being told. If it did "the right thing" as Emily said, it would have been much easier to justify the change being proposed as a simple fix for the bug that --recurse-submodules without pathspec does one thing (i.e. setting things up not to recurse for later "grep" etc.") and the same option with "everything matches" pathspec "." does a different thing (i.e. always to recurse). The discrepancy would have given us an excuse, an argument for changing the behaviour for the former to match the latter. Some users may have deliberately built their workflow relying on the distinction and the result still may give them a regression, but at least it would have gave us a viable justification: A command run without pathspec means the entire tree and it is the same as running it with pathspec '.' in the rest of Git, but the way "git clone --recurse-submodules" handles its optional pathspec is inconsistent. Treat "clone --recurse-submodules" without pathspec as if it came with pathspec '.' and give the same configuration. But unfortunately it does not seem to be the case.
Mahi Kolla via GitGitGadget wrote: > From: Mahi Kolla <mahikolla@google.com> > --- a/builtin/clone.c > +++ b/builtin/clone.c > @@ -986,6 +986,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > struct remote *remote; > int err = 0, complete_refs_before_fetch = 1; > int submodule_progress; > + int experimental_flag; > > struct transport_ls_refs_options transport_ls_refs_options = > TRANSPORT_LS_REFS_OPTIONS_INIT; > @@ -1130,6 +1131,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > strbuf_detach(&sb, NULL)); > } > > + if(!git_config_get_bool("feature.experimental", &experimental_flag) && You are missing a space after the 'if'. > + experimental_flag) { > + string_list_append(&option_config, "submodule.recurse=true"); > + } > +
Mahi Kolla <mahikolla@google.com> writes: > 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). If you cloned without recurse-submodules and lack submodules, wouldn't it be a no-op to have submodule.recurse set to true, so it would not hurt anyway? IOW, that may already solve the original problem you wanted to solve---those who want their submodules recursively descended into by default can just set submodule.recurse to true (in ~/.gitconfig presumably) and after "git clone" with --recurse-submodules they will get what they want, no? Am I missing something obvious? Thanks.
On Thu, Aug 12, 2021 at 09:34:47PM -0700, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> 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. 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
Emily Shaffer <emilyshaffer@google.com> writes: > 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. Not that one---it was a 40% tongue-in-cheek comment, and does not deserve to be called a suggestion.
On Fri, Aug 13, 2021 at 01:30:22PM -0700, Junio C Hamano wrote: > > Emily Shaffer <emilyshaffer@google.com> writes: > > > 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. > > Not that one---it was a 40% tongue-in-cheek comment, and does not > deserve to be called a suggestion. Ah well ;) Anyway, I think it does not make sense, as behavior starts to change for people who already cloned expecting not to recurse (Jonathan N says this is the case for his Rust checkout, for example) - and apparently 'submodule.recurse=true' has some weird edge cases for commands which are happy to run out-of-repo. Mahi mentioned wanting to rework her commit to use a config besides 'feature.experimental' for this same behavior, so hopefully we will see that change come through soon - but today is also the last day of her internship, so we may not be so lucky. - Emily
> Mahi mentioned wanting to rework her commit to use a config besides > 'feature.experimental' for this same behavior, so hopefully we will see > that change come through soon - but today is also the last day of her > internship, so we may not be so lucky. > This change is pretty much good to go! I implemented it under `submodule.stickyRecursiveClone`. The commit is actually in the PR. Just wanted to hear more from you guys before submitting :)
Emily Shaffer <emilyshaffer@google.com> writes: > 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. You may (or may not) have noticed it, but the entry for the topic in the last "What's cooking" report pretends as if we have already concensus that it is a good thing to do and be made default. As it seems to be valid use cases to have submodule.recurse not set to true even when you have submodules (a clone of Rust Jonathan N uses you mentioned in the other message, perhaps?), it probably does make sense to initially make this opt-in and also keep opt-in, not as a candidate for future default, but we'll see. Thanks.
diff --git a/builtin/clone.c b/builtin/clone.c index 66fe66679c8..24f242b4a60 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -986,6 +986,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) struct remote *remote; int err = 0, complete_refs_before_fetch = 1; int submodule_progress; + int experimental_flag; struct transport_ls_refs_options transport_ls_refs_options = TRANSPORT_LS_REFS_OPTIONS_INIT; @@ -1130,6 +1131,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix) strbuf_detach(&sb, NULL)); } + if(!git_config_get_bool("feature.experimental", &experimental_flag) && + experimental_flag) { + 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..f20cdfa6fca 100755 --- a/t/t5606-clone-options.sh +++ b/t/t5606-clone-options.sh @@ -16,6 +16,18 @@ test_expect_success 'setup' ' ' +test_expect_success 'feature.experimental flag manipulates submodule.recurse value' ' + + test_config_global feature.experimental true && + git clone --recurse-submodules parent clone_recurse_true && + test_cmp_config -C clone_recurse_true true submodule.recurse && + + test_config_global feature.experimental false && + git clone --recurse-submodules parent clone_recurse_false && + test_expect_code 1 git -C clone_recurse_false config --get submodule.recurse + +' + test_expect_success 'clone -o' ' git clone -o foo parent clone-o &&