[PATCH 0/4] Fix 'pull --rebase --recurse-submodules' when local and upstream branches have no fork-point

[PATCH 0/4] Fix 'pull --rebase --recurse-submodules' when local and upstream branches have no fork-point

[Philippe Blain via GitGitGadget]

This series fixes a bug in 'git pull --rebase --recurse-submodules' when the
current branch and the remote-tracking branch we are pulling from have no
fork-point (git merge-base --fork-point refs/remotes/<remote>/<upstream>
<current-branch> returns empty), resulting in a misleading and fatal error
message:

fatal: cannot rebase with locally recorded submodule modifications

This is patch 4/4.

Patch 1/4 is a preparatory refactoring, and patches 2-3 add comments to
t5552-pull-submodules to describe 'pull --rebase --recurse-submodules' tests
a little better.

Philippe Blain (4):
  pull --rebase: compute rebase arguments in separate function
  t5572: add notes on a peculiar test
  t5572: describe '--rebase' tests a little more
  pull: check for local submodule modifications with the right range

 builtin/pull.c            | 48 ++++++++++++++++++++++++---------
 t/t5572-pull-submodule.sh | 56 +++++++++++++++++++++++++++++++++++++--
 2 files changed, 90 insertions(+), 14 deletions(-)


base-commit: e4d83eee9239207622e2b1cc43967da5051c189c
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-789%2Fphil-blain%2Fpull-recurse-no-fork-point-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-789/phil-blain/pull-recurse-no-fork-point-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/789
-- 
gitgitgadget

[PATCH 1/4] pull --rebase: compute rebase arguments in separate function

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

The function 'run_rebase' is responsible for constructing the
command line to be passed to 'git rebase'. This includes both forwarding
pass-through options given to 'git pull' as well computing the <newbase>
and <upstream> arguments to 'git rebase'.

A following commit will need to access the <upstream> argument in
'cmd_pull' to fix a bug with 'git pull --rebase --recurse-submodules'.
In order to do so, refactor the code so that the <newbase> and
<upstream> commits are computed in a new, separate function,
'get_rebase_newbase_and_upstream'.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 builtin/pull.c | 46 +++++++++++++++++++++++++++++++++++-----------
 1 file changed, 35 insertions(+), 11 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 17aa63cd35..ac6ef650ab 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -852,21 +852,42 @@ static int get_octopus_merge_base(struct object_id *merge_base,
 
 /**
  * Given the current HEAD oid, the merge head returned from git-fetch and the
- * fork point calculated by get_rebase_fork_point(), runs git-rebase with the
- * appropriate arguments and returns its exit status.
+ * fork point calculated by get_rebase_fork_point(), compute the <newbase> and
+ * <upstream> arguments to use for the upcoming git-rebase invocation.
  */
-static int run_rebase(const struct object_id *curr_head,
+static int get_rebase_newbase_and_upstream(struct object_id *newbase,
+		struct object_id *upstream,
+		const struct object_id *curr_head,
 		const struct object_id *merge_head,
 		const struct object_id *fork_point)
 {
-	int ret;
 	struct object_id oct_merge_base;
-	struct strvec args = STRVEC_INIT;
 
 	if (!get_octopus_merge_base(&oct_merge_base, curr_head, merge_head, fork_point))
 		if (!is_null_oid(fork_point) && oideq(&oct_merge_base, fork_point))
 			fork_point = NULL;
 
+	if (fork_point && !is_null_oid(fork_point))
+		oidcpy(upstream, fork_point);
+	else
+		oidcpy(upstream, merge_head);
+
+	oidcpy(newbase, merge_head);
+
+	return 0;
+}
+
+/**
+ * Given the <newbase> and <upstream> calculated by
+ * get_rebase_newbase_and_upstream(), runs git-rebase with the
+ * appropriate arguments and returns its exit status.
+ */
+static int run_rebase(const struct object_id *newbase,
+		const struct object_id *upstream)
+{
+	int ret;
+	struct strvec args = STRVEC_INIT;
+
 	strvec_push(&args, "rebase");
 
 	/* Shared options */
@@ -894,12 +915,9 @@ static int run_rebase(const struct object_id *curr_head,
 		warning(_("ignoring --verify-signatures for rebase"));
 
 	strvec_push(&args, "--onto");
-	strvec_push(&args, oid_to_hex(merge_head));
+	strvec_push(&args, oid_to_hex(newbase));
 
-	if (fork_point && !is_null_oid(fork_point))
-		strvec_push(&args, oid_to_hex(fork_point));
-	else
-		strvec_push(&args, oid_to_hex(merge_head));
+	strvec_push(&args, oid_to_hex(upstream));
 
 	ret = run_command_v_opt(args.v, RUN_GIT_CMD);
 	strvec_clear(&args);
@@ -1011,6 +1029,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (opt_rebase) {
 		int ret = 0;
 		int ran_ff = 0;
+
+		struct object_id newbase;
+		struct object_id upstream;
+		get_rebase_newbase_and_upstream(&newbase, &upstream, &curr_head,
+						merge_heads.oid, &rebase_fork_point);
+
 		if ((recurse_submodules == RECURSE_SUBMODULES_ON ||
 		     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
 		    submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head))
@@ -1034,7 +1058,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 			free_commit_list(list);
 		}
 		if (!ran_ff)
-			ret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);
+			ret = run_rebase(&newbase, &upstream);
 
 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
-- 
gitgitgadget

[PATCH 2/4] t5572: add notes on a peculiar test

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

Test 5572.63 ("branch has no merge base with remote-tracking
counterpart") was introduced in 4d36f88be7 (submodule: do not pass null
OID to setup_revisions, 2018-05-24), as a regression test for the bug
this commit was fixing (preventing a 'fatal: bad object' error when the
current branch and the remote-tracking branch we are pulling have no
merge-base).

However, the commit message for 4d36f88be7 does not describe in which
real-life situation this bug was encountered. The brief discussion on the
mailing list [1] does not either.

The regression test is not really representative of a real-life
scenario: both the local repository and its upstream have only a single
commit, and the "no merge-base" scenario is simulated by recreating this
root commit in the local repository using 'git commit-tree' before
calling 'git pull --rebase --recurse-submodules'. The rebase succeeds
and results in the local branch being reset to the same root commit as
the upstream branch.

The fix in 4d36f88be7 modifies 'submodule.c::submodule_touches_in_range'
so that if 'excl_oid' is null, which is the case when the 'git merge-base
--fork-point' invocation in 'builtin/pull.c::get_rebase_fork_point'
errors (no fork-point), then instead of 'incl_oid --not excl_oid' being
passed to setup_revisions, only 'incl_oid' is passed, and
'submodule_touches_in_range' examines 'incl_oid' and all its ancestors
to verify that they do not touch the submodule.

In test 5572.63, the recreated lone root commit in the local repository is
thus the only commit being examined by 'submodule_touches_in_range', and
this commit *adds* the submodule. However, 'submodule_touches_in_range'
*succeeds* because 'combine-diff.c::diff_tree_combined' (see the
backtrace below) returns early since this commit is the root commit
and has no parents.

  #0  diff_tree_combined at combine-diff.c:1494
  #1  0x0000000100150cbe in diff_tree_combined_merge at combine-diff.c:1649
  #2  0x00000001002c7147 in collect_changed_submodules at submodule.c:869
  #3  0x00000001002c7d6f in submodule_touches_in_range at submodule.c:1268
  #4  0x00000001000ad58b in cmd_pull at builtin/pull.c:1040

In light of all this, add a note in t5572 documenting this peculiar
test.

[1] https://lore.kernel.org/git/20180524204729.19896-1-jonathantanmy@google.com/t/#u

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 t/t5572-pull-submodule.sh | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index 1d75e3b12b..7f658dba6d 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -136,6 +136,21 @@ test_expect_success 'pull rebase recursing fails with conflicts' '
 	test_i18ngrep "locally recorded submodule modifications" err
 '
 
+# NOTE:
+#
+# This test is particular because there is only a single commit in the upstream superproject
+# 'parent' (which adds the submodule 'a-submodule'). The clone of the superproject
+# ('child') hard-resets its branch to a new root commit with the same tree as the one
+# from the upstream superproject, so that its branch has no merge-base with its
+# remote-tracking counterpart, and then calls 'git pull --recurse-submodules --rebase'.
+# The result is that the local branch is reset to the remote-tracking branch (as it was
+# originally before the hard-reset).
+
+# The only commit in the range generated by 'submodule.c::submodule_touches_in_range' and
+# passed to 'submodule.c::collect_changed_submodules' is the new (regenerated) initial commit,
+# which adds the submodule.
+# However, 'submodule_touches_in_range' does not error (even though this commit adds the submodule)
+# because 'combine-diff.c::diff_tree_combined' returns early, as the initial commit has no parents.
 test_expect_success 'branch has no merge base with remote-tracking counterpart' '
 	rm -rf parent child &&
 
-- 
gitgitgadget

[PATCH 3/4] t5572: describe '--rebase' tests a little more

[Philippe Blain via GitGitGadget]

From: Philippe Blain <levraiphilippeblain@gmail.com>

It can be hard at first glance to distinguish what is different between
the two tests 'recursive rebasing pull' and 'pull rebase recursing fails
with conflicts' in 't5572-pull-submodule.sh', and to understand how they
relate to the scenarios described in a6d7eb2c7a (pull: optionally rebase
submodules (remote submodule changes only), 2017-06-23), which
implemented '--recurse-submodules' for 'git pull' and added these tests.

Rename the tests to be more descriptive and add some bullet points
comments describing the different scenarios.

Signed-off-by: Philippe Blain <levraiphilippeblain@gmail.com>
---
 t/t5572-pull-submodule.sh | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh
index 7f658dba6d..7d9e12df4d 100755
--- a/t/t5572-pull-submodule.sh
+++ b/t/t5572-pull-submodule.sh
@@ -101,7 +101,12 @@ test_expect_success " --[no-]recurse-submodule and submodule.recurse" '
 	test_path_is_file super/sub/merge_strategy_4.t
 '
 
-test_expect_success 'recursive rebasing pull' '
+test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '
+	# This tests the following scenario :
+	# - local submodule has new commits
+	# - local superproject does not have new commits
+	# - upstream superproject has new commits that change the submodule pointer
+
 	# change upstream
 	test_commit -C child rebase_strategy &&
 	git -C parent submodule update --remote &&
@@ -116,7 +121,10 @@ test_expect_success 'recursive rebasing pull' '
 	test_path_is_file super/sub/local_stuff.t
 '
 
-test_expect_success 'pull rebase recursing fails with conflicts' '
+test_expect_success 'pull --rebase --recurse-submodules fails if both sides record submodule changes' '
+	# This tests the following scenario :
+	# - local superproject has new commits that change the submodule pointer
+	# - upstream superproject has new commits that change the submodule pointer
 
 	# local changes in submodule recorded in superproject:
 	test_commit -C super/sub local_stuff_2 &&
-- 
gitgitgadget

[PATCH 4/4] pull: check for local submodule modifications with the right range

[Philippe Blain via GitGitGadget]

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