Re: [PATCHv2] pull: honor submodule.recurse config option

[Junio C Hamano]

Nicolas Morey-Chaisemartin  writes:

>> If it is not the latter, perhaps we may want to flip the order of
>> config parsing and option parsing around?  That will allow us to fix
>> the handling of autostash thing to use only one variable, and also
>> fix your patch to do the right thing.
>
> I see what you mean.

> It looks like switching the code around works but I think there
> still needs to be 2 variables for autstash for this piece of code:
>
> if (!opt_rebase && opt_autostash != -1)
> die(_("--[no-]autostash option is only valid with --rebase."));
>
> The config option should not cause git pull to die when not using
> --rebase, the CLI option should.

Ah, OK.  That is a worthwhile observation that needs to be recorded
in the log message of a commit that flips the order of option/config
parsing.

Thanks.

Re: [PATCHv2] pull: honor submodule.recurse config option

[Nicolas Morey-Chaisemartin]

Le 06/09/2017 à 03:17, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin  writes:
>
>> "git pull" supports a --recurse-submodules option but does not parse the
>> submodule.recurse configuration item to set the default for that option.
>> Meanwhile "git fetch" does support submodule.recurse, producing
>> confusing behavior: when submodule.recurse is enabled, "git pull"
>> recursively fetches submodules but does not update them after fetch.
>>
>> Handle submodule.recurse in "git pull" to fix this.
>>
>> Reported-by: Magnus Homann 
>> Signed-off-by: Nicolas Morey-Chaisemartin 
>> ---
>>
>> Changes since v1:
>>  * Cleanup commit message
>>  * Add test
>>  * Remove extra var in code and fallthrough to git_default_config
>>
>>  builtin/pull.c|  4 
>>  t/t5572-pull-submodule.sh | 10 ++
>>  2 files changed, 14 insertions(+)
>>
>> diff --git a/builtin/pull.c b/builtin/pull.c
>> index 7fe281414..ce8ccb15b 100644
>> --- a/builtin/pull.c
>> +++ b/builtin/pull.c
>> @@ -325,6 +325,10 @@ static int git_pull_config(const char *var, const char 
>> *value, void *cb)
>>  if (!strcmp(var, "rebase.autostash")) {
>>  config_autostash = git_config_bool(var, value);
>>  return 0;
>> +} else if (!strcmp(var, "submodule.recurse")) {
>> +recurse_submodules = git_config_bool(var, value) ?
>> +RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
>> +return 0;
>>  }
>>  return git_default_config(var, value, cb);
>>  }
> If I am reading the existing code correctly, things happen in
> cmd_pull() in this order:
>
>  - recurse_submodules is a file-scope static that is initialized to
>RECURSE_SUBMODULES_DEFAULT
>
>  - pull_options[] is given to parse_options() so that
>submodule-config.c::option_fetch_parse_recurse_submodules() can
>read "--recurse-submodules=" from the command line to
>update recurse_submodules.
>
>  - git_pull_config() is given to git_config() so that settings in
>the configuration files are read.
>
> Care must be taken to make sure that values given from the command
> line is never overriden by the default value specified in the
> configuration system because the order of the second and third items
> in the above are backwards from the usual flow.  This patch does not
> seem to have any such provision.
>
> Existing handling of "--autostash" vs "rebase.autostash" solves this
> issue by having opt_autostash and config_autostash as two separate
> variables, so I suspect that something similar to it must be there,
> at least, for this new configuration.
>
> If we want to keep the current code structure, that is.  I do not
> recall if we did not notice the fact that the order of options and
> config parsing is backwards and unknowingly worked it around with
> two variables when we added the rebase.autostash thing, or we knew
> the order was unusual but there was a good reason to keep that
> unusual order (iow, if we simply swapped the order of
> parse_options() and git_config() calls, there are things that will
> break).  
>
> If it is not the latter, perhaps we may want to flip the order of
> config parsing and option parsing around?  That will allow us to fix
> the handling of autostash thing to use only one variable, and also
> fix your patch to do the right thing.

I see what you mean.
It looks like switching the code around works but I think there still needs to 
be 2variables for autstash for this piece of code:

    if (!opt_rebase && opt_autostash != -1)
        die(_("--[no-]autostash option is only valid with --rebase."));

The config option should not cause git pull to die when not using --rebase, the 
CLI option should.

>> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
>> index 077eb07e1..1b3a3f445 100755
>> --- a/t/t5572-pull-submodule.sh
>> +++ b/t/t5572-pull-submodule.sh
>> @@ -65,6 +65,16 @@ test_expect_success 'recursive pull updates working tree' 
>> '
>>  test_path_is_file super/sub/merge_strategy.t
>>  '
>>  
>> +test_expect_success "submodule.recurse option triggers recursive pull" '
>> +test_commit -C child merge_strategy_2 &&
>> +git -C parent submodule update --remote &&
>> +git -C parent add sub &&
>> +git -C parent commit -m "update submodule" &&
>> +
>> +git -C super -c submodule.recurse pull --no-rebase &&
>> +test_path_is_file super/sub/merge_strategy_2.t
>> +'
> This new test does not test interactions with submodule.recurse
> configuration and --recurse-submodules= from the command
> line.  It would be necessary to add tests to cover the permutations
> in addition to the basic test we see above.

Will fix

Thanks

Nicolas

Re: [PATCHv2] pull: honor submodule.recurse config option

[Junio C Hamano]

Nicolas Morey-Chaisemartin  writes:

> "git pull" supports a --recurse-submodules option but does not parse the
> submodule.recurse configuration item to set the default for that option.
> Meanwhile "git fetch" does support submodule.recurse, producing
> confusing behavior: when submodule.recurse is enabled, "git pull"
> recursively fetches submodules but does not update them after fetch.
>
> Handle submodule.recurse in "git pull" to fix this.
>
> Reported-by: Magnus Homann 
> Signed-off-by: Nicolas Morey-Chaisemartin 
> ---
>
> Changes since v1:
>  * Cleanup commit message
>  * Add test
>  * Remove extra var in code and fallthrough to git_default_config
>
>  builtin/pull.c|  4 
>  t/t5572-pull-submodule.sh | 10 ++
>  2 files changed, 14 insertions(+)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 7fe281414..ce8ccb15b 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -325,6 +325,10 @@ static int git_pull_config(const char *var, const char 
> *value, void *cb)
>   if (!strcmp(var, "rebase.autostash")) {
>   config_autostash = git_config_bool(var, value);
>   return 0;
> + } else if (!strcmp(var, "submodule.recurse")) {
> + recurse_submodules = git_config_bool(var, value) ?
> + RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
> + return 0;
>   }
>   return git_default_config(var, value, cb);
>  }

If I am reading the existing code correctly, things happen in
cmd_pull() in this order:

 - recurse_submodules is a file-scope static that is initialized to
   RECURSE_SUBMODULES_DEFAULT

 - pull_options[] is given to parse_options() so that
   submodule-config.c::option_fetch_parse_recurse_submodules() can
   read "--recurse-submodules=" from the command line to
   update recurse_submodules.

 - git_pull_config() is given to git_config() so that settings in
   the configuration files are read.

Care must be taken to make sure that values given from the command
line is never overriden by the default value specified in the
configuration system because the order of the second and third items
in the above are backwards from the usual flow.  This patch does not
seem to have any such provision.

Existing handling of "--autostash" vs "rebase.autostash" solves this
issue by having opt_autostash and config_autostash as two separate
variables, so I suspect that something similar to it must be there,
at least, for this new configuration.

If we want to keep the current code structure, that is.  I do not
recall if we did not notice the fact that the order of options and
config parsing is backwards and unknowingly worked it around with
two variables when we added the rebase.autostash thing, or we knew
the order was unusual but there was a good reason to keep that
unusual order (iow, if we simply swapped the order of
parse_options() and git_config() calls, there are things that will
break).  

If it is not the latter, perhaps we may want to flip the order of
config parsing and option parsing around?  That will allow us to fix
the handling of autostash thing to use only one variable, and also
fix your patch to do the right thing.

> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
> index 077eb07e1..1b3a3f445 100755
> --- a/t/t5572-pull-submodule.sh
> +++ b/t/t5572-pull-submodule.sh
> @@ -65,6 +65,16 @@ test_expect_success 'recursive pull updates working tree' '
>   test_path_is_file super/sub/merge_strategy.t
>  '
>  
> +test_expect_success "submodule.recurse option triggers recursive pull" '
> + test_commit -C child merge_strategy_2 &&
> + git -C parent submodule update --remote &&
> + git -C parent add sub &&
> + git -C parent commit -m "update submodule" &&
> +
> + git -C super -c submodule.recurse pull --no-rebase &&
> + test_path_is_file super/sub/merge_strategy_2.t
> +'

This new test does not test interactions with submodule.recurse
configuration and --recurse-submodules= from the command
line.  It would be necessary to add tests to cover the permutations
in addition to the basic test we see above.

Thanks.

[PATCHv2] pull: honor submodule.recurse config option

[Nicolas Morey-Chaisemartin]

"git pull" supports a --recurse-submodules option but does not parse the
submodule.recurse configuration item to set the default for that option.
Meanwhile "git fetch" does support submodule.recurse, producing
confusing behavior: when submodule.recurse is enabled, "git pull"
recursively fetches submodules but does not update them after fetch.

Handle submodule.recurse in "git pull" to fix this.

Reported-by: Magnus Homann 
Signed-off-by: Nicolas Morey-Chaisemartin 
---

Changes since v1:
 * Cleanup commit message
 * Add test
 * Remove extra var in code and fallthrough to git_default_config

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

diff --git a/builtin/pull.c b/builtin/pull.c
index 7fe281414..ce8ccb15b 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -325,6 +325,10 @@ static int git_pull_config(const char *var, const char 
*value, void *cb)
if (!strcmp(var, "rebase.autostash")) {
config_autostash = git_config_bool(var, value);
return 0;
+   } else if (!strcmp(var, "submodule.recurse")) {
+   recurse_submodules = git_config_bool(var, value) ?
+   RECURSE_SUBMODULES_ON : RECURSE_SUBMODULES_OFF;
+   return 0;
}
return git_default_config(var, value, cb);
 }
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index 077eb07e1..1b3a3f445 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -65,6 +65,16 @@ test_expect_success 'recursive pull updates working tree' '
test_path_is_file super/sub/merge_strategy.t
 '
 
+test_expect_success "submodule.recurse option triggers recursive pull" '
+   test_commit -C child merge_strategy_2 &&
+   git -C parent submodule update --remote &&
+   git -C parent add sub &&
+   git -C parent commit -m "update submodule" &&
+
+   git -C super -c submodule.recurse pull --no-rebase &&
+   test_path_is_file super/sub/merge_strategy_2.t
+'
+
 test_expect_success 'recursive rebasing pull' '
# change upstream
test_commit -C child rebase_strategy &&
-- 
2.14.1.460.g695108176