[PATCH] pull: only pass '--recurse-submodules' to subcommands

[PATCH] pull: only pass '--recurse-submodules' to subcommands

[Glen Choo via GitGitGadget]

From: Glen Choo <chooglen@google.com>

Fix a bug in "git pull" where `submodule.recurse` is preferred over
`fetch.recurseSubmodules` (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", 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 Huang Zou for the report [1], and Philippe Blain for the initial
    investigation.
    
    This patch fixes the original bug, but not in the 'obvious' way of
    teaching "git pull" to parse fetch.recurseSubmodules. Instead, "git
    pull" now propagates its value of "--recurse-submodules" to "git fetch"
    (ignoring any config values), and leaves the config parsing to "git
    fetch".
    
    I think this works better because we get a nice separation of "config
    that git pull cares about" and "config that its subprocess care about",
    and as a result:
    
     * We don't maintain two identical config-parsing implementations 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.
    
    PS I'm having a hard time writing today, let me know how the commit
    message/cover letter can be improved :)
    
    [1]
    https://lore.kernel.org/git/CAFnZ=JNE_Sa3TsKghBPj1d0cz3kc6o91Ogj-op8o6qK8t9hPgg@mail.gmail.com
    
    In-Reply-To:
    CAFnZ=JNE_Sa3TsKghBPj1d0cz3kc6o91Ogj-op8o6qK8t9hPgg@mail.gmail.com

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1262%2Fchooglen%2Fpull%2Ffetch-recurse-submodules-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1262/chooglen/pull/fetch-recurse-submodules-v1
Pull-Request: https://github.com/git/git/pull/1262

 builtin/pull.c            | 10 +++++++---
 t/t5572-pull-submodule.sh | 14 ++++++++++++++
 2 files changed, 21 insertions(+), 3 deletions(-)

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..65aaa7927fb 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -107,6 +107,20 @@ 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 &&
+	git -C parent submodule update --remote &&
+	git -C parent add sub &&
+	git -C parent commit -m "update submodule" &&
+
+	git -C super -c fetch.recursesubmodules=true pull --no-rebase &&
+	# Check that the submodule commit was fetched
+	sub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&
+	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 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '
 	# This tests the following scenario :
 	# - local submodule has new commits

base-commit: e8005e4871f130c4e402ddca2032c111252f070a
-- 
gitgitgadget

Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands

[Junio C Hamano]

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

>     This patch fixes the original bug, but not in the 'obvious' way of
>     teaching "git pull" to parse fetch.recurseSubmodules. Instead, "git
>     pull" now propagates its value of "--recurse-submodules" to "git fetch"
>     (ignoring any config values), and leaves the config parsing to "git
>     fetch".

OK.  So the sub-git that is run in submodules will always see a
command line option, and what its configuration file says does not
matter?

>     I think this works better because we get a nice separation of "config
>     that git pull cares about" and "config that its subprocess care about",
>     and as a result:
>     
>      * We don't maintain two identical config-parsing implementations 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.
>     
>     PS I'm having a hard time writing today, let me know how the commit
>     message/cover letter can be improved :)

OK.

> 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;

This ...

>  /* 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),

... is where we keep track of what value we got from the command
line.  OK.

> @@ -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)

The fact that the variable is different from _DEFAULT is a sure sign
that we got something from the command line, because there is no way
for the command line option to set the variable to _DEFAULT (in
other words, _DEFAULT is not really a default, it is a sign that it
is not yet set to any value).  OK.

> +		switch (recurse_submodules_cli) {
>  		case RECURSE_SUBMODULES_ON:
>  			strvec_push(&args, "--recurse-submodules=on");
>  			break;

OK, so the net effect is that we only strvec_push() a command line
option to underlying "fetch" when we got a command line option.  It
does not matter what "recurse_submodules" variable is set to.  The
variable can be set via the configuration mechanism.  _cli one is
different.

OK.  And they underying "git fetch" will read its configuration as
needed anyway (if we do not do these strvec_push() here).

Sounds very sensible.  FWIW, despite what you said earlier, I find
this "if we have command line override, pass it down, otherwise they
know how to read and interpret configuration on their own" a very
sensible and intuitive approach.

Very nicely done.

> @@ -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;

This is a small fallout from the separation of the variables.
Again, _DEFAULT is not "the default behaviour whatever it is", but
is a signal "This was not set at all", and that makes this addition
correct.  At some point, we may want to rename the _DEFAULT to
_UNSPECIFIED or something for readability, but it does not have to
be a part of this fix.

> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
> index fa6b4cca65c..65aaa7927fb 100755
> --- a/t/t5572-pull-submodule.sh
> +++ b/t/t5572-pull-submodule.sh
> @@ -107,6 +107,20 @@ 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 &&
> +	git -C parent submodule update --remote &&
> +	git -C parent add sub &&
> +	git -C parent commit -m "update submodule" &&
> +
> +	git -C super -c fetch.recursesubmodules=true pull --no-rebase &&
> +	# Check that the submodule commit was fetched
> +	sub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&
> +	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 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '
>  	# This tests the following scenario :
>  	# - local submodule has new commits
>
> base-commit: e8005e4871f130c4e402ddca2032c111252f070a

Thanks, will queue.

Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands

[Junio C Hamano]

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Glen Choo <chooglen@google.com>
>
> Fix a bug in "git pull" where `submodule.recurse` is preferred over
> `fetch.recurseSubmodules` (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.
> ...

Do we know if this ever worked correctly or it once used to work but
we broke it?

>  builtin/pull.c            | 10 +++++++---
>  t/t5572-pull-submodule.sh | 14 ++++++++++++++
>  2 files changed, 21 insertions(+), 3 deletions(-)

The reason I ask is because applying this patch to maint-2.35 and
then reverting the builtin/pull.c part still allow the new test in
t5572 this patch introduces to pass.  So either it used to work
without the change to builtin/pull.c back in v2.35.x days (but in a
newer codebase we need the change to builtin/pull.c), or the new
test is not testing the alleged breakage at all.

It seems that applying only the test part to 'master' does not detect
any test failure, so perhaps the test is faulty?

> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
> index fa6b4cca65c..65aaa7927fb 100755
> --- a/t/t5572-pull-submodule.sh
> +++ b/t/t5572-pull-submodule.sh
> @@ -107,6 +107,20 @@ 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 &&
> +	git -C parent submodule update --remote &&
> +	git -C parent add sub &&
> +	git -C parent commit -m "update submodule" &&
> +
> +	git -C super -c fetch.recursesubmodules=true pull --no-rebase &&
> +	# Check that the submodule commit was fetched
> +	sub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&
> +	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 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '
>  	# This tests the following scenario :
>  	# - local submodule has new commits
>
> base-commit: e8005e4871f130c4e402ddca2032c111252f070a

Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands

[Philippe Blain]

Hi Glen,

Le 2022-05-09 à 19:27, Glen Choo via GitGitGadget a écrit :
> From: Glen Choo <chooglen@google.com>

First, regarding the commit message title (I never know where to comment
on that since I can't quote it :P)

> Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands

I understand the intent, i.e. "only pass the CLI flag, not any config, to subcommands"
but only because I already know what the patch is about. It could be
read to mean "pass --recurse-submodules only to subcommands, and not to 
something else". Since this is really a bug that affects the underlying 
'git fetch', maybe something like this ?

    pull: pass '--recurse-submodules' to 'fetch' from CLI, not config

> 
> Fix a bug in "git pull" where `submodule.recurse` is preferred over
> `fetch.recurseSubmodules` 

here I would add "for the underlying 'git 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.

nit: "or rebase".

> - 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", overriding the value of
> `fetch.recurseSubmodules`.

the wording is a litlle bit misleading here, as submodule.recurse could
be set to 'false', and then 'git fetch' will be invoked with '--recurse-submodules=false'.

> 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`.

I'm not sure of the meaning of the second bullet, since "git merge" should
never perform a fetch ?... 

> 
> 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 Huang Zou for the report [1], and Philippe Blain for the initial
>     investigation.
>     
>     This patch fixes the original bug, but not in the 'obvious' way of
>     teaching "git pull" to parse fetch.recurseSubmodules. Instead, "git
>     pull" now propagates its value of "--recurse-submodules" to "git fetch"
>     (ignoring any config values), and leaves the config parsing to "git
>     fetch".
>     
>     I think this works better because we get a nice separation of "config
>     that git pull cares about" and "config that its subprocess care about",
>     and as a result:
>     
>      * We don't maintain two identical config-parsing implementations 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.
>     
>     PS I'm having a hard time writing today, let me know how the commit
>     message/cover letter can be improved :)
>     
>     [1]
>     https://lore.kernel.org/git/CAFnZ=JNE_Sa3TsKghBPj1d0cz3kc6o91Ogj-op8o6qK8t9hPgg@mail.gmail.com
>     
>     In-Reply-To:
>     CAFnZ=JNE_Sa3TsKghBPj1d0cz3kc6o91Ogj-op8o6qK8t9hPgg@mail.gmail.com
> 
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1262%2Fchooglen%2Fpull%2Ffetch-recurse-submodules-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1262/chooglen/pull/fetch-recurse-submodules-v1
> Pull-Request: https://github.com/git/git/pull/1262
> 
>  builtin/pull.c            | 10 +++++++---
>  t/t5572-pull-submodule.sh | 14 ++++++++++++++
>  2 files changed, 21 insertions(+), 3 deletions(-)
> 
> 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;
> +

I agree with you and Junio that this implementation is very clear
and concise. Nice!

>  	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..65aaa7927fb 100755
> --- a/t/t5572-pull-submodule.sh
> +++ b/t/t5572-pull-submodule.sh
> @@ -107,6 +107,20 @@ 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 &&
> +	git -C parent submodule update --remote &&
> +	git -C parent add sub &&
> +	git -C parent commit -m "update submodule" &&
> +
> +	git -C super -c fetch.recursesubmodules=true pull --no-rebase &&
> +	# Check that the submodule commit was fetched
> +	sub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&
> +	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
> +'
> +

OK so here we test the following situation:

fetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result  pull result
-----------------------  -----------------  --------------------  ------------  ------------
true                     unset              absent                recursive     not recursive

I think this is a good start, but since the default behaviour 
of Git is the following:

fetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result  pull result
-----------------------  -----------------  --------------------  ------------  ------------
default: on-demand       default: false     unset                 on-demand     not recursive

we end up testing something not very far from the default behaviour. I think
this answers Junio's question as to why the test does not fail when applied
without the fix. So I think for completeness, we should add more tests,
including (maybe) the configuration that Huang reported:


fetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result   pull result
-----------------------  -----------------  --------------------  -------------  ------------
false                    true                absent               not recursive*  recursive

* the submodule will be fetched by 'git submodule update', not by the 'git fetch' ran by 'git pull'

and probably also this configuration that I think is what Huang wanted to achieve:

fetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result   pull result
-----------------------  -----------------  --------------------  -------------  ------------
on-demand                false               absent               on-demand      recursive

and probably this one also:

fetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result   pull result
-----------------------  -----------------  --------------------  -------------  ------------
on-demand                true               absent                on-demand      recursive

and maybe some others with an explicit '--recurse-submodules' flag on the CLI.

I must tell you that I think there is another bug I think in the 'git fetch' config parsing 
for 'fetch.recurseSubmodules' and 'submodule.recurse'. Even if 'fetch.recurseSubmodules'
is documented as overriding 'submodule.recurse', this is actually not  enforced
in 'git_fetch_config', currently it's a "last one wins" situation so the actualy behaviour
depends on which config is encountered last in the config file(s)...

Thanks for taking this on,

Philippe.

Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands

[Glen Choo]

Thanks Philippe and Junio, both! 

Philippe Blain <levraiphilippeblain@gmail.com> writes:

>>  	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..65aaa7927fb 100755
>> --- a/t/t5572-pull-submodule.sh
>> +++ b/t/t5572-pull-submodule.sh
>> @@ -107,6 +107,20 @@ 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 &&
>> +	git -C parent submodule update --remote &&
>> +	git -C parent add sub &&
>> +	git -C parent commit -m "update submodule" &&
>> +
>> +	git -C super -c fetch.recursesubmodules=true pull --no-rebase &&
>> +	# Check that the submodule commit was fetched
>> +	sub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&
>> +	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
>> +'
>> +
>
> OK so here we test the following situation:
>
> fetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result  pull result
> -----------------------  -----------------  --------------------  ------------  ------------
> true                     unset              absent                recursive     not recursive
>
> I think this is a good start, but since the default behaviour 
> of Git is the following:
>
> fetch.recurseSubmodules  submodule.recurse  --recurse-submodules  fetch result  pull result
> -----------------------  -----------------  --------------------  ------------  ------------
> default: on-demand       default: false     unset                 on-demand     not recursive
>
> we end up testing something not very far from the default behaviour.

Yes, I made a silly mistake with the test.. (I could've
sworn it made sense at the time).

- fetch.recurseSubmodules=true always worked correctly when
  submodule.recurse is not set because "git pull" realizes that the
  behavior is unspecified, so "git fetch" doesn't receive a
  "--recurse-submodules" and does the right thing.

  For completeness, we probably still want this test (even though it has
  always passed).

- I didn't test the actual buggy behavior, which is the precedence of
  the two config values.

  This would be a new test that tests something like
  fetch.recurseSubmodules=foo and submodule.recurse=bar. This should be
  enough to test precedence; I don't think we need to test more
  combinations of values.

I hope you both didn't waste too much time on this :)

Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands

[Glen Choo]

Philippe Blain <levraiphilippeblain@gmail.com> writes:

> Hi Glen,
>
> Le 2022-05-09 à 19:27, Glen Choo via GitGitGadget a écrit :
>> From: Glen Choo <chooglen@google.com>
>
> First, regarding the commit message title (I never know where to comment
> on that since I can't quote it :P)
>
>> Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands
>
> I understand the intent, i.e. "only pass the CLI flag, not any config, to subcommands"
> but only because I already know what the patch is about. It could be
> read to mean "pass --recurse-submodules only to subcommands, and not to 
> something else". Since this is really a bug that affects the underlying 
> 'git fetch', maybe something like this ?
>
>     pull: pass '--recurse-submodules' to 'fetch' from CLI, not config
>
>> 
>> Fix a bug in "git pull" where `submodule.recurse` is preferred over
>> `fetch.recurseSubmodules` 
>
> here I would add "for the underlying 'git 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.
>
> nit: "or rebase".
>
>> - 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", overriding the value of
>> `fetch.recurseSubmodules`.
>
> the wording is a litlle bit misleading here, as submodule.recurse could
> be set to 'false', and then 'git fetch' will be invoked with '--recurse-submodules=false'.

Thanks for the wording suggestions :) I'll see what I can incorporate.
 
>> 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`.
>
> I'm not sure of the meaning of the second bullet, since "git merge" should
> never perform a fetch ?... 

Ah, I'm describing a hypothetical issue with the 'obvious' (aka a
literal reading of the docs) approach of teaching "git pull" to handle
`fetch.recurseSubmodules`.

You are correct, "git merge" should never perform a fetch, so it
shouldn't care about `fetch.recurseSubmodules`. But a careless 'fix'
might be to copy what "git fetch" does with its config, e.g.


	if (!strcmp(k, "submodule.recurse")) {
		int r = git_config_bool(k, v) ?
			RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
		recurse_submodules = r;
	} else if (!strcmp(k, "fetch.recursesubmodules")) {
		recurse_submodules = parse_fetch_recurse_submodules_arg(k, v);
		return 0;
	}

which might make the internal "git merge" suddenly recurse into
submodules. Of course, this can be fixed by using a fetch-specific
variable, like:

	if (!strcmp(k, "submodule.recurse")) {
		int r = git_config_bool(k, v) ?
			RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
		recurse_submodules = r;
	}
  if (!strcmp(k, "fetch.recursesubmodules")) {
		fetch_recurse_submodules = parse_fetch_recurse_submodules_arg(k, v);
		return 0;
	}

  static int run_fetch(const char *repo, const char **refspecs)
  {
    /* ... */
    if (fetch_recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
       /* This is actually wrong wrt the docs, but assume that we could
       combine the two values here correctly. */
       recurse_submodules = fetch_recurse_submodules;
    if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
      switch (recurse_submodules) {
  }

but then we'd have to add more variables if we have
`merge.recurseSubmodules`, or `rebase.recurseSubmodules` etc.

So the easiest way is to just tell "git pull" to stop assuming that it
knows what the subcommands want :)

[PATCH v2] pull: only pass '--recurse-submodules' to subcommands

[Glen Choo via GitGitGadget]

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

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1262%2Fchooglen%2Fpull%2Ffetch-recurse-submodules-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1262/chooglen/pull/fetch-recurse-submodules-v2
Pull-Request: https://github.com/git/git/pull/1262

Range-diff vs v1:

 1:  caad7092826 ! 1:  ba08e10b759 pull: only pass '--recurse-submodules' to subcommands
     @@ Commit message
          pull: only pass '--recurse-submodules' to subcommands
      
          Fix a bug in "git pull" where `submodule.recurse` is preferred over
     -    `fetch.recurseSubmodules` (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.
     +    `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:
     @@ Commit message
            fetch".
      
          Thus, when `submodule.recurse` is set, the underlying "git fetch" gets
     -    invoked with "--recurse-submodules", overriding the value of
     +    invoked with "--recurse-submodules[=value]", overriding the value of
          `fetch.recurseSubmodules`.
      
          An alternative (and more obvious) approach to fix the bug would be to
     @@ t/t5572-pull-submodule.sh: test_expect_success " --[no-]recurse-submodule and su
       
      +test_expect_success "fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)" '
      +	test_commit -C child merge_strategy_5 &&
     -+	git -C parent submodule update --remote &&
     -+	git -C parent add sub &&
     -+	git -C parent commit -m "update submodule" &&
     ++	# 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 super rev-parse FETCH_HEAD:sub) &&
     ++	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 :


 builtin/pull.c            | 10 +++++++---
 t/t5572-pull-submodule.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

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

base-commit: e8005e4871f130c4e402ddca2032c111252f070a
-- 
gitgitgadget

Re: [PATCH v2] pull: only pass '--recurse-submodules' to subcommands

[Philippe Blain]

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.

Re: [PATCH v2] pull: only pass '--recurse-submodules' to subcommands

[Junio C Hamano]

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)?

Re: [PATCH v2] pull: only pass '--recurse-submodules' to subcommands

[Philippe Blain]

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.

Re: [PATCH v2] pull: only pass '--recurse-submodules' to subcommands

[Glen Choo]

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` :)

[PATCH v3] pull: do not let submodule.recurse override fetch.recurseSubmodules

[Glen Choo via GitGitGadget]

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: do not let submodule.recurse override fetch.recurseSubmodules
    
    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

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1262%2Fchooglen%2Fpull%2Ffetch-recurse-submodules-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1262/chooglen/pull/fetch-recurse-submodules-v3
Pull-Request: https://github.com/git/git/pull/1262

Range-diff vs v2:

 1:  ba08e10b759 ! 1:  07f052c377c pull: only pass '--recurse-submodules' to subcommands
     @@ Metadata
      Author: Glen Choo <chooglen@google.com>
      
       ## Commit message ##
     -    pull: only pass '--recurse-submodules' to subcommands
     +    pull: do not let submodule.recurse override fetch.recurseSubmodules
      
          Fix a bug in "git pull" where `submodule.recurse` is preferred over
          `fetch.recurseSubmodules` when performing a fetch


 builtin/pull.c            | 10 +++++++---
 t/t5572-pull-submodule.sh | 26 ++++++++++++++++++++++++++
 2 files changed, 33 insertions(+), 3 deletions(-)

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

base-commit: e8005e4871f130c4e402ddca2032c111252f070a
-- 
gitgitgadget

Re: [PATCH v2] pull: only pass '--recurse-submodules' to subcommands

[Junio C Hamano]

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.

Re: [PATCH v3] pull: do not let submodule.recurse override fetch.recurseSubmodules

[Junio C Hamano]

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Glen Choo <chooglen@google.com>

Let's see if there are any more inputs and start merging down to
'next'.

Thanks.

Re: [PATCH v3] pull: do not let submodule.recurse override fetch.recurseSubmodules

[Philippe Blain]

Hi Glen and Junio,

Le 2022-05-12 à 16:38, Junio C Hamano a écrit :
> "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> From: Glen Choo <chooglen@google.com>
> 
> Let's see if there are any more inputs and start merging down to
> 'next'.
> 

Well I just thought that since you just merged pb/submodule-recurse-mode-enum
into 'next', maybe Glen could make use of the new enum in his new code,
though that could also be a separate cleanup for later.

Philippe.