From 4f66d79ae301a74cead51e703d566ec5f395beb8 Mon Sep 17 00:00:00 2001 From: Philippe Blain Date: Sat, 14 Nov 2020 00:34:42 +0000 Subject: [PATCH 1/4] pull --rebase: compute rebase arguments in separate function 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 and arguments to 'git rebase'. A following commit will need to access the 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 and commits are computed in a new, separate function, 'get_rebase_newbase_and_upstream'. Signed-off-by: Philippe Blain Signed-off-by: Junio C Hamano --- builtin/pull.c | 46 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/builtin/pull.c b/builtin/pull.c index 425950f469..441e94aafd 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 and + * 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 and 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)) From ba58ddd0bf295f45361cdcddd07a2d0183dde4bd Mon Sep 17 00:00:00 2001 From: Philippe Blain Date: Sat, 14 Nov 2020 00:34:43 +0000 Subject: [PATCH 2/4] t5572: add notes on a peculiar test 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 Signed-off-by: Junio C Hamano --- 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 && From f260c6b46c1ebb5b1b4dfffd8812b5416cfcc4e7 Mon Sep 17 00:00:00 2001 From: Philippe Blain Date: Sat, 14 Nov 2020 00:34:44 +0000 Subject: [PATCH 3/4] t5572: describe '--rebase' tests a little more 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 Signed-off-by: Junio C Hamano --- 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 && From 5176f20ffeab9c2bda6f93195096372364ec6f88 Mon Sep 17 00:00:00 2001 From: Philippe Blain Date: Sat, 14 Nov 2020 00:34:45 +0000 Subject: [PATCH 4/4] pull: check for local submodule modifications with the right range 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 Signed-off-by: Philippe Blain Signed-off-by: Junio C Hamano --- 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 441e94aafd..ac40c439aa 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