@matheusbsbs / matheus-bagliano / commits / 5819417365

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 (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> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Glen Choo committed May 10, 2022 at 19:25 UTC 58194173652786709ba9dd1f56df6922a92f419f

2 files changed +33 -3

builtin/pull.c

+7 -3

@@ -72,6 +72,7 @@ static const char * const pull_usage[] = {

72 static int opt_verbosity;

73 static char *opt_progress;

74 static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;

75 +static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;

77 /* Options passed to git-merge or git-rebase */

78 static enum rebase_type opt_rebase = -1;

@@ -119,7 +120,7 @@ static struct option pull_options[] = {

120 N_("force progress reporting"),

121 PARSE_OPT_NOARG),

122 OPT_CALLBACK_F(0, "recurse-submodules",

122 - &recurse_submodules, N_("on-demand"),

123 + &recurse_submodules_cli, N_("on-demand"),

124 N_("control for recursive fetching of submodules"),

125 PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),

126

@@ -545,8 +546,8 @@ static int run_fetch(const char *repo, const char **refspecs)

546 strvec_push(&args, opt_tags);

547 if (opt_prune)

548 strvec_push(&args, opt_prune);

548 - if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)

549 - switch (recurse_submodules) {

549 + if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)

550 + switch (recurse_submodules_cli) {

551 case RECURSE_SUBMODULES_ON:

552 strvec_push(&args, "--recurse-submodules=on");

553 break;

@@ -939,6 +940,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)

940

941 argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);

942

943 + if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)

944 + recurse_submodules = recurse_submodules_cli;

945 +

946 if (cleanup_arg)

947 /*

948 * this only checks the validity of cleanup_arg; we don't need

t/t5572-pull-submodule.sh

+26

@@ -101,6 +101,32 @@ test_expect_success " --[no-]recurse-submodule and submodule.recurse" '

101 test_path_is_file super/sub/merge_strategy_4.t

102 '

103

104 +test_expect_success "fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)" '

105 + test_commit -C child merge_strategy_5 &&

106 + # Omit the parent commit, otherwise this passes with the

107 + # default "pull" behavior.

108 +

109 + git -C super -c fetch.recursesubmodules=true pull --no-rebase &&

110 + # Check that the submodule commit was fetched

111 + sub_oid=$(git -C child rev-parse HEAD) &&

112 + git -C super/sub cat-file -e $sub_oid &&

113 + # Check that the submodule worktree did not update

114 + ! test_path_is_file super/sub/merge_strategy_5.t

115 +'

116 +

117 +test_expect_success "fetch.recurseSubmodules takes precedence over submodule.recurse" '

118 + test_commit -C child merge_strategy_6 &&

119 + # Omit the parent commit, otherwise this passes with the

120 + # default "pull" behavior.

121 +

122 + git -C super -c submodule.recurse=false -c fetch.recursesubmodules=true pull --no-rebase &&

123 + # Check that the submodule commit was fetched

124 + sub_oid=$(git -C child rev-parse HEAD) &&

125 + git -C super/sub cat-file -e $sub_oid &&

126 + # Check that the submodule worktree did not update

127 + ! test_path_is_file super/sub/merge_strategy_6.t

128 +'

129 +

130 test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '

131 # This tests the following scenario :

132 # - local submodule has new commits