From: "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Philippe Blain <levraiphilippeblain@gmail.com>,
Huang Zou <huang.zou@schrodinger.com>,
Josh Steadmon <steadmon@google.com>,
Glen Choo <chooglen@google.com>, Glen Choo <chooglen@google.com>
Subject: [PATCH v2] pull: only pass '--recurse-submodules' to subcommands
Date: Tue, 10 May 2022 19:25:47 +0000 [thread overview]
Message-ID: <pull.1262.v2.git.git.1652210747614.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.1262.git.git.1652138854255.gitgitgadget@gmail.com>
From: Glen Choo <chooglen@google.com>
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>
---
pull: only pass '--recurse-submodules' to subcommands
Thanks for the debugging help :)
Changes since v1:
* add a test that actually tests the precedence of the config values
* I've kept the previous test; it has always worked, but it still
seems like a useful smoke test
* reworded the commit message slightly
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1262%2Fchooglen%2Fpull%2Ffetch-recurse-submodules-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1262/chooglen/pull/fetch-recurse-submodules-v2
Pull-Request: https://github.com/git/git/pull/1262
Range-diff vs v1:
1: caad7092826 ! 1: ba08e10b759 pull: only pass '--recurse-submodules' to subcommands
@@ Commit message
pull: only pass '--recurse-submodules' to subcommands
Fix a bug in "git pull" where `submodule.recurse` is preferred over
- `fetch.recurseSubmodules` (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.
+ `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:
@@ Commit message
fetch".
Thus, when `submodule.recurse` is set, the underlying "git fetch" gets
- invoked with "--recurse-submodules", overriding the value of
+ invoked with "--recurse-submodules[=value]", overriding the value of
`fetch.recurseSubmodules`.
An alternative (and more obvious) approach to fix the bug would be to
@@ t/t5572-pull-submodule.sh: test_expect_success " --[no-]recurse-submodule and su
+test_expect_success "fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)" '
+ test_commit -C child merge_strategy_5 &&
-+ git -C parent submodule update --remote &&
-+ git -C parent add sub &&
-+ git -C parent commit -m "update submodule" &&
++ # Omit the parent commit, otherwise this passes with the
++ # default "pull" behavior.
+
+ git -C super -c fetch.recursesubmodules=true pull --no-rebase &&
+ # Check that the submodule commit was fetched
-+ sub_oid=$(git -C super rev-parse FETCH_HEAD:sub) &&
++ sub_oid=$(git -C child rev-parse HEAD) &&
+ git -C super/sub cat-file -e $sub_oid &&
+ # Check that the submodule worktree did not update
+ ! test_path_is_file super/sub/merge_strategy_5.t
+'
++
++test_expect_success "fetch.recurseSubmodules takes precedence over submodule.recurse" '
++ test_commit -C child merge_strategy_6 &&
++ # Omit the parent commit, otherwise this passes with the
++ # default "pull" behavior.
++
++ git -C super -c submodule.recurse=false -c fetch.recursesubmodules=true pull --no-rebase &&
++ # Check that the submodule commit was fetched
++ sub_oid=$(git -C child rev-parse HEAD) &&
++ git -C super/sub cat-file -e $sub_oid &&
++ # Check that the submodule worktree did not update
++ ! test_path_is_file super/sub/merge_strategy_6.t
++'
+
test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '
# This tests the following scenario :
builtin/pull.c | 10 +++++++---
t/t5572-pull-submodule.sh | 26 ++++++++++++++++++++++++++
2 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/builtin/pull.c b/builtin/pull.c
index 4d667abc19d..01155ba67b2 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -72,6 +72,7 @@ static const char * const pull_usage[] = {
static int opt_verbosity;
static char *opt_progress;
static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
+static int recurse_submodules_cli = RECURSE_SUBMODULES_DEFAULT;
/* Options passed to git-merge or git-rebase */
static enum rebase_type opt_rebase = -1;
@@ -120,7 +121,7 @@ static struct option pull_options[] = {
N_("force progress reporting"),
PARSE_OPT_NOARG),
OPT_CALLBACK_F(0, "recurse-submodules",
- &recurse_submodules, N_("on-demand"),
+ &recurse_submodules_cli, N_("on-demand"),
N_("control for recursive fetching of submodules"),
PARSE_OPT_OPTARG, option_fetch_parse_recurse_submodules),
@@ -536,8 +537,8 @@ static int run_fetch(const char *repo, const char **refspecs)
strvec_push(&args, opt_tags);
if (opt_prune)
strvec_push(&args, opt_prune);
- if (recurse_submodules != RECURSE_SUBMODULES_DEFAULT)
- switch (recurse_submodules) {
+ if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
+ switch (recurse_submodules_cli) {
case RECURSE_SUBMODULES_ON:
strvec_push(&args, "--recurse-submodules=on");
break;
@@ -1001,6 +1002,9 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
+ if (recurse_submodules_cli != RECURSE_SUBMODULES_DEFAULT)
+ recurse_submodules = recurse_submodules_cli;
+
if (cleanup_arg)
/*
* this only checks the validity of cleanup_arg; we don't need
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index fa6b4cca65c..a35396fadf5 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -107,6 +107,32 @@ test_expect_success " --[no-]recurse-submodule and submodule.recurse" '
test_path_is_file super/sub/merge_strategy_4.t
'
+test_expect_success "fetch.recurseSubmodules option triggers recursive fetch (but not recursive update)" '
+ test_commit -C child merge_strategy_5 &&
+ # Omit the parent commit, otherwise this passes with the
+ # default "pull" behavior.
+
+ git -C super -c fetch.recursesubmodules=true pull --no-rebase &&
+ # Check that the submodule commit was fetched
+ sub_oid=$(git -C child rev-parse HEAD) &&
+ git -C super/sub cat-file -e $sub_oid &&
+ # Check that the submodule worktree did not update
+ ! test_path_is_file super/sub/merge_strategy_5.t
+'
+
+test_expect_success "fetch.recurseSubmodules takes precedence over submodule.recurse" '
+ test_commit -C child merge_strategy_6 &&
+ # Omit the parent commit, otherwise this passes with the
+ # default "pull" behavior.
+
+ git -C super -c submodule.recurse=false -c fetch.recursesubmodules=true pull --no-rebase &&
+ # Check that the submodule commit was fetched
+ sub_oid=$(git -C child rev-parse HEAD) &&
+ git -C super/sub cat-file -e $sub_oid &&
+ # Check that the submodule worktree did not update
+ ! test_path_is_file super/sub/merge_strategy_6.t
+'
+
test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '
# This tests the following scenario :
# - local submodule has new commits
base-commit: e8005e4871f130c4e402ddca2032c111252f070a
--
gitgitgadget
next prev parent reply other threads:[~2022-05-10 19:28 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-05-09 23:27 [PATCH] pull: only pass '--recurse-submodules' to subcommands Glen Choo via GitGitGadget
2022-05-10 0:09 ` Junio C Hamano
2022-05-10 0:44 ` Junio C Hamano
2022-05-10 13:28 ` Philippe Blain
2022-05-10 18:27 ` Glen Choo
2022-05-10 18:43 ` Glen Choo
2022-05-10 19:25 ` Glen Choo via GitGitGadget [this message]
2022-05-11 22:30 ` [PATCH v2] " Philippe Blain
2022-05-11 22:34 ` Junio C Hamano
2022-05-11 22:35 ` Philippe Blain
2022-05-11 23:21 ` Glen Choo
2022-05-12 20:37 ` Junio C Hamano
2022-05-11 23:42 ` [PATCH v3] pull: do not let submodule.recurse override fetch.recurseSubmodules Glen Choo via GitGitGadget
2022-05-12 20:38 ` Junio C Hamano
2022-05-12 23:35 ` Philippe Blain
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=pull.1262.v2.git.git.1652210747614.gitgitgadget@gmail.com \
--to=gitgitgadget@gmail.com \
--cc=chooglen@google.com \
--cc=git@vger.kernel.org \
--cc=huang.zou@schrodinger.com \
--cc=levraiphilippeblain@gmail.com \
--cc=steadmon@google.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).