{"thread":{"id":"54640","subject":"[PATCH 0/4] Fix 'pull --rebase --recurse-submodules' when local and upstream branches have no fork-point","startedAt":"2020-11-14T00:35:08Z","lastAt":"2020-11-14T00:35:09Z","messageCount":5,"participants":["Philippe Blain via GitGitGadget"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"409940","messageId":"pull.789.git.1605314085.gitgitgadget@gmail.com","threadId":"54640","inReplyTo":null,"subject":"[PATCH 0/4] Fix 'pull --rebase --recurse-submodules' when local and upstream branches have no fork-point","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-14T00:34:41Z","receivedAt":"2020-11-14T00:35:08Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"This series fixes a bug in 'git pull --rebase --recurse-submodules' when the\ncurrent branch and the remote-tracking branch we are pulling from have no\nfork-point (git merge-base --fork-point refs/remotes//\n returns empty), resulting in a misleading and fatal error\nmessage:\n\nfatal: cannot rebase with locally recorded submodule modifications\n\nThis is patch 4/4.\n\nPatch 1/4 is a preparatory refactoring, and patches 2-3 add comments to\nt5552-pull-submodules to describe 'pull --rebase --recurse-submodules' tests\na little better.\n\nPhilippe Blain (4):\n pull --rebase: compute rebase arguments in separate function\n t5572: add notes on a peculiar test\n t5572: describe '--rebase' tests a little more\n pull: check for local submodule modifications with the right range\n\n builtin/pull.c | 48 ++++++++++++++++++++++++---------\n t/t5572-pull-submodule.sh | 56 +++++++++++++++++++++++++++++++++++++--\n 2 files changed, 90 insertions(+), 14 deletions(-)\n\n\nbase-commit: e4d83eee9239207622e2b1cc43967da5051c189c\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-789%2Fphil-blain%2Fpull-recurse-no-fork-point-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-789/phil-blain/pull-recurse-no-fork-point-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/789\n-- \ngitgitgadget\n"},{"id":"409941","messageId":"cdabd96daa8bccf6338c118273c6f8af661e8b41.1605314085.git.gitgitgadget@gmail.com","threadId":"54640","inReplyTo":"pull.789.git.1605314085.gitgitgadget@gmail.com","subject":"[PATCH 1/4] pull --rebase: compute rebase arguments in separate function","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-14T00:34:42Z","receivedAt":"2020-11-14T00:35:08Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nThe function 'run_rebase' is responsible for constructing the\ncommand line to be passed to 'git rebase'. This includes both forwarding\npass-through options given to 'git pull' as well computing the \nand arguments to 'git rebase'.\n\nA following commit will need to access the argument in\n'cmd_pull' to fix a bug with 'git pull --rebase --recurse-submodules'.\nIn order to do so, refactor the code so that the and\n commits are computed in a new, separate function,\n'get_rebase_newbase_and_upstream'.\n\nSigned-off-by: Philippe Blain \n---\n builtin/pull.c | 46 +++++++++++++++++++++++++++++++++++-----------\n 1 file changed, 35 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 17aa63cd35..ac6ef650ab 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -852,21 +852,42 @@ static int get_octopus_merge_base(struct object_id *merge_base,\n \n /**\n * Given the current HEAD oid, the merge head returned from git-fetch and the\n- * fork point calculated by get_rebase_fork_point(), runs git-rebase with the\n- * appropriate arguments and returns its exit status.\n+ * fork point calculated by get_rebase_fork_point(), compute the and\n+ * arguments to use for the upcoming git-rebase invocation.\n */\n-static int run_rebase(const struct object_id *curr_head,\n+static int get_rebase_newbase_and_upstream(struct object_id *newbase,\n+\t\tstruct object_id *upstream,\n+\t\tconst struct object_id *curr_head,\n \t\tconst struct object_id *merge_head,\n \t\tconst struct object_id *fork_point)\n {\n-\tint ret;\n \tstruct object_id oct_merge_base;\n-\tstruct strvec args = STRVEC_INIT;\n \n \tif (!get_octopus_merge_base(&oct_merge_base, curr_head, merge_head, fork_point))\n \t\tif (!is_null_oid(fork_point) && oideq(&oct_merge_base, fork_point))\n \t\t\tfork_point = NULL;\n \n+\tif (fork_point && !is_null_oid(fork_point))\n+\t\toidcpy(upstream, fork_point);\n+\telse\n+\t\toidcpy(upstream, merge_head);\n+\n+\toidcpy(newbase, merge_head);\n+\n+\treturn 0;\n+}\n+\n+/**\n+ * Given the and calculated by\n+ * get_rebase_newbase_and_upstream(), runs git-rebase with the\n+ * appropriate arguments and returns its exit status.\n+ */\n+static int run_rebase(const struct object_id *newbase,\n+\t\tconst struct object_id *upstream)\n+{\n+\tint ret;\n+\tstruct strvec args = STRVEC_INIT;\n+\n \tstrvec_push(&args, \"rebase\");\n \n \t/* Shared options */\n@@ -894,12 +915,9 @@ static int run_rebase(const struct object_id *curr_head,\n \t\twarning(_(\"ignoring --verify-signatures for rebase\"));\n \n \tstrvec_push(&args, \"--onto\");\n-\tstrvec_push(&args, oid_to_hex(merge_head));\n+\tstrvec_push(&args, oid_to_hex(newbase));\n \n-\tif (fork_point && !is_null_oid(fork_point))\n-\t\tstrvec_push(&args, oid_to_hex(fork_point));\n-\telse\n-\t\tstrvec_push(&args, oid_to_hex(merge_head));\n+\tstrvec_push(&args, oid_to_hex(upstream));\n \n \tret = run_command_v_opt(args.v, RUN_GIT_CMD);\n \tstrvec_clear(&args);\n@@ -1011,6 +1029,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \tif (opt_rebase) {\n \t\tint ret = 0;\n \t\tint ran_ff = 0;\n+\n+\t\tstruct object_id newbase;\n+\t\tstruct object_id upstream;\n+\t\tget_rebase_newbase_and_upstream(&newbase, &upstream, &curr_head,\n+\t\t\t\t\t\tmerge_heads.oid, &rebase_fork_point);\n+\n \t\tif ((recurse_submodules == RECURSE_SUBMODULES_ON ||\n \t\t recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&\n \t\t submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head))\n@@ -1034,7 +1058,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \t\t\tfree_commit_list(list);\n \t\t}\n \t\tif (!ran_ff)\n-\t\t\tret = run_rebase(&curr_head, merge_heads.oid, &rebase_fork_point);\n+\t\t\tret = run_rebase(&newbase, &upstream);\n \n \t\tif (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||\n \t\t\t recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))\n-- \ngitgitgadget\n\n"},{"id":"409942","messageId":"4a124031400c38d7b44298a3328e4e81d756b637.1605314085.git.gitgitgadget@gmail.com","threadId":"54640","inReplyTo":"pull.789.git.1605314085.gitgitgadget@gmail.com","subject":"[PATCH 3/4] t5572: describe '--rebase' tests a little more","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-14T00:34:44Z","receivedAt":"2020-11-14T00:35:08Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nIt can be hard at first glance to distinguish what is different between\nthe two tests 'recursive rebasing pull' and 'pull rebase recursing fails\nwith conflicts' in 't5572-pull-submodule.sh', and to understand how they\nrelate to the scenarios described in a6d7eb2c7a (pull: optionally rebase\nsubmodules (remote submodule changes only), 2017-06-23), which\nimplemented '--recurse-submodules' for 'git pull' and added these tests.\n\nRename the tests to be more descriptive and add some bullet points\ncomments describing the different scenarios.\n\nSigned-off-by: Philippe Blain \n---\n t/t5572-pull-submodule.sh | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\nindex 7f658dba6d..7d9e12df4d 100755\n--- a/t/t5572-pull-submodule.sh\n+++ b/t/t5572-pull-submodule.sh\n@@ -101,7 +101,12 @@ test_expect_success \" --[no-]recurse-submodule and submodule.recurse\" '\n \ttest_path_is_file super/sub/merge_strategy_4.t\n '\n \n-test_expect_success 'recursive rebasing pull' '\n+test_expect_success 'pull --rebase --recurse-submodules (remote superproject submodule changes, local submodule changes)' '\n+\t# This tests the following scenario :\n+\t# - local submodule has new commits\n+\t# - local superproject does not have new commits\n+\t# - upstream superproject has new commits that change the submodule pointer\n+\n \t# change upstream\n \ttest_commit -C child rebase_strategy &&\n \tgit -C parent submodule update --remote &&\n@@ -116,7 +121,10 @@ test_expect_success 'recursive rebasing pull' '\n \ttest_path_is_file super/sub/local_stuff.t\n '\n \n-test_expect_success 'pull rebase recursing fails with conflicts' '\n+test_expect_success 'pull --rebase --recurse-submodules fails if both sides record submodule changes' '\n+\t# This tests the following scenario :\n+\t# - local superproject has new commits that change the submodule pointer\n+\t# - upstream superproject has new commits that change the submodule pointer\n \n \t# local changes in submodule recorded in superproject:\n \ttest_commit -C super/sub local_stuff_2 &&\n-- \ngitgitgadget\n\n"},{"id":"409943","messageId":"85c5766556e53f780b9a3ac677a0c758c51baae2.1605314085.git.gitgitgadget@gmail.com","threadId":"54640","inReplyTo":"pull.789.git.1605314085.gitgitgadget@gmail.com","subject":"[PATCH 2/4] t5572: add notes on a peculiar test","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-14T00:34:43Z","receivedAt":"2020-11-14T00:35:08Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nTest 5572.63 (\"branch has no merge base with remote-tracking\ncounterpart\") was introduced in 4d36f88be7 (submodule: do not pass null\nOID to setup_revisions, 2018-05-24), as a regression test for the bug\nthis commit was fixing (preventing a 'fatal: bad object' error when the\ncurrent branch and the remote-tracking branch we are pulling have no\nmerge-base).\n\nHowever, the commit message for 4d36f88be7 does not describe in which\nreal-life situation this bug was encountered. The brief discussion on the\nmailing list [1] does not either.\n\nThe regression test is not really representative of a real-life\nscenario: both the local repository and its upstream have only a single\ncommit, and the \"no merge-base\" scenario is simulated by recreating this\nroot commit in the local repository using 'git commit-tree' before\ncalling 'git pull --rebase --recurse-submodules'. The rebase succeeds\nand results in the local branch being reset to the same root commit as\nthe upstream branch.\n\nThe fix in 4d36f88be7 modifies 'submodule.c::submodule_touches_in_range'\nso that if 'excl_oid' is null, which is the case when the 'git merge-base\n--fork-point' invocation in 'builtin/pull.c::get_rebase_fork_point'\nerrors (no fork-point), then instead of 'incl_oid --not excl_oid' being\npassed to setup_revisions, only 'incl_oid' is passed, and\n'submodule_touches_in_range' examines 'incl_oid' and all its ancestors\nto verify that they do not touch the submodule.\n\nIn test 5572.63, the recreated lone root commit in the local repository is\nthus the only commit being examined by 'submodule_touches_in_range', and\nthis commit *adds* the submodule. However, 'submodule_touches_in_range'\n*succeeds* because 'combine-diff.c::diff_tree_combined' (see the\nbacktrace below) returns early since this commit is the root commit\nand has no parents.\n\n #0 diff_tree_combined at combine-diff.c:1494\n #1 0x0000000100150cbe in diff_tree_combined_merge at combine-diff.c:1649\n #2 0x00000001002c7147 in collect_changed_submodules at submodule.c:869\n #3 0x00000001002c7d6f in submodule_touches_in_range at submodule.c:1268\n #4 0x00000001000ad58b in cmd_pull at builtin/pull.c:1040\n\nIn light of all this, add a note in t5572 documenting this peculiar\ntest.\n\n[1] https://lore.kernel.org/git/20180524204729.19896-1-jonathantanmy@google.com/t/#u\n\nSigned-off-by: Philippe Blain \n---\n t/t5572-pull-submodule.sh | 15 +++++++++++++++\n 1 file changed, 15 insertions(+)\n\ndiff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\nindex 1d75e3b12b..7f658dba6d 100755\n--- a/t/t5572-pull-submodule.sh\n+++ b/t/t5572-pull-submodule.sh\n@@ -136,6 +136,21 @@ test_expect_success 'pull rebase recursing fails with conflicts' '\n \ttest_i18ngrep \"locally recorded submodule modifications\" err\n '\n \n+# NOTE:\n+#\n+# This test is particular because there is only a single commit in the upstream superproject\n+# 'parent' (which adds the submodule 'a-submodule'). The clone of the superproject\n+# ('child') hard-resets its branch to a new root commit with the same tree as the one\n+# from the upstream superproject, so that its branch has no merge-base with its\n+# remote-tracking counterpart, and then calls 'git pull --recurse-submodules --rebase'.\n+# The result is that the local branch is reset to the remote-tracking branch (as it was\n+# originally before the hard-reset).\n+\n+# The only commit in the range generated by 'submodule.c::submodule_touches_in_range' and\n+# passed to 'submodule.c::collect_changed_submodules' is the new (regenerated) initial commit,\n+# which adds the submodule.\n+# However, 'submodule_touches_in_range' does not error (even though this commit adds the submodule)\n+# because 'combine-diff.c::diff_tree_combined' returns early, as the initial commit has no parents.\n test_expect_success 'branch has no merge base with remote-tracking counterpart' '\n \trm -rf parent child &&\n \n-- \ngitgitgadget\n\n"},{"id":"409944","messageId":"d019be1602e48fc2ab899a59027a084f85e4d5f2.1605314085.git.gitgitgadget@gmail.com","threadId":"54640","inReplyTo":"pull.789.git.1605314085.gitgitgadget@gmail.com","subject":"[PATCH 4/4] pull: check for local submodule modifications with the right range","fromName":"Philippe Blain via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-11-14T00:34:45Z","receivedAt":"2020-11-14T00:35:09Z","isPatch":true,"sender":{"key":"levraiphilippeblain@gmail.com","avatar":"https://avatars.githubusercontent.com/u/44212482?v=4"},"body":"From: Philippe Blain \n\nEver since 'git pull' learned '--recurse-submodules' in a6d7eb2c7a\n(pull: optionally rebase submodules (remote submodule changes only),\n2017-06-23), we check if there are local submodule modifications by\nchecking the revision range 'curr_head --not rebase_fork_point'.\n\nThe goal of this check is to abort the pull if there are submodule\nmodifications in the local commits being rebased, since this scenario is\nnot supported.\n\nHowever, the actual range of commits being rebased is not\n'rebase_fork_point..curr_head', as the logic in\n'get_rebase_newbase_and_upstream' reveals, it is 'upstream..curr_head'.\n\nIf the 'git merge-base --fork-point' invocation in\n'get_rebase_fork_point' fails to find a fork point between the current\nbranch and the remote-tracking branch we are pulling from,\n'rebase_fork_point' is null and since 4d36f88be7 (submodule: do not pass\nnull OID to setup_revisions, 2018-05-24), 'submodule_touches_in_range'\nchecks 'curr_head' and all its ancestors for submodule modifications.\n\nSince it is highly likely that there are submodule modifications in this\nrange (which is in effect the whole history of the current branch), this\nprevents 'git pull --rebase --recurse-submodules' from succeeding if no\nfork point exists between the current branch and the remote-tracking\nbranch being pulled. This can happen, for example, when the current\nbranch was forked from a commit which was never recorded in the reflog\nof the remote-tracking branch we are pulling, as the last two paragraphs\nof the \"Discussion on fork-point mode\" section in git-merge-base(1)\nexplain.\n\nFix this bug by passing 'upstream' instead of 'rebase_fork_point' as the\n'excl_oid' argument to 'submodule_touches_in_range'.\n\nReported-by: Brice Goglin \nSigned-off-by: Philippe Blain \n---\n builtin/pull.c | 2 +-\n t/t5572-pull-submodule.sh | 29 +++++++++++++++++++++++++++++\n 2 files changed, 30 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex ac6ef650ab..b7c87b87ea 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -1037,7 +1037,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix)\n \n \t\tif ((recurse_submodules == RECURSE_SUBMODULES_ON ||\n \t\t recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&\n-\t\t submodule_touches_in_range(the_repository, &rebase_fork_point, &curr_head))\n+\t\t submodule_touches_in_range(the_repository, &upstream, &curr_head))\n \t\t\tdie(_(\"cannot rebase with locally recorded submodule modifications\"));\n \t\tif (!autostash) {\n \t\t\tstruct commit_list *list = NULL;\ndiff --git a/t/t5572-pull-submodule.sh b/t/t5572-pull-submodule.sh\nindex 7d9e12df4d..37fd06b0be 100755\n--- a/t/t5572-pull-submodule.sh\n+++ b/t/t5572-pull-submodule.sh\n@@ -144,6 +144,35 @@ test_expect_success 'pull --rebase --recurse-submodules fails if both sides reco\n \ttest_i18ngrep \"locally recorded submodule modifications\" err\n '\n \n+test_expect_success 'pull --rebase --recurse-submodules (no submodule changes, no fork-point)' '\n+\t# This tests the following scenario :\n+\t# - local submodule does not have new commits\n+\t# - local superproject has new commits that *do not* change the submodule pointer\n+\t# - upstream superproject has new commits that *do not* change the submodule pointer\n+\t# - local superproject branch has no fork-point with its remote-tracking counter-part\n+\n+\t# create upstream superproject\n+\ttest_create_repo submodule &&\n+\ttest_commit -C submodule first_in_sub &&\n+\n+\ttest_create_repo superprojet &&\n+\ttest_commit -C superprojet first_in_super &&\n+\tgit -C superprojet submodule add ../submodule &&\n+\tgit -C superprojet commit -m \"add submodule\" &&\n+\ttest_commit -C superprojet third_in_super &&\n+\n+\t# clone superproject\n+\tgit clone --recurse-submodules superprojet superclone &&\n+\n+\t# add commits upstream\n+\ttest_commit -C superprojet fourth_in_super &&\n+\n+\t# create topic branch in clone, not based on any remote-tracking branch\n+\tgit -C superclone checkout -b feat HEAD~1 &&\n+\ttest_commit -C superclone first_on_feat &&\n+\tgit -C superclone pull --rebase --recurse-submodules origin master\n+'\n+\n # NOTE:\n #\n # This test is particular because there is only a single commit in the upstream superproject\n-- \ngitgitgadget\n"}]}