Le 06/09/2017 à 03:17, Junio C Hamano a écrit :
> Nicolas Morey-Chaisemartin <nicolas@xxxxxxxxxxxxxxxxxxxxxx> 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 <magnus@xxxxxxxxx>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@xxxxxxxxxxxxxxxxxxxxxx>
>> ---
>>
>> 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=<value>" 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=<value> 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
