{"thread":{"id":"57867","subject":"[PATCH] pull: only pass '--recurse-submodules' to subcommands","startedAt":"2022-05-09T23:27:43Z","lastAt":"2022-05-12T23:36:21Z","messageCount":15,"participants":["Glen Choo via GitGitGadget","Junio C Hamano","Philippe Blain","Glen Choo"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"455064","messageId":"pull.1262.git.git.1652138854255.gitgitgadget@gmail.com","threadId":"57867","inReplyTo":null,"subject":"[PATCH] pull: only pass '--recurse-submodules' to subcommands","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-09T23:27:34Z","receivedAt":"2022-05-09T23:27:43Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo <chooglen@google.com>\n\nFix a bug in \"git pull\" where `submodule.recurse` is preferred over\n`fetch.recurseSubmodules` (Documentation/config/fetch.txt says that\n`fetch.recurseSubmodules` should be preferred.). Do this by passing the\nvalue of the \"--recurse-submodules\" CLI option to the underlying fetch,\ninstead of passing a value that combines the CLI option and config\nvariables.\n\nIn other words, this bug occurred because builtin/pull.c is conflating\ntwo similar-sounding, but different concepts:\n\n- Whether \"git pull\" itself should care about submodules e.g. whether it\n  should update the submodule worktrees after performing a merge.\n- The value of \"--recurse-submodules\" to pass to the underlying \"git\n  fetch\".\n\nThus, when `submodule.recurse` is set, the underlying \"git fetch\" gets\ninvoked with \"--recurse-submodules\", overriding the value of\n`fetch.recurseSubmodules`.\n\nAn alternative (and more obvious) approach to fix the bug would be to\nteach \"git pull\" to understand `fetch.recurseSubmodules`, but the\nproposed solution works better because:\n\n- We don't maintain two identical config-parsing implementions in \"git\n  pull\" and \"git fetch\".\n- It works better with other commands invoked by \"git pull\" e.g. \"git\n  merge\" won't accidentally respect `fetch.recurseSubmodules`.\n\nReported-by: Huang Zou <huang.zou@schrodinger.com>\nHelped-by: Philippe Blain <levraiphilippeblain@gmail.com>\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n    pull: only pass '--recurse-submodules' to subcommands\n    \n    Thanks Huang Zou for the report [1], and Philippe Blain for the initial\n    investigation.\n    \n    This patch fixes the original bug, but not in the 'obvious' way of\n    teaching \"git pull\" to parse fetch.recurseSubmodules. Instead, \"git\n    pull\" now propagates its value of \"--recurse-submodules\" to \"git fetch\"\n    (ignoring any config values), and leaves the config parsing to \"git\n    fetch\".\n    \n    I think this works better because we get a nice separation of \"config\n    that git pull cares about\" and \"config that its subprocess care about\",\n    and as a result:\n    \n     * We don't maintain two identical config-parsing implementations in\n       \"git pull\" and \"git fetch\".\n     * It works better with other commands invoked by \"git pull\" e.g. \"git\n       merge\" won't accidentally respect fetch.recurseSubmodules.\n    \n    PS I'm having a hard time writing today, let me know how the commit\n    message/cover letter can be improved :)\n    \n    [1]\n    https://lore.kernel.org/git/CAFnZ=JNE_Sa3TsKghBPj1d0cz3kc6o91Ogj-op8o6qK8t9hPgg@mail.gmail.com\n    \n    In-Reply-To:\n    CAFnZ=JNE_Sa3TsKghBPj1d0cz3kc6o91Ogj-op8o6qK8t9hPgg@mail.gmail.com\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1262%2Fchooglen%2Fpull%2Ffetch-recurse-submodules-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1262/chooglen/pull/fetch-recurse-submodules-v1\nPull-Request: https://github.com/git/git/pull/1262\n\n builtin/pull.c            | 10 +++++++---\n t/t5572-pull-submodule.sh | 14 ++++++++++++++\n 2 files changed, 21 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 4d667abc19d..01155ba67b2 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -72,6 +72,7 @@ static const char * const pull_usage[] = {\n static int opt_verbosity;\n static char *opt_progress;\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n \n /* Options passed to git-merge or git-rebase */\n static enum rebase_type opt_rebase = -1;\n@@ -120,7 +121,7 @@ static struct option pull_options[] = {\n \t\tN_(\"force progress reporting\"),\n \t\tPARSE_OPT_NOARG),\n \tOPT_CALLBACK_F(0, \"recurse-submodules\",\n-\t\t   &recurse_submodules, N_(\"on-demand\"),\n+\t\t   &recurse_submodules_cli, N_(\"on-demand\"),\n \t\t   N_(\"control for recursive fetching of submodules\"),\n \t\t   PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n \n@@ -536,8 +537,8 @@ static int run_fetch(const char *repo, const char **refspecs)\n \t\tstrvec_push(&args, opt_tags);\n \tif (opt_prune)\n \t\tstrvec_push(&args, opt_prune);\n-\tif (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)\n-\t\tswitch (recurse_submodules) {\n+\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n+\t\tswitch (recurse_submodules_cli) {\n \t\tcase RECURSE_SUBMODULES_ON:\n \t\t\tstrvec_push(&args, \"--recurse-submodules=on\");\n \t\t\tbreak;\n@@ -1001,6 +1002,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);\n \n+\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n+\t\trecurse_submodules = recurse_submodules_cli;\n+\n \tif (cleanup_arg)\n \t\t/*\n \t\t * this only checks the validity of cleanup_arg; we don't need\ndiff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\nindex fa6b4cca65c..65aaa7927fb 100755\n--- a/t/t5572-pull-submodule.sh\n+++ b/t/t5572-pull-submodule.sh\n@@ -107,6 +107,20 @@ test_expect_success \" --[no-]recurse-submodule and submodule.recurse\" '\n \ttest_path_is_file super/sub/merge_strategy_4.t\n '\n \n+test_expect_success \"fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)\" '\n+\ttest_commit -C child merge_strategy_5 &&\n+\tgit -C parent submodule update --remote &&\n+\tgit -C parent add sub &&\n+\tgit -C parent commit -m \"update submodule\" &&\n+\n+\tgit -C super -c fetch.recursesubmodules=true pull --no-rebase &&\n+\t# Check that the submodule commit was fetched\n+\tsub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&\n+\tgit -C super/sub cat-file -e $sub_oid &&\n+\t# Check that the submodule worktree did not update\n+\t! test_path_is_file super/sub/merge_strategy_5.t\n+'\n+\n test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '\n \t# This tests the following scenario :\n \t# - local submodule has new commits\n\nbase-commit: e8005e4871f130c4e402ddca2032c111252f070a\n-- \ngitgitgadget\n"},{"id":"455067","messageId":"xmqqpmkm9rkq.fsf@gitster.g","threadId":"57867","inReplyTo":"pull.1262.git.git.1652138854255.gitgitgadget@gmail.com","subject":"Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-10T00:09:09Z","receivedAt":"2022-05-10T00:09:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Glen Choo via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n>     This patch fixes the original bug, but not in the 'obvious' way of\n>     teaching \"git pull\" to parse fetch.recurseSubmodules. Instead, \"git\n>     pull\" now propagates its value of \"--recurse-submodules\" to \"git fetch\"\n>     (ignoring any config values), and leaves the config parsing to \"git\n>     fetch\".\n\nOK.  So the sub-git that is run in submodules will always see a\ncommand line option, and what its configuration file says does not\nmatter?\n\n>     I think this works better because we get a nice separation of \"config\n>     that git pull cares about\" and \"config that its subprocess care about\",\n>     and as a result:\n>     \n>      * We don't maintain two identical config-parsing implementations in\n>        \"git pull\" and \"git fetch\".\n>      * It works better with other commands invoked by \"git pull\" e.g. \"git\n>        merge\" won't accidentally respect fetch.recurseSubmodules.\n>     \n>     PS I'm having a hard time writing today, let me know how the commit\n>     message/cover letter can be improved :)\n\nOK.\n\n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 4d667abc19d..01155ba67b2 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -72,6 +72,7 @@ static const char * const pull_usage[] = {\n>  static int opt_verbosity;\n>  static char *opt_progress;\n>  static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> +static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n\nThis ...\n\n>  /* Options passed to git-merge or git-rebase */\n>  static enum rebase_type opt_rebase = -1;\n> @@ -120,7 +121,7 @@ static struct option pull_options[] = {\n>  \t\tN_(\"force progress reporting\"),\n>  \t\tPARSE_OPT_NOARG),\n>  \tOPT_CALLBACK_F(0, \"recurse-submodules\",\n> -\t\t   &recurse_submodules, N_(\"on-demand\"),\n> +\t\t   &recurse_submodules_cli, N_(\"on-demand\"),\n>  \t\t   N_(\"control for recursive fetching of submodules\"),\n>  \t\t   PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n\n... is where we keep track of what value we got from the command\nline.  OK.\n\n> @@ -536,8 +537,8 @@ static int run_fetch(const char *repo, const char **refspecs)\n>  \t\tstrvec_push(&args, opt_tags);\n>  \tif (opt_prune)\n>  \t\tstrvec_push(&args, opt_prune);\n> -\tif (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)\n> -\t\tswitch (recurse_submodules) {\n> +\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n\nThe fact that the variable is different from _DEFAULT is a sure sign\nthat we got something from the command line, because there is no way\nfor the command line option to set the variable to _DEFAULT (in\nother words, _DEFAULT is not really a default, it is a sign that it\nis not yet set to any value).  OK.\n\n> +\t\tswitch (recurse_submodules_cli) {\n>  \t\tcase RECURSE_SUBMODULES_ON:\n>  \t\t\tstrvec_push(&args, \"--recurse-submodules=on\");\n>  \t\t\tbreak;\n\nOK, so the net effect is that we only strvec_push() a command line\noption to underlying \"fetch\" when we got a command line option.  It\ndoes not matter what \"recurse_submodules\" variable is set to.  The\nvariable can be set via the configuration mechanism.  _cli one is\ndifferent.\n\nOK.  And they underying \"git fetch\" will read its configuration as\nneeded anyway (if we do not do these strvec_push() here).\n\nSounds very sensible.  FWIW, despite what you said earlier, I find\nthis \"if we have command line override, pass it down, otherwise they\nknow how to read and interpret configuration on their own\" a very\nsensible and intuitive approach.\n\nVery nicely done.\n\n> @@ -1001,6 +1002,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>  \n>  \targc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);\n>  \n> +\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n> +\t\trecurse_submodules = recurse_submodules_cli;\n\nThis is a small fallout from the separation of the variables.\nAgain, _DEFAULT is not \"the default behaviour whatever it is\", but\nis a signal \"This was not set at all\", and that makes this addition\ncorrect.  At some point, we may want to rename the _DEFAULT to\n_UNSPECIFIED or something for readability, but it does not have to\nbe a part of this fix.\n\n> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\n> index fa6b4cca65c..65aaa7927fb 100755\n> --- a/t/t5572-pull-submodule.sh\n> +++ b/t/t5572-pull-submodule.sh\n> @@ -107,6 +107,20 @@ test_expect_success \" --[no-]recurse-submodule and submodule.recurse\" '\n>  \ttest_path_is_file super/sub/merge_strategy_4.t\n>  '\n>  \n> +test_expect_success \"fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)\" '\n> +\ttest_commit -C child merge_strategy_5 &&\n> +\tgit -C parent submodule update --remote &&\n> +\tgit -C parent add sub &&\n> +\tgit -C parent commit -m \"update submodule\" &&\n> +\n> +\tgit -C super -c fetch.recursesubmodules=true pull --no-rebase &&\n> +\t# Check that the submodule commit was fetched\n> +\tsub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&\n> +\tgit -C super/sub cat-file -e $sub_oid &&\n> +\t# Check that the submodule worktree did not update\n> +\t! test_path_is_file super/sub/merge_strategy_5.t\n> +'\n> +\n>  test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '\n>  \t# This tests the following scenario :\n>  \t# - local submodule has new commits\n>\n> base-commit: e8005e4871f130c4e402ddca2032c111252f070a\n\nThanks, will queue.\n"},{"id":"455069","messageId":"xmqq7d6u9px0.fsf@gitster.g","threadId":"57867","inReplyTo":"pull.1262.git.git.1652138854255.gitgitgadget@gmail.com","subject":"Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-10T00:44:59Z","receivedAt":"2022-05-10T00:45:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Glen Choo via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Glen Choo <chooglen@google.com>\n>\n> Fix a bug in \"git pull\" where `submodule.recurse` is preferred over\n> `fetch.recurseSubmodules` (Documentation/config/fetch.txt says that\n> `fetch.recurseSubmodules` should be preferred.). Do this by passing the\n> value of the \"--recurse-submodules\" CLI option to the underlying fetch,\n> instead of passing a value that combines the CLI option and config\n> variables.\n> ...\n\nDo we know if this ever worked correctly or it once used to work but\nwe broke it?\n\n>  builtin/pull.c            | 10 +++++++---\n>  t/t5572-pull-submodule.sh | 14 ++++++++++++++\n>  2 files changed, 21 insertions(+), 3 deletions(-)\n\nThe reason I ask is because applying this patch to maint-2.35 and\nthen reverting the builtin/pull.c part still allow the new test in\nt5572 this patch introduces to pass.  So either it used to work\nwithout the change to builtin/pull.c back in v2.35.x days (but in a\nnewer codebase we need the change to builtin/pull.c), or the new\ntest is not testing the alleged breakage at all.\n\nIt seems that applying only the test part to 'master' does not detect\nany test failure, so perhaps the test is faulty?\n\n> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\n> index fa6b4cca65c..65aaa7927fb 100755\n> --- a/t/t5572-pull-submodule.sh\n> +++ b/t/t5572-pull-submodule.sh\n> @@ -107,6 +107,20 @@ test_expect_success \" --[no-]recurse-submodule and submodule.recurse\" '\n>  \ttest_path_is_file super/sub/merge_strategy_4.t\n>  '\n>  \n> +test_expect_success \"fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)\" '\n> +\ttest_commit -C child merge_strategy_5 &&\n> +\tgit -C parent submodule update --remote &&\n> +\tgit -C parent add sub &&\n> +\tgit -C parent commit -m \"update submodule\" &&\n> +\n> +\tgit -C super -c fetch.recursesubmodules=true pull --no-rebase &&\n> +\t# Check that the submodule commit was fetched\n> +\tsub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&\n> +\tgit -C super/sub cat-file -e $sub_oid &&\n> +\t# Check that the submodule worktree did not update\n> +\t! test_path_is_file super/sub/merge_strategy_5.t\n> +'\n> +\n>  test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '\n>  \t# This tests the following scenario :\n>  \t# - local submodule has new commits\n>\n> base-commit: e8005e4871f130c4e402ddca2032c111252f070a\n"},{"id":"455081","messageId":"3234941c-5190-819f-fe3a-f528942c6b44@gmail.com","threadId":"57867","inReplyTo":"pull.1262.git.git.1652138854255.gitgitgadget@gmail.com","subject":"Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-05-10T13:28:38Z","receivedAt":"2022-05-10T13:35:44Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Glen,\n\nLe 2022-05-09 à 19:27, Glen Choo via GitGitGadget a écrit :\n> From: Glen Choo <chooglen@google.com>\n\nFirst, regarding the commit message title (I never know where to comment\non that since I can't quote it :P)\n\n> Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands\n\nI understand the intent, i.e. \"only pass the CLI flag, not any config, to subcommands\"\nbut only because I already know what the patch is about. It could be\nread to mean \"pass --recurse-submodules only to subcommands, and not to \nsomething else\". Since this is really a bug that affects the underlying \n'git fetch', maybe something like this ?\n\n    pull: pass '--recurse-submodules' to 'fetch' from CLI, not config\n\n> \n> Fix a bug in \"git pull\" where `submodule.recurse` is preferred over\n> `fetch.recurseSubmodules` \n\nhere I would add \"for the underlying 'git fetch'\"\n\n(Documentation/config/fetch.txt says that\n> `fetch.recurseSubmodules` should be preferred.). Do this by passing the\n> value of the \"--recurse-submodules\" CLI option to the underlying fetch,\n> instead of passing a value that combines the CLI option and config\n> variables.\n> \n> In other words, this bug occurred because builtin/pull.c is conflating\n> two similar-sounding, but different concepts:\n> \n> - Whether \"git pull\" itself should care about submodules e.g. whether it\n>   should update the submodule worktrees after performing a merge.\n\nnit: \"or rebase\".\n\n> - The value of \"--recurse-submodules\" to pass to the underlying \"git\n>   fetch\".\n> \n> Thus, when `submodule.recurse` is set, the underlying \"git fetch\" gets\n> invoked with \"--recurse-submodules\", overriding the value of\n> `fetch.recurseSubmodules`.\n\nthe wording is a litlle bit misleading here, as submodule.recurse could\nbe set to 'false', and then 'git fetch' will be invoked with '--recurse-submodules=false'.\n\n> An alternative (and more obvious) approach to fix the bug would be to\n> teach \"git pull\" to understand `fetch.recurseSubmodules`, but the\n> proposed solution works better because:\n> \n> - We don't maintain two identical config-parsing implementions in \"git\n>   pull\" and \"git fetch\".\n> - It works better with other commands invoked by \"git pull\" e.g. \"git\n>   merge\" won't accidentally respect `fetch.recurseSubmodules`.\n\nI'm not sure of the meaning of the second bullet, since \"git merge\" should\nnever perform a fetch ?... \n\n> \n> Reported-by: Huang Zou <huang.zou@schrodinger.com>\n> Helped-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> Signed-off-by: Glen Choo <chooglen@google.com>\n> ---\n>     pull: only pass '--recurse-submodules' to subcommands\n>     \n>     Thanks Huang Zou for the report [1], and Philippe Blain for the initial\n>     investigation.\n>     \n>     This patch fixes the original bug, but not in the 'obvious' way of\n>     teaching \"git pull\" to parse fetch.recurseSubmodules. Instead, \"git\n>     pull\" now propagates its value of \"--recurse-submodules\" to \"git fetch\"\n>     (ignoring any config values), and leaves the config parsing to \"git\n>     fetch\".\n>     \n>     I think this works better because we get a nice separation of \"config\n>     that git pull cares about\" and \"config that its subprocess care about\",\n>     and as a result:\n>     \n>      * We don't maintain two identical config-parsing implementations in\n>        \"git pull\" and \"git fetch\".\n>      * It works better with other commands invoked by \"git pull\" e.g. \"git\n>        merge\" won't accidentally respect fetch.recurseSubmodules.\n>     \n>     PS I'm having a hard time writing today, let me know how the commit\n>     message/cover letter can be improved :)\n>     \n>     [1]\n>     https://lore.kernel.org/git/CAFnZ=JNE_Sa3TsKghBPj1d0cz3kc6o91Ogj-op8o6qK8t9hPgg@mail.gmail.com\n>     \n>     In-Reply-To:\n>     CAFnZ=JNE_Sa3TsKghBPj1d0cz3kc6o91Ogj-op8o6qK8t9hPgg@mail.gmail.com\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1262%2Fchooglen%2Fpull%2Ffetch-recurse-submodules-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1262/chooglen/pull/fetch-recurse-submodules-v1\n> Pull-Request: https://github.com/git/git/pull/1262\n> \n>  builtin/pull.c            | 10 +++++++---\n>  t/t5572-pull-submodule.sh | 14 ++++++++++++++\n>  2 files changed, 21 insertions(+), 3 deletions(-)\n> \n> diff --git a/builtin/pull.c b/builtin/pull.c\n> index 4d667abc19d..01155ba67b2 100644\n> --- a/builtin/pull.c\n> +++ b/builtin/pull.c\n> @@ -72,6 +72,7 @@ static const char * const pull_usage[] = {\n>  static int opt_verbosity;\n>  static char *opt_progress;\n>  static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n> +static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n>  \n>  /* Options passed to git-merge or git-rebase */\n>  static enum rebase_type opt_rebase = -1;\n> @@ -120,7 +121,7 @@ static struct option pull_options[] = {\n>  \t\tN_(\"force progress reporting\"),\n>  \t\tPARSE_OPT_NOARG),\n>  \tOPT_CALLBACK_F(0, \"recurse-submodules\",\n> -\t\t   &recurse_submodules, N_(\"on-demand\"),\n> +\t\t   &recurse_submodules_cli, N_(\"on-demand\"),\n>  \t\t   N_(\"control for recursive fetching of submodules\"),\n>  \t\t   PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n>  \n> @@ -536,8 +537,8 @@ static int run_fetch(const char *repo, const char **refspecs)\n>  \t\tstrvec_push(&args, opt_tags);\n>  \tif (opt_prune)\n>  \t\tstrvec_push(&args, opt_prune);\n> -\tif (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)\n> -\t\tswitch (recurse_submodules) {\n> +\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n> +\t\tswitch (recurse_submodules_cli) {\n>  \t\tcase RECURSE_SUBMODULES_ON:\n>  \t\t\tstrvec_push(&args, \"--recurse-submodules=on\");\n>  \t\t\tbreak;\n> @@ -1001,6 +1002,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n>  \n>  \targc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);\n>  \n> +\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n> +\t\trecurse_submodules = recurse_submodules_cli;\n> +\n\nI agree with you and Junio that this implementation is very clear\nand concise. Nice!\n\n>  \tif (cleanup_arg)\n>  \t\t/*\n>  \t\t * this only checks the validity of cleanup_arg; we don't need\n> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\n> index fa6b4cca65c..65aaa7927fb 100755\n> --- a/t/t5572-pull-submodule.sh\n> +++ b/t/t5572-pull-submodule.sh\n> @@ -107,6 +107,20 @@ test_expect_success \" --[no-]recurse-submodule and submodule.recurse\" '\n>  \ttest_path_is_file super/sub/merge_strategy_4.t\n>  '\n>  \n> +test_expect_success \"fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)\" '\n> +\ttest_commit -C child merge_strategy_5 &&\n> +\tgit -C parent submodule update --remote &&\n> +\tgit -C parent add sub &&\n> +\tgit -C parent commit -m \"update submodule\" &&\n> +\n> +\tgit -C super -c fetch.recursesubmodules=true pull --no-rebase &&\n> +\t# Check that the submodule commit was fetched\n> +\tsub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&\n> +\tgit -C super/sub cat-file -e $sub_oid &&\n> +\t# Check that the submodule worktree did not update\n> +\t! test_path_is_file super/sub/merge_strategy_5.t\n> +'\n> +\n\nOK so here we test the following situation:\n\nfetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result  pull result\n-----------------------  -----------------  --------------------  ------------  ------------\ntrue                     unset              absent                recursive     not recursive\n\nI think this is a good start, but since the default behaviour \nof Git is the following:\n\nfetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result  pull result\n-----------------------  -----------------  --------------------  ------------  ------------\ndefault: on-demand       default: false     unset                 on-demand     not recursive\n\nwe end up testing something not very far from the default behaviour. I think\nthis answers Junio's question as to why the test does not fail when applied\nwithout the fix. So I think for completeness, we should add more tests,\nincluding (maybe) the configuration that Huang reported:\n\n\nfetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result   pull result\n-----------------------  -----------------  --------------------  -------------  ------------\nfalse                    true                absent               not recursive*  recursive\n\n* the submodule will be fetched by 'git submodule update', not by the 'git fetch' ran by 'git pull'\n\nand probably also this configuration that I think is what Huang wanted to achieve:\n\nfetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result   pull result\n-----------------------  -----------------  --------------------  -------------  ------------\non-demand                false               absent               on-demand      recursive\n\nand probably this one also:\n\nfetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result   pull result\n-----------------------  -----------------  --------------------  -------------  ------------\non-demand                true               absent                on-demand      recursive\n\nand maybe some others with an explicit '--recurse-submodules' flag on the CLI.\n\nI must tell you that I think there is another bug I think in the 'git fetch' config parsing \nfor 'fetch.recurseSubmodules' and 'submodule.recurse'. Even if 'fetch.recurseSubmodules'\nis documented as overriding 'submodule.recurse', this is actually not  enforced\nin 'git_fetch_config', currently it's a \"last one wins\" situation so the actualy behaviour\ndepends on which config is encountered last in the config file(s)...\n\nThanks for taking this on,\n\nPhilippe.\n"},{"id":"455100","messageId":"kl6l35hh8cqe.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57867","inReplyTo":"3234941c-5190-819f-fe3a-f528942c6b44@gmail.com","subject":"Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-05-10T18:27:21Z","receivedAt":"2022-05-10T18:27:36Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"\nThanks Philippe and Junio, both! \n\nPhilippe Blain <levraiphilippeblain@gmail.com> writes:\n\n>>  \tif (cleanup_arg)\n>>  \t\t/*\n>>  \t\t * this only checks the validity of cleanup_arg; we don't need\n>> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\n>> index fa6b4cca65c..65aaa7927fb 100755\n>> --- a/t/t5572-pull-submodule.sh\n>> +++ b/t/t5572-pull-submodule.sh\n>> @@ -107,6 +107,20 @@ test_expect_success \" --[no-]recurse-submodule and submodule.recurse\" '\n>>  \ttest_path_is_file super/sub/merge_strategy_4.t\n>>  '\n>>  \n>> +test_expect_success \"fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)\" '\n>> +\ttest_commit -C child merge_strategy_5 &&\n>> +\tgit -C parent submodule update --remote &&\n>> +\tgit -C parent add sub &&\n>> +\tgit -C parent commit -m \"update submodule\" &&\n>> +\n>> +\tgit -C super -c fetch.recursesubmodules=true pull --no-rebase &&\n>> +\t# Check that the submodule commit was fetched\n>> +\tsub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&\n>> +\tgit -C super/sub cat-file -e $sub_oid &&\n>> +\t# Check that the submodule worktree did not update\n>> +\t! test_path_is_file super/sub/merge_strategy_5.t\n>> +'\n>> +\n>\n> OK so here we test the following situation:\n>\n> fetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result  pull result\n> -----------------------  -----------------  --------------------  ------------  ------------\n> true                     unset              absent                recursive     not recursive\n>\n> I think this is a good start, but since the default behaviour \n> of Git is the following:\n>\n> fetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result  pull result\n> -----------------------  -----------------  --------------------  ------------  ------------\n> default: on-demand       default: false     unset                 on-demand     not recursive\n>\n> we end up testing something not very far from the default behaviour.\n\nYes, I made a silly mistake with the test.. (I could've\nsworn it made sense at the time).\n\n- fetch.recurseSubmodules=true always worked correctly when\n  submodule.recurse is not set because \"git pull\" realizes that the\n  behavior is unspecified, so \"git fetch\" doesn't receive a\n  \"--recurse-submodules\" and does the right thing.\n\n  For completeness, we probably still want this test (even though it has\n  always passed).\n\n- I didn't test the actual buggy behavior, which is the precedence of\n  the two config values.\n\n  This would be a new test that tests something like\n  fetch.recurseSubmodules=foo and submodule.recurse=bar. This should be\n  enough to test precedence; I don't think we need to test more\n  combinations of values.\n\nI hope you both didn't waste too much time on this :)\n\n"},{"id":"455102","messageId":"kl6lzgjp6xeg.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57867","inReplyTo":"3234941c-5190-819f-fe3a-f528942c6b44@gmail.com","subject":"Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-05-10T18:43:51Z","receivedAt":"2022-05-10T18:43:56Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n> Hi Glen,\n>\n> Le 2022-05-09 à 19:27, Glen Choo via GitGitGadget a écrit :\n>> From: Glen Choo <chooglen@google.com>\n>\n> First, regarding the commit message title (I never know where to comment\n> on that since I can't quote it :P)\n>\n>> Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands\n>\n> I understand the intent, i.e. \"only pass the CLI flag, not any config, to subcommands\"\n> but only because I already know what the patch is about. It could be\n> read to mean \"pass --recurse-submodules only to subcommands, and not to \n> something else\". Since this is really a bug that affects the underlying \n> 'git fetch', maybe something like this ?\n>\n>     pull: pass '--recurse-submodules' to 'fetch' from CLI, not config\n>\n>> \n>> Fix a bug in \"git pull\" where `submodule.recurse` is preferred over\n>> `fetch.recurseSubmodules` \n>\n> here I would add \"for the underlying 'git fetch'\"\n>\n> (Documentation/config/fetch.txt says that\n>> `fetch.recurseSubmodules` should be preferred.). Do this by passing the\n>> value of the \"--recurse-submodules\" CLI option to the underlying fetch,\n>> instead of passing a value that combines the CLI option and config\n>> variables.\n>> \n>> In other words, this bug occurred because builtin/pull.c is conflating\n>> two similar-sounding, but different concepts:\n>> \n>> - Whether \"git pull\" itself should care about submodules e.g. whether it\n>>   should update the submodule worktrees after performing a merge.\n>\n> nit: \"or rebase\".\n>\n>> - The value of \"--recurse-submodules\" to pass to the underlying \"git\n>>   fetch\".\n>> \n>> Thus, when `submodule.recurse` is set, the underlying \"git fetch\" gets\n>> invoked with \"--recurse-submodules\", overriding the value of\n>> `fetch.recurseSubmodules`.\n>\n> the wording is a litlle bit misleading here, as submodule.recurse could\n> be set to 'false', and then 'git fetch' will be invoked with '--recurse-submodules=false'.\n\nThanks for the wording suggestions :) I'll see what I can incorporate.\n \n>> An alternative (and more obvious) approach to fix the bug would be to\n>> teach \"git pull\" to understand `fetch.recurseSubmodules`, but the\n>> proposed solution works better because:\n>> \n>> - We don't maintain two identical config-parsing implementions in \"git\n>>   pull\" and \"git fetch\".\n>> - It works better with other commands invoked by \"git pull\" e.g. \"git\n>>   merge\" won't accidentally respect `fetch.recurseSubmodules`.\n>\n> I'm not sure of the meaning of the second bullet, since \"git merge\" should\n> never perform a fetch ?... \n\nAh, I'm describing a hypothetical issue with the 'obvious' (aka a\nliteral reading of the docs) approach of teaching \"git pull\" to handle\n`fetch.recurseSubmodules`.\n\nYou are correct, \"git merge\" should never perform a fetch, so it\nshouldn't care about `fetch.recurseSubmodules`. But a careless 'fix'\nmight be to copy what \"git fetch\" does with its config, e.g.\n\n\n\tif (!strcmp(k, \"submodule.recurse\")) {\n\t\tint r = git_config_bool(k, v) ?\n\t\t\tRECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;\n\t\trecurse_submodules = r;\n\t} else if (!strcmp(k, \"fetch.recursesubmodules\")) {\n\t\trecurse_submodules = parse_fetch_recurse_submodules_arg(k, v);\n\t\treturn 0;\n\t}\n\nwhich might make the internal \"git merge\" suddenly recurse into\nsubmodules. Of course, this can be fixed by using a fetch-specific\nvariable, like:\n\n\tif (!strcmp(k, \"submodule.recurse\")) {\n\t\tint r = git_config_bool(k, v) ?\n\t\t\tRECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;\n\t\trecurse_submodules = r;\n\t}\n  if (!strcmp(k, \"fetch.recursesubmodules\")) {\n\t\tfetch_recurse_submodules = parse_fetch_recurse_submodules_arg(k, v);\n\t\treturn 0;\n\t}\n\n  static int run_fetch(const char *repo, const char **refspecs)\n  {\n    /* ... */\n    if (fetch_recurse_submodules != RECURSE_SUBMODULES_DEFAULT)\n       /* This is actually wrong wrt the docs, but assume that we could\n       combine the two values here correctly. */\n       recurse_submodules = fetch_recurse_submodules;\n    if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)\n      switch (recurse_submodules) {\n  }\n\nbut then we'd have to add more variables if we have\n`merge.recurseSubmodules`, or `rebase.recurseSubmodules` etc.\n\nSo the easiest way is to just tell \"git pull\" to stop assuming that it\nknows what the subcommands want :)\n"},{"id":"455112","messageId":"pull.1262.v2.git.git.1652210747614.gitgitgadget@gmail.com","threadId":"57867","inReplyTo":"pull.1262.git.git.1652138854255.gitgitgadget@gmail.com","subject":"[PATCH v2] pull: only pass '--recurse-submodules' to subcommands","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-10T19:25:47Z","receivedAt":"2022-05-10T19:27:59Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo <chooglen@google.com>\n\nFix a bug in \"git pull\" where `submodule.recurse` is preferred over\n`fetch.recurseSubmodules` when performing a fetch\n(Documentation/config/fetch.txt says that `fetch.recurseSubmodules`\nshould be preferred.). Do this by passing the value of the\n\"--recurse-submodules\" CLI option to the underlying fetch, instead of\npassing a value that combines the CLI option and config variables.\n\nIn other words, this bug occurred because builtin/pull.c is conflating\ntwo similar-sounding, but different concepts:\n\n- Whether \"git pull\" itself should care about submodules e.g. whether it\n  should update the submodule worktrees after performing a merge.\n- The value of \"--recurse-submodules\" to pass to the underlying \"git\n  fetch\".\n\nThus, when `submodule.recurse` is set, the underlying \"git fetch\" gets\ninvoked with \"--recurse-submodules[=value]\", overriding the value of\n`fetch.recurseSubmodules`.\n\nAn alternative (and more obvious) approach to fix the bug would be to\nteach \"git pull\" to understand `fetch.recurseSubmodules`, but the\nproposed solution works better because:\n\n- We don't maintain two identical config-parsing implementions in \"git\n  pull\" and \"git fetch\".\n- It works better with other commands invoked by \"git pull\" e.g. \"git\n  merge\" won't accidentally respect `fetch.recurseSubmodules`.\n\nReported-by: Huang Zou <huang.zou@schrodinger.com>\nHelped-by: Philippe Blain <levraiphilippeblain@gmail.com>\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n    pull: only pass '--recurse-submodules' to subcommands\n    \n    Thanks for the debugging help :)\n    \n    Changes since v1:\n    \n     * add a test that actually tests the precedence of the config values\n       * I've kept the previous test; it has always worked, but it still\n         seems like a useful smoke test\n     * reworded the commit message slightly\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1262%2Fchooglen%2Fpull%2Ffetch-recurse-submodules-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1262/chooglen/pull/fetch-recurse-submodules-v2\nPull-Request: https://github.com/git/git/pull/1262\n\nRange-diff vs v1:\n\n 1:  caad7092826 ! 1:  ba08e10b759 pull: only pass '--recurse-submodules' to subcommands\n     @@ Commit message\n          pull: only pass '--recurse-submodules' to subcommands\n      \n          Fix a bug in \"git pull\" where `submodule.recurse` is preferred over\n     -    `fetch.recurseSubmodules` (Documentation/config/fetch.txt says that\n     -    `fetch.recurseSubmodules` should be preferred.). Do this by passing the\n     -    value of the \"--recurse-submodules\" CLI option to the underlying fetch,\n     -    instead of passing a value that combines the CLI option and config\n     -    variables.\n     +    `fetch.recurseSubmodules` when performing a fetch\n     +    (Documentation/config/fetch.txt says that `fetch.recurseSubmodules`\n     +    should be preferred.). Do this by passing the value of the\n     +    \"--recurse-submodules\" CLI option to the underlying fetch, instead of\n     +    passing a value that combines the CLI option and config variables.\n      \n          In other words, this bug occurred because builtin/pull.c is conflating\n          two similar-sounding, but different concepts:\n     @@ Commit message\n            fetch\".\n      \n          Thus, when `submodule.recurse` is set, the underlying \"git fetch\" gets\n     -    invoked with \"--recurse-submodules\", overriding the value of\n     +    invoked with \"--recurse-submodules[=value]\", overriding the value of\n          `fetch.recurseSubmodules`.\n      \n          An alternative (and more obvious) approach to fix the bug would be to\n     @@ t/t5572-pull-submodule.sh: test_expect_success \" --[no-]recurse-submodule and su\n       \n      +test_expect_success \"fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)\" '\n      +\ttest_commit -C child merge_strategy_5 &&\n     -+\tgit -C parent submodule update --remote &&\n     -+\tgit -C parent add sub &&\n     -+\tgit -C parent commit -m \"update submodule\" &&\n     ++\t# Omit the parent commit, otherwise this passes with the\n     ++\t# default \"pull\" behavior.\n      +\n      +\tgit -C super -c fetch.recursesubmodules=true pull --no-rebase &&\n      +\t# Check that the submodule commit was fetched\n     -+\tsub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&\n     ++\tsub_oid=$(git -C child rev-parse HEAD) &&\n      +\tgit -C super/sub cat-file -e $sub_oid &&\n      +\t# Check that the submodule worktree did not update\n      +\t! test_path_is_file super/sub/merge_strategy_5.t\n      +'\n     ++\n     ++test_expect_success \"fetch.recurseSubmodules takes precedence over submodule.recurse\" '\n     ++\ttest_commit -C child merge_strategy_6 &&\n     ++\t# Omit the parent commit, otherwise this passes with the\n     ++\t# default \"pull\" behavior.\n     ++\n     ++\tgit -C super -c submodule.recurse=false -c fetch.recursesubmodules=true pull --no-rebase &&\n     ++\t# Check that the submodule commit was fetched\n     ++\tsub_oid=$(git -C child rev-parse HEAD) &&\n     ++\tgit -C super/sub cat-file -e $sub_oid &&\n     ++\t# Check that the submodule worktree did not update\n     ++\t! test_path_is_file super/sub/merge_strategy_6.t\n     ++'\n      +\n       test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '\n       \t# This tests the following scenario :\n\n\n builtin/pull.c            | 10 +++++++---\n t/t5572-pull-submodule.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 4d667abc19d..01155ba67b2 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -72,6 +72,7 @@ static const char * const pull_usage[] = {\n static int opt_verbosity;\n static char *opt_progress;\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n \n /* Options passed to git-merge or git-rebase */\n static enum rebase_type opt_rebase = -1;\n@@ -120,7 +121,7 @@ static struct option pull_options[] = {\n \t\tN_(\"force progress reporting\"),\n \t\tPARSE_OPT_NOARG),\n \tOPT_CALLBACK_F(0, \"recurse-submodules\",\n-\t\t   &recurse_submodules, N_(\"on-demand\"),\n+\t\t   &recurse_submodules_cli, N_(\"on-demand\"),\n \t\t   N_(\"control for recursive fetching of submodules\"),\n \t\t   PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n \n@@ -536,8 +537,8 @@ static int run_fetch(const char *repo, const char **refspecs)\n \t\tstrvec_push(&args, opt_tags);\n \tif (opt_prune)\n \t\tstrvec_push(&args, opt_prune);\n-\tif (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)\n-\t\tswitch (recurse_submodules) {\n+\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n+\t\tswitch (recurse_submodules_cli) {\n \t\tcase RECURSE_SUBMODULES_ON:\n \t\t\tstrvec_push(&args, \"--recurse-submodules=on\");\n \t\t\tbreak;\n@@ -1001,6 +1002,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);\n \n+\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n+\t\trecurse_submodules = recurse_submodules_cli;\n+\n \tif (cleanup_arg)\n \t\t/*\n \t\t * this only checks the validity of cleanup_arg; we don't need\ndiff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\nindex fa6b4cca65c..a35396fadf5 100755\n--- a/t/t5572-pull-submodule.sh\n+++ b/t/t5572-pull-submodule.sh\n@@ -107,6 +107,32 @@ test_expect_success \" --[no-]recurse-submodule and submodule.recurse\" '\n \ttest_path_is_file super/sub/merge_strategy_4.t\n '\n \n+test_expect_success \"fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)\" '\n+\ttest_commit -C child merge_strategy_5 &&\n+\t# Omit the parent commit, otherwise this passes with the\n+\t# default \"pull\" behavior.\n+\n+\tgit -C super -c fetch.recursesubmodules=true pull --no-rebase &&\n+\t# Check that the submodule commit was fetched\n+\tsub_oid=$(git -C child rev-parse HEAD) &&\n+\tgit -C super/sub cat-file -e $sub_oid &&\n+\t# Check that the submodule worktree did not update\n+\t! test_path_is_file super/sub/merge_strategy_5.t\n+'\n+\n+test_expect_success \"fetch.recurseSubmodules takes precedence over submodule.recurse\" '\n+\ttest_commit -C child merge_strategy_6 &&\n+\t# Omit the parent commit, otherwise this passes with the\n+\t# default \"pull\" behavior.\n+\n+\tgit -C super -c submodule.recurse=false -c fetch.recursesubmodules=true pull --no-rebase &&\n+\t# Check that the submodule commit was fetched\n+\tsub_oid=$(git -C child rev-parse HEAD) &&\n+\tgit -C super/sub cat-file -e $sub_oid &&\n+\t# Check that the submodule worktree did not update\n+\t! test_path_is_file super/sub/merge_strategy_6.t\n+'\n+\n test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '\n \t# This tests the following scenario :\n \t# - local submodule has new commits\n\nbase-commit: e8005e4871f130c4e402ddca2032c111252f070a\n-- \ngitgitgadget\n"},{"id":"455182","messageId":"deafd5db-6303-46b3-a5d5-b4117ba16781@gmail.com","threadId":"57867","inReplyTo":"pull.1262.v2.git.git.1652210747614.gitgitgadget@gmail.com","subject":"Re: [PATCH v2] pull: only pass '--recurse-submodules' to subcommands","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-05-11T22:30:27Z","receivedAt":"2022-05-11T22:30:38Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Glen,\n\nLe 2022-05-10 à 15:25, Glen Choo via GitGitGadget a écrit :\n> From: Glen Choo <chooglen@google.com>\n> \n> Fix a bug in \"git pull\" where `submodule.recurse` is preferred over\n> `fetch.recurseSubmodules` when performing a fetch\n> (Documentation/config/fetch.txt says that `fetch.recurseSubmodules`\n> should be preferred.). Do this by passing the value of the\n> \"--recurse-submodules\" CLI option to the underlying fetch, instead of\n> passing a value that combines the CLI option and config variables.\n> \n> In other words, this bug occurred because builtin/pull.c is conflating\n> two similar-sounding, but different concepts:\n> \n> - Whether \"git pull\" itself should care about submodules e.g. whether it\n>   should update the submodule worktrees after performing a merge.\n> - The value of \"--recurse-submodules\" to pass to the underlying \"git\n>   fetch\".\n> \n> Thus, when `submodule.recurse` is set, the underlying \"git fetch\" gets\n> invoked with \"--recurse-submodules[=value]\", overriding the value of\n> `fetch.recurseSubmodules`.\n> \n> An alternative (and more obvious) approach to fix the bug would be to\n> teach \"git pull\" to understand `fetch.recurseSubmodules`, but the\n> proposed solution works better because:\n> \n> - We don't maintain two identical config-parsing implementions in \"git\n>   pull\" and \"git fetch\".\n> - It works better with other commands invoked by \"git pull\" e.g. \"git\n>   merge\" won't accidentally respect `fetch.recurseSubmodules`.\n> \n> Reported-by: Huang Zou <huang.zou@schrodinger.com>\n> Helped-by: Philippe Blain <levraiphilippeblain@gmail.com>\n> Signed-off-by: Glen Choo <chooglen@google.com>\n> ---\n>     pull: only pass '--recurse-submodules' to subcommands\n>     \n>     Thanks for the debugging help :)\n>     \n>     Changes since v1:\n>     \n>      * add a test that actually tests the precedence of the config values\n>        * I've kept the previous test; it has always worked, but it still\n>          seems like a useful smoke test\n>      * reworded the commit message slightly\n\nThanks, this version looks good to me. I don't feel to strongly about the \ntitle either, so as you wish :)\n\nPhilippe.\n\n"},{"id":"455183","messageId":"xmqqzgjnhf5u.fsf@gitster.g","threadId":"57867","inReplyTo":"deafd5db-6303-46b3-a5d5-b4117ba16781@gmail.com","subject":"Re: [PATCH v2] pull: only pass '--recurse-submodules' to subcommands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-11T22:34:37Z","receivedAt":"2022-05-11T22:34:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n>>     pull: only pass '--recurse-submodules' to subcommands\n>>     \n>>     Thanks for the debugging help :)\n>>     \n>>     Changes since v1:\n>>     \n>>      * add a test that actually tests the precedence of the config values\n>>        * I've kept the previous test; it has always worked, but it still\n>>          seems like a useful smoke test\n>>      * reworded the commit message slightly\n>\n> Thanks, this version looks good to me. I don't feel to strongly about the \n> title either, so as you wish :)\n\nPerhaps\n\n\tpull: do not let submodule.recurse override fetch.recurseSubmodules\n\nmore clearly expresses what it wants to do (not passing the command\nline option is \"how\" we achieve that goal)?\n"},{"id":"455184","messageId":"6e59a82c-22cc-8f0c-f7f7-94da59e8da89@gmail.com","threadId":"57867","inReplyTo":"xmqqzgjnhf5u.fsf@gitster.g","subject":"Re: [PATCH v2] pull: only pass '--recurse-submodules' to subcommands","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-05-11T22:35:33Z","receivedAt":"2022-05-11T22:35:43Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"\n\nLe 2022-05-11 à 18:34, Junio C Hamano a écrit :\n> Philippe Blain <levraiphilippeblain@gmail.com> writes:\n> \n>>>     pull: only pass '--recurse-submodules' to subcommands\n>>>     \n>>>     Thanks for the debugging help :)\n>>>     \n>>>     Changes since v1:\n>>>     \n>>>      * add a test that actually tests the precedence of the config values\n>>>        * I've kept the previous test; it has always worked, but it still\n>>>          seems like a useful smoke test\n>>>      * reworded the commit message slightly\n>>\n>> Thanks, this version looks good to me. I don't feel to strongly about the \n>> title either, so as you wish :)\n> \n> Perhaps\n> \n> \tpull: do not let submodule.recurse override fetch.recurseSubmodules\n> \n> more clearly expresses what it wants to do (not passing the command\n> line option is \"how\" we achieve that goal)?\n> \n\nI like that title, yes.\n"},{"id":"455186","messageId":"kl6ltu9vy7sc.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"57867","inReplyTo":"6e59a82c-22cc-8f0c-f7f7-94da59e8da89@gmail.com","subject":"Re: [PATCH v2] pull: only pass '--recurse-submodules' to subcommands","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-05-11T23:21:55Z","receivedAt":"2022-05-11T23:24:29Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"Philippe Blain <levraiphilippeblain@gmail.com> writes:\n\n> Le 2022-05-11 à 18:34, Junio C Hamano a écrit :\n>> Philippe Blain <levraiphilippeblain@gmail.com> writes:\n>> \n>>>>     pull: only pass '--recurse-submodules' to subcommands\n>>>>     \n>>>>     Thanks for the debugging help :)\n>>>>     \n>>>>     Changes since v1:\n>>>>     \n>>>>      * add a test that actually tests the precedence of the config values\n>>>>        * I've kept the previous test; it has always worked, but it still\n>>>>          seems like a useful smoke test\n>>>>      * reworded the commit message slightly\n>>>\n>>> Thanks, this version looks good to me. I don't feel to strongly about the \n>>> title either, so as you wish :)\n>> \n>> Perhaps\n>> \n>> \tpull: do not let submodule.recurse override fetch.recurseSubmodules\n>> \n>> more clearly expresses what it wants to do (not passing the command\n>> line option is \"how\" we achieve that goal)?\n>> \n>\n> I like that title, yes.\n\nHm, yes that makes more sense. The commit message leads with the bug, so\nit's more consistent to mention the bug in the title too.\n\nIt's arguably more _correct_ to say that passing only the CLI option is\nthe desired result, and we just so happen to be fixing a bug along the\nway. But meh, the bug is roughly 90% [1] of the motivation for making\nthis patch, so leading with the bug is ok.\n\nWill reroll.\n\n[1] The other 10% or so is some upcoming work with `git rebase\n--recurse-submodules` and supporting that in `git pull --rebase\n--recurse-submodules` :)\n"},{"id":"455187","messageId":"pull.1262.v3.git.git.1652312560207.gitgitgadget@gmail.com","threadId":"57867","inReplyTo":"pull.1262.v2.git.git.1652210747614.gitgitgadget@gmail.com","subject":"[PATCH v3] pull: do not let submodule.recurse override fetch.recurseSubmodules","fromName":"Glen Choo via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-05-11T23:42:40Z","receivedAt":"2022-05-11T23:42:49Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"From: Glen Choo <chooglen@google.com>\n\nFix a bug in \"git pull\" where `submodule.recurse` is preferred over\n`fetch.recurseSubmodules` when performing a fetch\n(Documentation/config/fetch.txt says that `fetch.recurseSubmodules`\nshould be preferred.). Do this by passing the value of the\n\"--recurse-submodules\" CLI option to the underlying fetch, instead of\npassing a value that combines the CLI option and config variables.\n\nIn other words, this bug occurred because builtin/pull.c is conflating\ntwo similar-sounding, but different concepts:\n\n- Whether \"git pull\" itself should care about submodules e.g. whether it\n  should update the submodule worktrees after performing a merge.\n- The value of \"--recurse-submodules\" to pass to the underlying \"git\n  fetch\".\n\nThus, when `submodule.recurse` is set, the underlying \"git fetch\" gets\ninvoked with \"--recurse-submodules[=value]\", overriding the value of\n`fetch.recurseSubmodules`.\n\nAn alternative (and more obvious) approach to fix the bug would be to\nteach \"git pull\" to understand `fetch.recurseSubmodules`, but the\nproposed solution works better because:\n\n- We don't maintain two identical config-parsing implementions in \"git\n  pull\" and \"git fetch\".\n- It works better with other commands invoked by \"git pull\" e.g. \"git\n  merge\" won't accidentally respect `fetch.recurseSubmodules`.\n\nReported-by: Huang Zou <huang.zou@schrodinger.com>\nHelped-by: Philippe Blain <levraiphilippeblain@gmail.com>\nSigned-off-by: Glen Choo <chooglen@google.com>\n---\n    pull: do not let submodule.recurse override fetch.recurseSubmodules\n    \n    Thanks for the debugging help :)\n    \n    Changes since v1:\n    \n     * add a test that actually tests the precedence of the config values\n       * I've kept the previous test; it has always worked, but it still\n         seems like a useful smoke test\n     * reworded the commit message slightly\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1262%2Fchooglen%2Fpull%2Ffetch-recurse-submodules-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1262/chooglen/pull/fetch-recurse-submodules-v3\nPull-Request: https://github.com/git/git/pull/1262\n\nRange-diff vs v2:\n\n 1:  ba08e10b759 ! 1:  07f052c377c pull: only pass '--recurse-submodules' to subcommands\n     @@ Metadata\n      Author: Glen Choo <chooglen@google.com>\n      \n       ## Commit message ##\n     -    pull: only pass '--recurse-submodules' to subcommands\n     +    pull: do not let submodule.recurse override fetch.recurseSubmodules\n      \n          Fix a bug in \"git pull\" where `submodule.recurse` is preferred over\n          `fetch.recurseSubmodules` when performing a fetch\n\n\n builtin/pull.c            | 10 +++++++---\n t/t5572-pull-submodule.sh | 26 ++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 4d667abc19d..01155ba67b2 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -72,6 +72,7 @@ static const char * const pull_usage[] = {\n static int opt_verbosity;\n static char *opt_progress;\n static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;\n \n /* Options passed to git-merge or git-rebase */\n static enum rebase_type opt_rebase = -1;\n@@ -120,7 +121,7 @@ static struct option pull_options[] = {\n \t\tN_(\"force progress reporting\"),\n \t\tPARSE_OPT_NOARG),\n \tOPT_CALLBACK_F(0, \"recurse-submodules\",\n-\t\t   &recurse_submodules, N_(\"on-demand\"),\n+\t\t   &recurse_submodules_cli, N_(\"on-demand\"),\n \t\t   N_(\"control for recursive fetching of submodules\"),\n \t\t   PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),\n \n@@ -536,8 +537,8 @@ static int run_fetch(const char *repo, const char **refspecs)\n \t\tstrvec_push(&args, opt_tags);\n \tif (opt_prune)\n \t\tstrvec_push(&args, opt_prune);\n-\tif (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)\n-\t\tswitch (recurse_submodules) {\n+\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n+\t\tswitch (recurse_submodules_cli) {\n \t\tcase RECURSE_SUBMODULES_ON:\n \t\t\tstrvec_push(&args, \"--recurse-submodules=on\");\n \t\t\tbreak;\n@@ -1001,6 +1002,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \targc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);\n \n+\tif (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)\n+\t\trecurse_submodules = recurse_submodules_cli;\n+\n \tif (cleanup_arg)\n \t\t/*\n \t\t * this only checks the validity of cleanup_arg; we don't need\ndiff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\nindex fa6b4cca65c..a35396fadf5 100755\n--- a/t/t5572-pull-submodule.sh\n+++ b/t/t5572-pull-submodule.sh\n@@ -107,6 +107,32 @@ test_expect_success \" --[no-]recurse-submodule and submodule.recurse\" '\n \ttest_path_is_file super/sub/merge_strategy_4.t\n '\n \n+test_expect_success \"fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)\" '\n+\ttest_commit -C child merge_strategy_5 &&\n+\t# Omit the parent commit, otherwise this passes with the\n+\t# default \"pull\" behavior.\n+\n+\tgit -C super -c fetch.recursesubmodules=true pull --no-rebase &&\n+\t# Check that the submodule commit was fetched\n+\tsub_oid=$(git -C child rev-parse HEAD) &&\n+\tgit -C super/sub cat-file -e $sub_oid &&\n+\t# Check that the submodule worktree did not update\n+\t! test_path_is_file super/sub/merge_strategy_5.t\n+'\n+\n+test_expect_success \"fetch.recurseSubmodules takes precedence over submodule.recurse\" '\n+\ttest_commit -C child merge_strategy_6 &&\n+\t# Omit the parent commit, otherwise this passes with the\n+\t# default \"pull\" behavior.\n+\n+\tgit -C super -c submodule.recurse=false -c fetch.recursesubmodules=true pull --no-rebase &&\n+\t# Check that the submodule commit was fetched\n+\tsub_oid=$(git -C child rev-parse HEAD) &&\n+\tgit -C super/sub cat-file -e $sub_oid &&\n+\t# Check that the submodule worktree did not update\n+\t! test_path_is_file super/sub/merge_strategy_6.t\n+'\n+\n test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '\n \t# This tests the following scenario :\n \t# - local submodule has new commits\n\nbase-commit: e8005e4871f130c4e402ddca2032c111252f070a\n-- \ngitgitgadget\n"},{"id":"455216","messageId":"xmqqzgjmebc4.fsf@gitster.g","threadId":"57867","inReplyTo":"kl6ltu9vy7sc.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH v2] pull: only pass '--recurse-submodules' to subcommands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-12T20:37:47Z","receivedAt":"2022-05-12T20:37:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Glen Choo <chooglen@google.com> writes:\n\n> Hm, yes that makes more sense. The commit message leads with the bug, so\n> it's more consistent to mention the bug in the title too.\n>\n> It's arguably more _correct_ to say that passing only the CLI option is\n> the desired result,\n\nCorrect.\n\nIf it were clear that the title meant \"teach 'git pull' to try\naffecting the recursive behaviour of subcommands only when it got\n'--recurse-submodules' on its command line\", I wouldn't have waited\nfor retitling.\n\nIt however is easy to misread the original title in such a way that\nthe mention of '--recurse-submodules' refers to the fact that 'pull'\ninvokes 'fetch' and passes the \"--recurse-submodules\" command line\noption, and \"only\" incorrectly hints that 'pull' passes only that\noption and no other option from the command line when it does so.\n\nThat will be puzzling because (1) that is clearly not what we do in\ntoday's code, suggesting this is a totally different change from\nwhat we have in the patch, and (2) it is not clear what we want to\nachieve by changing the code in such a drastic way to stop passing\n\"--all\" and other options to \"fetch\".\n\nSo, it is arguable, but the original title does not say what we\nwanted it to say, exactly.\n\n"},{"id":"455217","messageId":"xmqqv8uaebav.fsf@gitster.g","threadId":"57867","inReplyTo":"pull.1262.v3.git.git.1652312560207.gitgitgadget@gmail.com","subject":"Re: [PATCH v3] pull: do not let submodule.recurse override fetch.recurseSubmodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-05-12T20:38:32Z","receivedAt":"2022-05-12T20:38:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Glen Choo via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n\n> From: Glen Choo <chooglen@google.com>\n\nLet's see if there are any more inputs and start merging down to\n'next'.\n\nThanks.\n\n"},{"id":"455237","messageId":"14349c04-6879-6fd6-30f0-4272a627b93d@gmail.com","threadId":"57867","inReplyTo":"xmqqv8uaebav.fsf@gitster.g","subject":"Re: [PATCH v3] pull: do not let submodule.recurse override fetch.recurseSubmodules","fromName":"Philippe Blain","fromEmail":"levraiphilippeblain@gmail.com","sentAt":"2022-05-12T23:35:53Z","receivedAt":"2022-05-12T23:36:21Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"Hi Glen and Junio,\n\nLe 2022-05-12 à 16:38, Junio C Hamano a écrit :\n> \"Glen Choo via GitGitGadget\" <gitgitgadget@gmail.com> writes:\n> \n>> From: Glen Choo <chooglen@google.com>\n> \n> Let's see if there are any more inputs and start merging down to\n> 'next'.\n> \n\nWell I just thought that since you just merged pb/submodule-recurse-mode-enum\ninto 'next', maybe Glen could make use of the new enum in his new code,\nthough that could also be a separate cleanup for later.\n\nPhilippe.\n"}]}