Nicolas Morey-Chaisemartin <nico...@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 <mag...@homann.se>
> Signed-off-by: Nicolas Morey-Chaisemartin <nico...@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.
Thanks.
