git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: "Philippe Blain via GitGitGadget" <gitgitgadget@gmail.com>
To: git@vger.kernel.org
Cc: Jonathan Tan <jonathantanmy@google.com>,
	Stefan Beller <stefanbeller@gmail.com>,
	Brandon Williams <bwilliamseng@gmail.com>,
	Brice Goglin <bgoglin@free.fr>,
	Philippe Blain <levraiphilippeblain@gmail.com>,
	Philippe Blain <levraiphilippeblain@gmail.com>
Subject: [PATCH 4/4] pull: check for local submodule modifications with the right range
Date: Sat, 14 Nov 2020 00:34:45 +0000	[thread overview]
Message-ID: <d019be1602e48fc2ab899a59027a084f85e4d5f2.1605314085.git.gitgitgadget@gmail.com> (raw)
In-Reply-To: <pull.789.git.1605314085.gitgitgadget@gmail.com>

From: Philippe Blain <levraiphilippeblain@gmail.com>

Ever since 'git pull' learned '--recurse-submodules' in a6d7eb2c7a
(pull: optionally rebase submodules (remote submodule changes only),
2017-06-23), we check if there are local submodule modifications by
checking the revision range 'curr_head --not rebase_fork_point'.

The goal of this check is to abort the pull if there are submodule
modifications in the local commits being rebased, since this scenario is
not supported.

However, the actual range of commits being rebased is not
'rebase_fork_point..curr_head', as the logic in
'get_rebase_newbase_and_upstream' reveals, it is 'upstream..curr_head'.

If the 'git merge-base --fork-point' invocation in
'get_rebase_fork_point' fails to find a fork point between the current
branch and the remote-tracking branch we are pulling from,
'rebase_fork_point' is null and since 4d36f88be7 (submodule: do not pass
null OID to setup_revisions, 2018-05-24), 'submodule_touches_in_range'
checks 'curr_head' and all its ancestors for submodule modifications.

Since it is highly likely that there are submodule modifications in this
range (which is in effect the whole history of the current branch), this
prevents 'git pull --rebase --recurse-submodules' from succeeding if no
fork point exists between the current branch and the remote-tracking
branch being pulled. This can happen, for example, when the current
branch was forked from a commit which was never recorded in the reflog
of the remote-tracking branch we are pulling, as the last two paragraphs
of the "Discussion on fork-point mode" section in git-merge-base(1)
explain.

Fix this bug by passing 'upstream' instead of 'rebase_fork_point' as the
'excl_oid' argument to 'submodule_touches_in_range'.

Reported-by: Brice Goglin <bgoglin@free.fr>
Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 builtin/pull.c            |  2 +-
 t/t5572-pull-submodule.sh | 29 +++++++++++++++++++++++++++++
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index ac6ef650ab..b7c87b87ea 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1037,7 +1037,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 		if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
 		     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
-		    submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head))
+		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
 			die(_("cannot rebase with locally recorded submodule modifications"));
 		if (!autostash) {
 			struct commit_list *list = NULL;
diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index 7d9e12df4d..37fd06b0be 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -144,6 +144,35 @@ test_expect_success 'pull --rebase --recurse-submodules fails if both sides reco
 	test_i18ngrep "locally recorded submodule modifications" err
 '
 
+test_expect_success 'pull --rebase --recurse-submodules (no submodule changes, no fork-point)' '
+	# This tests the following scenario :
+	# - local submodule does not have new commits
+	# - local superproject has new commits that *do not* change the submodule pointer
+	# - upstream superproject has new commits that *do not* change the submodule pointer
+	# - local superproject branch has no fork-point with its remote-tracking counter-part
+
+	# create upstream superproject
+	test_create_repo submodule &&
+	test_commit -C submodule first_in_sub &&
+
+	test_create_repo superprojet &&
+	test_commit -C superprojet first_in_super &&
+	git -C superprojet submodule add ../submodule &&
+	git -C superprojet commit -m "add submodule" &&
+	test_commit -C superprojet third_in_super &&
+
+	# clone superproject
+	git clone --recurse-submodules superprojet superclone &&
+
+	# add commits upstream
+	test_commit -C superprojet fourth_in_super &&
+
+	# create topic branch in clone, not based on any remote-tracking branch
+	git -C superclone checkout -b feat HEAD~1 &&
+	test_commit -C superclone first_on_feat &&
+	git -C superclone pull --rebase --recurse-submodules origin master
+'
+
 # NOTE:
 #
 # This test is particular because there is only a single commit in the upstream superproject
-- 
gitgitgadget

      parent reply	other threads:[~2020-11-14  0:35 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-14  0:34 [PATCH 0/4] Fix 'pull --rebase --recurse-submodules' when local and upstream branches have no fork-point Philippe Blain via GitGitGadget
2020-11-14  0:34 ` [PATCH 1/4] pull --rebase: compute rebase arguments in separate function Philippe Blain via GitGitGadget
2020-11-14  0:34 ` [PATCH 2/4] t5572: add notes on a peculiar test Philippe Blain via GitGitGadget
2020-11-14  0:34 ` [PATCH 3/4] t5572: describe '--rebase' tests a little more Philippe Blain via GitGitGadget
2020-11-14  0:34 ` Philippe Blain via GitGitGadget [this message]

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=d019be1602e48fc2ab899a59027a084f85e4d5f2.1605314085.git.gitgitgadget@gmail.com \
    --to=gitgitgadget@gmail.com \
    --cc=bgoglin@free.fr \
    --cc=bwilliamseng@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=levraiphilippeblain@gmail.com \
    --cc=stefanbeller@gmail.com \
    --subject='Re: [PATCH 4/4] pull: check for local submodule modifications with the right range' \
    /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).