| Message ID | pull.1262.v2.git.git.1652210747614.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 58194173652786709ba9dd1f56df6922a92f419f |
| Headers | show |
| Series | [v2] pull: only pass '--recurse-submodules' to subcommands | expand |
Hi Glen, Le 2022-05-10 à 15:25, Glen Choo via GitGitGadget a écrit : > From: Glen Choo <chooglen@google.com> > > Fix a bug in "git pull" where `submodule.recurse` is preferred over > `fetch.recurseSubmodules` when performing a fetch > (Documentation/config/fetch.txt says that `fetch.recurseSubmodules` > should be preferred.). Do this by passing the value of the > "--recurse-submodules" CLI option to the underlying fetch, instead of > passing a value that combines the CLI option and config variables. > > In other words, this bug occurred because builtin/pull.c is conflating > two similar-sounding, but different concepts: > > - Whether "git pull" itself should care about submodules e.g. whether it > should update the submodule worktrees after performing a merge. > - The value of "--recurse-submodules" to pass to the underlying "git > fetch". > > Thus, when `submodule.recurse` is set, the underlying "git fetch" gets > invoked with "--recurse-submodules[=value]", overriding the value of > `fetch.recurseSubmodules`. > > An alternative (and more obvious) approach to fix the bug would be to > teach "git pull" to understand `fetch.recurseSubmodules`, but the > proposed solution works better because: > > - We don't maintain two identical config-parsing implementions in "git > pull" and "git fetch". > - It works better with other commands invoked by "git pull" e.g. "git > merge" won't accidentally respect `fetch.recurseSubmodules`. > > Reported-by: Huang Zou <huang.zou@schrodinger.com> > Helped-by: Philippe Blain <levraiphilippeblain@gmail.com> > Signed-off-by: Glen Choo <chooglen@google.com> > --- > pull: only pass '--recurse-submodules' to subcommands > > Thanks for the debugging help :) > > Changes since v1: > > * add a test that actually tests the precedence of the config values > * I've kept the previous test; it has always worked, but it still > seems like a useful smoke test > * reworded the commit message slightly Thanks, this version looks good to me. I don't feel to strongly about the title either, so as you wish :) Philippe.
Philippe Blain <levraiphilippeblain@gmail.com> writes: >> pull: only pass '--recurse-submodules' to subcommands >> >> Thanks for the debugging help :) >> >> Changes since v1: >> >> * add a test that actually tests the precedence of the config values >> * I've kept the previous test; it has always worked, but it still >> seems like a useful smoke test >> * reworded the commit message slightly > > Thanks, this version looks good to me. I don't feel to strongly about the > title either, so as you wish :) Perhaps pull: do not let submodule.recurse override fetch.recurseSubmodules more clearly expresses what it wants to do (not passing the command line option is "how" we achieve that goal)?
Le 2022-05-11 à 18:34, Junio C Hamano a écrit : > Philippe Blain <levraiphilippeblain@gmail.com> writes: > >>> pull: only pass '--recurse-submodules' to subcommands >>> >>> Thanks for the debugging help :) >>> >>> Changes since v1: >>> >>> * add a test that actually tests the precedence of the config values >>> * I've kept the previous test; it has always worked, but it still >>> seems like a useful smoke test >>> * reworded the commit message slightly >> >> Thanks, this version looks good to me. I don't feel to strongly about the >> title either, so as you wish :) > > Perhaps > > pull: do not let submodule.recurse override fetch.recurseSubmodules > > more clearly expresses what it wants to do (not passing the command > line option is "how" we achieve that goal)? > I like that title, yes.
Philippe Blain <levraiphilippeblain@gmail.com> writes: > Le 2022-05-11 à 18:34, Junio C Hamano a écrit : >> Philippe Blain <levraiphilippeblain@gmail.com> writes: >> >>>> pull: only pass '--recurse-submodules' to subcommands >>>> >>>> Thanks for the debugging help :) >>>> >>>> Changes since v1: >>>> >>>> * add a test that actually tests the precedence of the config values >>>> * I've kept the previous test; it has always worked, but it still >>>> seems like a useful smoke test >>>> * reworded the commit message slightly >>> >>> Thanks, this version looks good to me. I don't feel to strongly about the >>> title either, so as you wish :) >> >> Perhaps >> >> pull: do not let submodule.recurse override fetch.recurseSubmodules >> >> more clearly expresses what it wants to do (not passing the command >> line option is "how" we achieve that goal)? >> > > I like that title, yes. Hm, yes that makes more sense. The commit message leads with the bug, so it's more consistent to mention the bug in the title too. It's arguably more _correct_ to say that passing only the CLI option is the desired result, and we just so happen to be fixing a bug along the way. But meh, the bug is roughly 90% [1] of the motivation for making this patch, so leading with the bug is ok. Will reroll. [1] The other 10% or so is some upcoming work with `git rebase --recurse-submodules` and supporting that in `git pull --rebase --recurse-submodules` :)
Glen Choo <chooglen@google.com> writes: > Hm, yes that makes more sense. The commit message leads with the bug, so > it's more consistent to mention the bug in the title too. > > It's arguably more _correct_ to say that passing only the CLI option is > the desired result, Correct. If it were clear that the title meant "teach 'git pull' to try affecting the recursive behaviour of subcommands only when it got '--recurse-submodules' on its command line", I wouldn't have waited for retitling. It however is easy to misread the original title in such a way that the mention of '--recurse-submodules' refers to the fact that 'pull' invokes 'fetch' and passes the "--recurse-submodules" command line option, and "only" incorrectly hints that 'pull' passes only that option and no other option from the command line when it does so. That will be puzzling because (1) that is clearly not what we do in today's code, suggesting this is a totally different change from what we have in the patch, and (2) it is not clear what we want to achieve by changing the code in such a drastic way to stop passing "--all" and other options to "fetch". So, it is arguable, but the original title does not say what we wanted it to say, exactly.
diff --git a/builtin/pull.c b/builtin/pull.c index 4d667abc19d..01155ba67b2 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -72,6 +72,7 @@ static const char * const pull_usage[] = { static int opt_verbosity; static char *opt_progress; static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT; +static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT; /* Options passed to git-merge or git-rebase */ static enum rebase_type opt_rebase = -1; @@ -120,7 +121,7 @@ static struct option pull_options[] = { N_("force progress reporting"), PARSE_OPT_NOARG), OPT_CALLBACK_F(0, "recurse-submodules", - &recurse_submodules, N_("on-demand"), + &recurse_submodules_cli, N_("on-demand"), N_("control for recursive fetching of submodules"), PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules), @@ -536,8 +537,8 @@ static int run_fetch(const char *repo, const char **refspecs) strvec_push(&args, opt_tags); if (opt_prune) strvec_push(&args, opt_prune); - if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT) - switch (recurse_submodules) { + if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT) + switch (recurse_submodules_cli) { case RECURSE_SUBMODULES_ON: strvec_push(&args, "--recurse-submodules=on"); break; @@ -1001,6 +1002,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0); + if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT) + recurse_submodules = recurse_submodules_cli; + if (cleanup_arg) /* * this only checks the validity of cleanup_arg; we don't need diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh index fa6b4cca65c..a35396fadf5 100755 --- a/t/t5572-pull-submodule.sh +++ b/t/t5572-pull-submodule.sh @@ -107,6 +107,32 @@ test_expect_success " --[no-]recurse-submodule and submodule.recurse" ' test_path_is_file super/sub/merge_strategy_4.t ' +test_expect_success "fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)" ' + test_commit -C child merge_strategy_5 && + # Omit the parent commit, otherwise this passes with the + # default "pull" behavior. + + git -C super -c fetch.recursesubmodules=true pull --no-rebase && + # Check that the submodule commit was fetched + sub_oid=$(git -C child rev-parse HEAD) && + git -C super/sub cat-file -e $sub_oid && + # Check that the submodule worktree did not update + ! test_path_is_file super/sub/merge_strategy_5.t +' + +test_expect_success "fetch.recurseSubmodules takes precedence over submodule.recurse" ' + test_commit -C child merge_strategy_6 && + # Omit the parent commit, otherwise this passes with the + # default "pull" behavior. + + git -C super -c submodule.recurse=false -c fetch.recursesubmodules=true pull --no-rebase && + # Check that the submodule commit was fetched + sub_oid=$(git -C child rev-parse HEAD) && + git -C super/sub cat-file -e $sub_oid && + # Check that the submodule worktree did not update + ! test_path_is_file super/sub/merge_strategy_6.t +' + test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' ' # This tests the following scenario : # - local submodule has new commits