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

> Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com> 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@homann.se>
>> Signed-off-by: Nicolas Morey-Chaisemartin <nicolas@morey-chaisemartin.com>
>> ---
>>
>> 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.

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