| Message ID | pull.1006.v6.git.1628903396783.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v6] clone: set submodule.recurse=true if submodule.stickyRecursiveClone enabled | expand |
"Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/builtin/clone.c b/builtin/clone.c > index 66fe66679c8..a08d9012243 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 sticky_recursive_clone; This variable does not have to be in such a wider scope, I think. > 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)); > } Just in this scope, where "struct string_list_item *item" and "struct strbuf sb" are declared, is where an extra int variable, which receives the configuration value, needs to exist. Also, for a variable that is used only to receive value from git_config_get_bool(), immediately to be used and then never used again, we do not need such a long and descriptive name. > + if (!git_config_get_bool("submodule.stickyRecursiveClone", &sticky_recursive_clone) > + && sticky_recursive_clone) { > + string_list_append(&option_config, "submodule.recurse=true"); > + } We do not need {} around a single statement block. Taken together, perhaps like the attached. I'll queue the patch posted as-is for now. Thanks. diff --git c/builtin/clone.c w/builtin/clone.c index 66fe66679c..c4e02d2f78 100644 --- c/builtin/clone.c +++ w/builtin/clone.c @@ -1114,6 +1114,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (option_recurse_submodules.nr > 0) { struct string_list_item *item; struct strbuf sb = STRBUF_INIT; + int val; /* remove duplicates */ string_list_sort(&option_recurse_submodules); @@ -1130,6 +1131,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix) strbuf_detach(&sb, NULL)); } + if (!git_config_get_bool("submodule.stickyRecursiveClone", &val) && + val) + string_list_append(&option_config, "submodule.recurse=true"); + if (option_required_reference.nr && option_optional_reference.nr) die(_("clone --recursive is not compatible with "
On Sat, Aug 14, 2021 at 11:05:41AM -0700, Junio C Hamano wrote: > > "Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > diff --git a/builtin/clone.c b/builtin/clone.c > > index 66fe66679c8..a08d9012243 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 sticky_recursive_clone; > > This variable does not have to be in such a wider scope, I think. > > > > 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)); > > } > > Just in this scope, where "struct string_list_item *item" and > "struct strbuf sb" are declared, is where an extra int variable, > which receives the configuration value, needs to exist. > > Also, for a variable that is used only to receive value from > git_config_get_bool(), immediately to be used and then never used > again, we do not need such a long and descriptive name. > > > + if (!git_config_get_bool("submodule.stickyRecursiveClone", &sticky_recursive_clone) > > + && sticky_recursive_clone) { > > + string_list_append(&option_config, "submodule.recurse=true"); > > + } > > We do not need {} around a single statement block. > > Taken together, perhaps like the attached. > > I'll queue the patch posted as-is for now. > > Thanks. > > diff --git c/builtin/clone.c w/builtin/clone.c > index 66fe66679c..c4e02d2f78 100644 > --- c/builtin/clone.c > +++ w/builtin/clone.c > @@ -1114,6 +1114,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > if (option_recurse_submodules.nr > 0) { > struct string_list_item *item; > struct strbuf sb = STRBUF_INIT; > + int val; > > /* remove duplicates */ > string_list_sort(&option_recurse_submodules); > @@ -1130,6 +1131,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix) > strbuf_detach(&sb, NULL)); > } > > + if (!git_config_get_bool("submodule.stickyRecursiveClone", &val) && > + val) > + string_list_append(&option_config, "submodule.recurse=true"); > + > if (option_required_reference.nr && > option_optional_reference.nr) > die(_("clone --recursive is not compatible with " I like the changes you propose here. It also looks to me like you wanted to wait for Mahi to send the updates herself, which I appreciate. Mahi's internship ended last week and I believe she told me she's planning to be very busy this week; so if we don't hear from her by this time next week, I'll send a reroll with these changes (unless you want to just take them yourself without the list churn). - Emily
Emily Shaffer <emilyshaffer@google.com> writes: > I like the changes you propose here. It also looks to me like you wanted > to wait for Mahi to send the updates herself, which I appreciate. > > Mahi's internship ended last week and I believe she told me she's > planning to be very busy this week; so if we don't hear from her by this > time next week, I'll send a reroll with these changes (unless you want > to just take them yourself without the list churn). I'd make a "fixup!" out of the message you are responding to, and then queue it on top of what was posted by Mahi. Then I'll send the fixup to the list as a reply to this message, so that later you and Mahi can ack (or further update) it. After that, I can squash the fixup into the patch and merge it down (unless there are objections from other places, that is). Thanks.
Hi Mahi, Emily and Junio, Le 2021-08-13 à 21:09, Mahi Kolla via GitGitGadget a écrit : > From: Mahi Kolla <mkolla2@illinois.edu> > > Based on current experience, 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=true > at this step could make for a simpler workflow by eliminating the need for > 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 > submodule.stickyRecursiveClone flag. > As I mentioned upthread [1], I'm really not sure that we need a new config variable. If people want to have "--recurse-submodules" the default behaviour for all commands that know this flag, they can set 'submodule.recurse' in their ~/.gitconfig, which enables the behaviour for all those commands (except clone and ls-files). And orgs shipping Git to their devs can set it in their system gitconfig. I've been using this setup for over two years, with almost zero adverse effect on repositories that do not contain submodules. The *only* bug that I encountered that affects git commands when *no* submodules are at play was: c56c48dd07 (grep: ignore --recurse-submodules if --no-index is given, 2020-01-30) I understand that once submodule.recurse is set in the global or system config file, then Git will start recursing in repos that were cloned prior to that config being enabled, as Emily mentions in [2]. Personnally I think it's a positive point. That *might* be a deal-breaker for other people, but in any case it would be good that this alternative - just using 'submodule.recurse' - is mentioned in the commit message and that it mentions that caveat, i.e. why we need a separate config. Le 2021-08-13 à 16:38, Emily Shaffer a écrit : > and apparently > 'submodule.recurse=true' has some weird edge cases for commands which > are happy to run out-of-repo. It would be nice if those known edge cases were documented somewhere (on the list, on Gitgitgadget's issues list or at https://bugs.chromium.org/p/git). Apart from the 'grep --no-index' glitch that I mentioned above, I did not encounter any myself. > Signed-off-by: Mahi Kolla <mkolla2@illinois.edu> > --- > clone: set submodule.recurse=true if submodule.stickyRecursiveClone > enabled > > Based on current experience, 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=true at this step could make for a simpler > workflow by eliminating the need for the --recurse-submodules option in > subsequent commands. To collect more data on developers' behavior and > preferences when making submodule.recurse=true a default, deploy this > feature under the opt in submodule.stickyRecursiveClone flag. > > Signed-off-by: Mahi Kolla mkolla2@illinois.edu Mahi, you can keep just the "Since v1" part in the GGG PR description (and the automatically added CC's). No need to also repeat the commit message. > > Since V1: Made this an opt in feature under a custom > submodule.stickyRecursiveClone flag as opposed to feature.experimental. > Updated tests to reflect this design change. Also updated commit > message. Additionally, I will be contributing from my personal email > going forward as opposed to my @google email. > > cc: Philippe Blain levraiphilippeblain@gmail.com Small nit: since you used '/submit' several times on Gitgitgadget, the previous version you sent was actually sent as v5, and this here version is v6. So for next round, you could write "Changes since v6" :) > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1006%2F24mahik%2Fmaster-v6 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1006/24mahik/master-v6 > Pull-Request: https://github.com/gitgitgadget/git/pull/1006 > > Range-diff vs v5: > > > > -+test_expect_success 'feature.experimental flag manipulates submodule.recurse value' ' > ++test_expect_success 'submodule.stickyRecursiveClone flag manipulates submodule.recurse value' ' > + > -+ test_config_global feature.experimental true && > ++ test_config_global submodule.stickyRecursiveClone 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 && > ++ test_config_global submodule.stickyRecursiveClone false && > + git clone --recurse-submodules parent clone_recurse_false && > + test_expect_code 1 git -C clone_recurse_false config --get submodule.recurse > + OK, so the expectation with 'submodule.stickyRecursiveClone' is that : - if it's true, then 'submodule.recurse' is set to true in the clone's local config file. That makes sense. - if it's false, then 'submodule.recurse' is not set in the clone. This means that if 'submodule.recurse' is already set in ~/.gitconfig (or the system config file) then the clone will respect the configured value. That also makes sense, I think. > > builtin/clone.c | 6 ++++++ > t/t5606-clone-options.sh | 12 ++++++++++++ > 2 files changed, 18 insertions(+) The new config variable should be documented in Documentation/config/clone.txt (which gets added to text of the git-config(1) man page). Cheers, Philippe. [1] https://lore.kernel.org/git/xmqqa6le5x1f.fsf_-_@gitster.g/T/#m1c2e522368ec4c9d458fcf6d83e75afab1632306 [2] https://lore.kernel.org/git/YRbYWR+X8vSq8CYe@google.com/
diff --git a/builtin/clone.c b/builtin/clone.c index 66fe66679c8..a08d9012243 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 sticky_recursive_clone; 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("submodule.stickyRecursiveClone", &sticky_recursive_clone) + && sticky_recursive_clone) { + 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..d822153e4d2 100755 --- a/t/t5606-clone-options.sh +++ b/t/t5606-clone-options.sh @@ -16,6 +16,18 @@ test_expect_success 'setup' ' ' +test_expect_success 'submodule.stickyRecursiveClone flag manipulates submodule.recurse value' ' + + test_config_global submodule.stickyRecursiveClone true && + git clone --recurse-submodules parent clone_recurse_true && + test_cmp_config -C clone_recurse_true true submodule.recurse && + + test_config_global submodule.stickyRecursiveClone 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 &&