git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Glen Choo via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	Philippe Blain <levraiphilippeblain@gmail.com>,
	Huang Zou <huang.zou@schrodinger.com>,
	Josh Steadmon <steadmon@google.com>,
	Glen Choo <chooglen@google.com>
Subject: Re: [PATCH] pull: only pass '--recurse-submodules' to subcommands
Date: Mon, 09 May 2022 17:44:59 -0700	[thread overview]
Message-ID: <xmqq7d6u9px0.fsf@gitster.g> (raw)
In-Reply-To: <pull.1262.git.git.1652138854255.gitgitgadget@gmail.com> (Glen Choo via GitGitGadget's message of "Mon, 09 May 2022 23:27:34 +0000")

"Glen Choo via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Glen Choo <chooglen@google.com>
>
> 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.
> ...

Do we know if this ever worked correctly or it once used to work but
we broke it?

>  builtin/pull.c            | 10 +++++++---
>  t/t5572-pull-submodule.sh | 14 ++++++++++++++
>  2 files changed, 21 insertions(+), 3 deletions(-)

The reason I ask is because applying this patch to maint-2.35 and
then reverting the builtin/pull.c part still allow the new test in
t5572 this patch introduces to pass.  So either it used to work
without the change to builtin/pull.c back in v2.35.x days (but in a
newer codebase we need the change to builtin/pull.c), or the new
test is not testing the alleged breakage at all.

It seems that applying only the test part to 'master' does not detect
any test failure, so perhaps the test is faulty?

> diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
> index fa6b4cca65c..65aaa7927fb 100755
> --- a/t/t5572-pull-submodule.sh
> +++ b/t/t5572-pull-submodule.sh
> @@ -107,6 +107,20 @@ 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 &&
> +	git -C parent submodule update --remote &&
> +	git -C parent add sub &&
> +	git -C parent commit -m "update submodule" &&
> +
> +	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) &&
> +	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 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '
>  	# This tests the following scenario :
>  	# - local submodule has new commits
>
> base-commit: e8005e4871f130c4e402ddca2032c111252f070a

  parent reply	other threads:[~2022-05-10  0:45 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-09 23:27 Glen Choo via GitGitGadget
2022-05-10  0:09 ` Junio C Hamano
2022-05-10  0:44 ` Junio C Hamano [this message]
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 ` [PATCH v2] " Glen Choo via GitGitGadget
2022-05-11 22:30   ` 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=xmqq7d6u9px0.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=chooglen@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=huang.zou@schrodinger.com \
    --cc=levraiphilippeblain@gmail.com \
    --cc=steadmon@google.com \
    --subject='Re: [PATCH] pull: only pass '\''--recurse-submodules'\'' to subcommands' \
    /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

Code repositories for project(s) associated with this 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).