{"thread":{"id":"57536","subject":"[PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","startedAt":"2022-03-11T05:05:51Z","lastAt":"2022-03-18T16:55:43Z","messageCount":41,"participants":["John Cai via GitGitGadget","Junio C Hamano","John Cai","Phillip Wood","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"451108","messageId":"pull.1226.git.git.1646975144178.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":null,"subject":"[PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-11T05:05:44Z","receivedAt":"2022-03-11T05:05:51Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nFixes a bug whereby rebase updates the deferenced reference HEAD points\nto instead of HEAD directly.\n\nIf HEAD is on main and if the following is a fast-forward operation,\n\ngit rebase $(git rev-parse main) $(git rev-parse topic)\n\nInstead of HEAD being set to $(git rev-parse topic), rebase erroneously\ndereferences HEAD and sets main to $(git rev-parse topic). This bug was\nreported by Michael McClimon. See [1].\n\nThis is happening because on a fast foward with an oid as a ,\nupdate_refs() will only call update_ref() with REF_NO_DEREF if\nRESET_HEAD_DETACH is set. This change was made in 176f5d96 (built-in rebase\n--autostash: leave the current branch alone if possible,\n2018-11-07). In rebase, we are not setting the RESET_HEAD_DETACH flag,\nwhich means that the update_ref() call ends up dereferencing\nHEAD and updating it to the oid used as .\n\nThe correct behavior is that git rebase should update HEAD to $(git\nrev-parse topic) without dereferencing it.\n\nFix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date\nso that once reset_head() calls update_refs(), it calls update_ref() with\nREF_NO_DEREF which updates HEAD directly intead of deferencing it and\nupdating the branch that HEAD points to.\n\nAlso add a test to ensure this behavior.\n\n1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n\nSigned-off-by: John Cai \n---\n rebase: update HEAD when is an oid\n \n Fixes a bug [1] reported by Michael McClimon where rebase with oids will\n erroneously update the branch HEAD points to.\n \n 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v1\nPull-Request: https://github.com/git/git/pull/1226\n\n builtin/rebase.c | 2 +-\n t/t3400-rebase.sh | 21 +++++++++++++++++++++\n 2 files changed, 22 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b29ad2b65e7..52afeffcc2e 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -828,7 +828,7 @@ static int checkout_up_to_date(struct rebase_options *options)\n \t\t options->switch_to);\n \tropts.oid = &options->orig_head;\n \tropts.branch = options->head_name;\n-\tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n+\tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK | RESET_HEAD_DETACH;\n \tropts.head_msg = buf.buf;\n \tif (reset_head(the_repository, &ropts) < 0)\n \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 71b1735e1dd..0b92e78976c 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' '\n \t)\n '\n \n+test_expect_success 'rebase with oids' '\n+\tgit init main-wt &&\n+\t(\n+\t\tcd main-wt &&\n+\t\t>file &&\n+\t\tgit add file &&\n+\t\tgit commit -m initial &&\n+\t\tgit checkout -b side &&\n+\t\techo >>file &&\n+\t\tgit commit -a -m side &&\n+\t\tgit checkout main &&\n+\t\tgit tag hold &&\n+\t\tgit checkout -B main hold &&\n+\t\tgit rev-parse main >pre &&\n+\t\tgit rebase $(git rev-parse main) $(git rev-parse side) &&\n+\t\tgit rev-parse main >post &&\n+\t\ttest \"$(git rev-parse side)\" = \"$(cat .git/HEAD)\" &&\n+\t\ttest_cmp pre post\n+\t)\n+'\n+\n test_done\n\nbase-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b\n-- \ngitgitgadget\n"},{"id":"451109","messageId":"xmqqczitaw8q.fsf@gitster.g","threadId":"57536","inReplyTo":"pull.1226.git.git.1646975144178.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-11T05:33:57Z","receivedAt":"2022-03-11T05:34:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 71b1735e1dd..0b92e78976c 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' '\n> \t)\n> '\n> \n> +test_expect_success 'rebase with oids' '\n> +\tgit init main-wt &&\n> +\t(\n> +\t\tcd main-wt &&\n> +\t\t>file &&\n> +\t\tgit add file &&\n> +\t\tgit commit -m initial &&\n> +\t\tgit checkout -b side &&\n> +\t\techo >>file &&\n> +\t\tgit commit -a -m side &&\n> +\t\tgit checkout main &&\n\nThe above is sufficient set-up.\n\n> +\t\tgit tag hold &&\n> +\t\tgit checkout -B main hold &&\n\nThese two are unnecessary. It was there in my bisect \"runme\" script\nonly because I originally used an out-of-line repository that is\nprepared beforehand, so that \"move main back to hold and rerun the\nrebase\" can be done regardless of the bug in the previous version\nchecked during \"bisect run\".\n\n> +\t\tgit rev-parse main >pre &&\n> +\t\tgit rebase $(git rev-parse main) $(git rev-parse side) &&\n> +\t\tgit rev-parse main >post &&\n> +\t\ttest \"$(git rev-parse side)\" = \"$(cat .git/HEAD)\" &&\n\nYou may want to prepare for segfaulting \"git rev-parse\" in the\nabove three lines.\n\nNever \"cat .git/HEAD\". use \"git rev-parse\".\n\n> +\t\ttest_cmp pre post\n> +\t)\n> +'\n> +\n> test_done\n>\n> base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b\n"},{"id":"451110","messageId":"xmqq5yolav8l.fsf@gitster.g","threadId":"57536","inReplyTo":"pull.1226.git.git.1646975144178.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-11T05:55:38Z","receivedAt":"2022-03-11T05:59:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> This is happening because on a fast foward with an oid as a ,\n> update_refs() will only call update_ref() with REF_NO_DEREF if\n> RESET_HEAD_DETACH is set. This change was made in 176f5d96 (built-in rebase\n> --autostash: leave the current branch alone if possible,\n> 2018-11-07). In rebase, we are not setting the RESET_HEAD_DETACH flag,\n> which means that the update_ref() call ends up dereferencing\n> HEAD and updating it to the oid used as .\n>\n> The correct behavior is that git rebase should update HEAD to $(git\n> rev-parse topic) without dereferencing it.\n\nIt is unintuitive that unconditionally setting the RESET_HEAD_DETACH\nbit is the right solution.\n\nIf the command weren't \"rebase master side^0\" but \"rebase master\nside\", i.e. \"please rebase the side branch itself, not an unnamed\nbranch created out of the side branch, on master\", according to\n, we ought to end up being on a detached HEAD, as\nreset_head() with the bit\n\n /* Request a detached checkout */\n #define RESET_HEAD_DETACH (1<<0)\n\nrequests a detached checkout. But that apparently is not what would\nhappen with your patch applied.\n\nPuzzled. The solution to the puzzle probably deserves to be in the\nproposed log message.\n\nThanks.\n\n"},{"id":"451132","messageId":"B1A94338-404E-43F2-AF3C-889097FD938B@gmail.com","threadId":"57536","inReplyTo":"xmqq5yolav8l.fsf@gitster.g","subject":"Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-03-11T14:14:39Z","receivedAt":"2022-03-11T14:14:45Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 11 Mar 2022, at 0:55, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" writes:\n>\n>> This is happening because on a fast foward with an oid as a ,\n>> update_refs() will only call update_ref() with REF_NO_DEREF if\n>> RESET_HEAD_DETACH is set. This change was made in 176f5d96 (built-in rebase\n>> --autostash: leave the current branch alone if possible,\n>> 2018-11-07). In rebase, we are not setting the RESET_HEAD_DETACH flag,\n>> which means that the update_ref() call ends up dereferencing\n>> HEAD and updating it to the oid used as .\n>>\n>> The correct behavior is that git rebase should update HEAD to $(git\n>> rev-parse topic) without dereferencing it.\n>\n> It is unintuitive that unconditionally setting the RESET_HEAD_DETACH\n> bit is the right solution.\n>\n> If the command weren't \"rebase master side^0\" but \"rebase master\n> side\", i.e. \"please rebase the side branch itself, not an unnamed\n> branch created out of the side branch, on master\", according to\n> , we ought to end up being on a detached HEAD, as\n> reset_head() with the bit\n>\n> /* Request a detached checkout */\n> #define RESET_HEAD_DETACH (1<<0)\n>\n> requests a detached checkout. But that apparently is not what would\n> happen with your patch applied.\n>\n> Puzzled. The solution to the puzzle probably deserves to be in the\n> proposed log message.\n\nGood point. Thinking aloud, here is the callstack.\n\ncheckout_up_to_date() -> reset_head() -> update_refs() -> update_ref()\n\nif is not a valid ref, rebase_options head_name is set to NULL. This\neventually leads update_refs() to decide that it doesn't need to switch to a\nbranch via its switch_to_branch variable.\n\nreset.c:\n\nif (!switch_to_branch)\n\tret = update_ref(reflog_head, \"HEAD\", oid, head,\n\t\t\t detach_head ? REF_NO_DEREF : 0,\n\t\t\t UPDATE_REFS_MSG_ON_ERR);\n else {\n\tret = update_ref(reflog_branch ? reflog_branch : reflog_head,\n\t\t\t switch_to_branch, oid, NULL, 0,\n\t\t\t UPDATE_REFS_MSG_ON_ERR);\n\tif (!ret)\n\t\tret = create_symref(\"HEAD\", switch_to_branch,\n\t\t\t\t reflog_head);\n}\n\nsince the flags do not include RESET_HEAD_DETACH, detach_head is set to false and we get a\ndeferenced HEAD update.\n\nThe solution I came up with works because when __is__ a valid branch,\nudpate_refs() takes a different code path that calls create_symref() with a\nbranch, which is why we don't end up with a detached HEAD.\n\nI see why this is confusing though. From checkout_up_to_date's perspective it looks like we\nare unconditionally detaching HEAD. So what we could do is only set the flag in\ncheckout_up_to_date() when, from checkout_up_to_date's perspective, we will end\nup with a detached head. something like this:\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b29ad2b65e72..f0403fb12421 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -827,8 +827,10 @@ static int checkout_up_to_date(struct rebase_options *options)\n getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n options->switch_to);\n ropts.oid = &options->orig_head;\n- ropts.branch = options->head_name;\n ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n+ ropts.branch = options->head_name;\n+ if (!ropts.branch)\n+ ropts.flags |= RESET_HEAD_DETACH;\n ropts.head_msg = buf.buf;\n if (reset_head(the_repository, &ropts) < 0)\n ret = error(_(\"could not switch to %s\"), options->switch_to);\n\nOtherwise, checkout_up_to_date() has to implicitly know the downstream logic in\nupdate_refs(). I believe that's the main source of the confusion--is that right?\n\n>\n> Thanks.\n\nThanks\nJohn\n"},{"id":"451135","messageId":"7c1c0b8e-7895-7a0e-6ab0-e45e21ec7329@gmail.com","threadId":"57536","inReplyTo":"pull.1226.git.git.1646975144178.gitgitgadget@gmail.com","subject":"Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-11T15:05:27Z","receivedAt":"2022-03-11T15:05:34Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi John\n\nThanks for working on this\n\nOn 11/03/2022 05:05, John Cai via GitGitGadget wrote:\n> From: John Cai \n> \n> Fixes a bug whereby rebase updates the deferenced reference HEAD points\n> to instead of HEAD directly.\n> \n> If HEAD is on main and if the following is a fast-forward operation,\n> \n> git rebase $(git rev-parse main) $(git rev-parse topic)\n> \n> Instead of HEAD being set to $(git rev-parse topic), rebase erroneously\n> dereferences HEAD and sets main to $(git rev-parse topic). This bug was\n> reported by Michael McClimon. See [1].\n\nOften we just add a Reported-by: trailer unless the liked email has some \nuseful extra info (which arguably should not be the case with a well \nwritten commit message)\n\n> This is happening because on a fast foward with an oid as a ,\n> update_refs() will only call update_ref() with REF_NO_DEREF if\n> RESET_HEAD_DETACH is set. This change was made in 176f5d96 (built-in rebase\n> --autostash: leave the current branch alone if possible,\n> 2018-11-07). In rebase, we are not setting the RESET_HEAD_DETACH flag,\n> which means that the update_ref() call ends up dereferencing\n> HEAD and updating it to the oid used as .\n> \n> The correct behavior is that git rebase should update HEAD to $(git\n> rev-parse topic) without dereferencing it.\n> \n> Fix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date\n\nAs Junio points out it is confusing that it is always ok to pass that \nflag, I think we should only set it if we are not checking out a branch, \nsee below.\n\n> so that once reset_head() calls update_refs(), it calls update_ref() with\n> REF_NO_DEREF which updates HEAD directly intead of deferencing it and\n> updating the branch that HEAD points to.\n> \n> Also add a test to ensure this behavior.\n> \n> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n\nMaybe\nReported-by: Michael McClimon \n?\n\n> Signed-off-by: John Cai \n> ---\n> rebase: update HEAD when is an oid\n> \n> Fixes a bug [1] reported by Michael McClimon where rebase with oids will\n> erroneously update the branch HEAD points to.\n> \n> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n> \n> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v1\n> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v1\n> Pull-Request: https://github.com/git/git/pull/1226\n> \n> builtin/rebase.c | 2 +-\n> t/t3400-rebase.sh | 21 +++++++++++++++++++++\n> 2 files changed, 22 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index b29ad2b65e7..52afeffcc2e 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -828,7 +828,7 @@ static int checkout_up_to_date(struct rebase_options *options)\n> \t\t options->switch_to);\n> \tropts.oid = &options->orig_head;\n> \tropts.branch = options->head_name;\n> -\tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n> +\tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK | RESET_HEAD_DETACH;\n\nI think it would be clearer if the post image ended up as\n\n\tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK\n\tif (options->head_name)\n\t\tropts.branch = option->head_name\n\telse\n\t\tropts.flags |= RESET_HEAD_DETACH\n\nand we changed reset_head() to BUG() if both branch and \nRESET_HEAD_DETACH are given.\n\n> \tropts.head_msg = buf.buf;\n> \tif (reset_head(the_repository, &ropts) < 0)\n> \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 71b1735e1dd..0b92e78976c 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' '\n> \t)\n> '\n> \n> +test_expect_success 'rebase with oids' '\n> +\tgit init main-wt &&\n> +\t(\n> +\t\tcd main-wt &&\n> +\t\t>file &&\n> +\t\tgit add file &&\n> +\t\tgit commit -m initial &&\n> +\t\tgit checkout -b side &&\n> +\t\techo >>file &&\n> +\t\tgit commit -a -m side &&\n> +\t\tgit checkout main &&\n> +\t\tgit tag hold &&\n> +\t\tgit checkout -B main hold &&\n> +\t\tgit rev-parse main >pre &&\n> +\t\tgit rebase $(git rev-parse main) $(git rev-parse side) &&\n> +\t\tgit rev-parse main >post &&\n> +\t\ttest \"$(git rev-parse side)\" = \"$(cat .git/HEAD)\" &&\n> +\t\ttest_cmp pre post\n> +\t)\n> +'\n\nUsing a stand alone test for bisecting makes sense but I think we should \ntry and use the existing test setup for the regression test (it \ncertainly does not need to run in its own worktree). The diff below \nshows how this could be done. Ideally there would be a preparatory \ncommit that modernized the whole of the setup test rather than just the \ntwo commits we're using in the new test but that's not essential.\n\nBest Wishes\n\nPhillip\n\n---- >8 ----\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 71b1735e1d..d5a8ee39fc 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address\n export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL\n\n test_expect_success 'prepare repository with topic branches' '\n- git config core.logAllRefUpdates true &&\n- echo First >A &&\n- git update-index --add A &&\n- git commit -m \"Add A.\" &&\n+ test_commit \"Add A.\" A First First &&\n git checkout -b force-3way &&\n echo Dummy >Y &&\n git update-index --add Y &&\n@@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic \nbranches' '\n git mv A D/A &&\n git commit -m \"Move A.\" &&\n git checkout -b my-topic-branch main &&\n- echo Second >B &&\n- git update-index --add B &&\n- git commit -m \"Add B.\" &&\n+ test_commit \"Add B.\" B Second Second &&\n git checkout -f main &&\n echo Third >>A &&\n git update-index A &&\n@@ -399,6 +394,15 @@ test_expect_success 'switch to branch not checked \nout' '\n git rebase main other\n '\n\n+test_expect_success 'switch to non-branch detaches HEAD' '\n+ git checkout main &&\n+ old_main=$(git rev-parse HEAD) &&\n+ git rebase First Second^0 &&\n+ test_cmp_rev HEAD Second &&\n+ test_cmp_rev main $old_main &&\n+ test_must_fail git symbolic-ref HEAD\n+'\n+\n test_expect_success 'refuse to switch to branch checked out elsewhere' '\n git checkout main &&\n git worktree add wt &&\n\n"},{"id":"451136","messageId":"6BBE6E16-C4CE-4A66-8A0F-1FDBCA0080D4@gmail.com","threadId":"57536","inReplyTo":"7c1c0b8e-7895-7a0e-6ab0-e45e21ec7329@gmail.com","subject":"Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-03-11T15:28:44Z","receivedAt":"2022-03-11T15:28:49Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Phillip,\n\nOn 11 Mar 2022, at 10:05, Phillip Wood wrote:\n\n> Hi John\n>\n> Thanks for working on this\n>\n> On 11/03/2022 05:05, John Cai via GitGitGadget wrote:\n>> From: John Cai \n>>\n>> Fixes a bug whereby rebase updates the deferenced reference HEAD points\n>> to instead of HEAD directly.\n>>\n>> If HEAD is on main and if the following is a fast-forward operation,\n>>\n>> git rebase $(git rev-parse main) $(git rev-parse topic)\n>>\n>> Instead of HEAD being set to $(git rev-parse topic), rebase erroneously\n>> dereferences HEAD and sets main to $(git rev-parse topic). This bug was\n>> reported by Michael McClimon. See [1].\n>\n> Often we just add a Reported-by: trailer unless the liked email has some useful extra info (which arguably should not be the case with a well written commit message)\n\nThanks, will adjust.\n\n>\n>> This is happening because on a fast foward with an oid as a ,\n>> update_refs() will only call update_ref() with REF_NO_DEREF if\n>> RESET_HEAD_DETACH is set. This change was made in 176f5d96 (built-in rebase\n>> --autostash: leave the current branch alone if possible,\n>> 2018-11-07). In rebase, we are not setting the RESET_HEAD_DETACH flag,\n>> which means that the update_ref() call ends up dereferencing\n>> HEAD and updating it to the oid used as .\n>>\n>> The correct behavior is that git rebase should update HEAD to $(git\n>> rev-parse topic) without dereferencing it.\n>>\n>> Fix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date\n>\n> As Junio points out it is confusing that it is always ok to pass that flag, I think we should only set it if we are not checking out a branch, see below.\n>\n>> so that once reset_head() calls update_refs(), it calls update_ref() with\n>> REF_NO_DEREF which updates HEAD directly intead of deferencing it and\n>> updating the branch that HEAD points to.\n>>\n>> Also add a test to ensure this behavior.\n>>\n>> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n>\n> Maybe\n> Reported-by: Michael McClimon \n> ?\n>\n>> Signed-off-by: John Cai \n>> ---\n>> rebase: update HEAD when is an oid\n>> Fixes a bug [1] reported by Michael McClimon where rebase with oids will\n>> erroneously update the branch HEAD points to.\n>> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n>>\n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v1\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v1\n>> Pull-Request: https://github.com/git/git/pull/1226\n>>\n>> builtin/rebase.c | 2 +-\n>> t/t3400-rebase.sh | 21 +++++++++++++++++++++\n>> 2 files changed, 22 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>> index b29ad2b65e7..52afeffcc2e 100644\n>> --- a/builtin/rebase.c\n>> +++ b/builtin/rebase.c\n>> @@ -828,7 +828,7 @@ static int checkout_up_to_date(struct rebase_options *options)\n>> \t\t options->switch_to);\n>> \tropts.oid = &options->orig_head;\n>> \tropts.branch = options->head_name;\n>> -\tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n>> +\tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK | RESET_HEAD_DETACH;\n>\n> I think it would be clearer if the post image ended up as\n>\n> \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK\n> \tif (options->head_name)\n> \t\tropts.branch = option->head_name\n> \telse\n> \t\tropts.flags |= RESET_HEAD_DETACH\n\nYes, this is what I had in mind as well :).\n\n>\n> and we changed reset_head() to BUG() if both branch and RESET_HEAD_DETACH are given.\n\nI didn't consider this though, thanks for the suggestion.\n\n>\n>> \tropts.head_msg = buf.buf;\n>> \tif (reset_head(the_repository, &ropts) < 0)\n>> \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\n>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n>> index 71b1735e1dd..0b92e78976c 100755\n>> --- a/t/t3400-rebase.sh\n>> +++ b/t/t3400-rebase.sh\n>> @@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' '\n>> \t)\n>> '\n>> +test_expect_success 'rebase with oids' '\n>> +\tgit init main-wt &&\n>> +\t(\n>> +\t\tcd main-wt &&\n>> +\t\t>file &&\n>> +\t\tgit add file &&\n>> +\t\tgit commit -m initial &&\n>> +\t\tgit checkout -b side &&\n>> +\t\techo >>file &&\n>> +\t\tgit commit -a -m side &&\n>> +\t\tgit checkout main &&\n>> +\t\tgit tag hold &&\n>> +\t\tgit checkout -B main hold &&\n>> +\t\tgit rev-parse main >pre &&\n>> +\t\tgit rebase $(git rev-parse main) $(git rev-parse side) &&\n>> +\t\tgit rev-parse main >post &&\n>> +\t\ttest \"$(git rev-parse side)\" = \"$(cat .git/HEAD)\" &&\n>> +\t\ttest_cmp pre post\n>> +\t)\n>> +'\n>\n> Using a stand alone test for bisecting makes sense but I think we should try and use the existing test setup for the regression test (it certainly does not need to run in its own worktree). The diff below shows how this could be done. Ideally there would be a preparatory commit that modernized the whole of the setup test rather than just the two commits we're using in the new test but that's not essential.\n\nsounds good to me, might as well clean things up while we're at it.\n\n>\n> Best Wishes\n>\n> Phillip\n>\n> ---- >8 ----\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 71b1735e1d..d5a8ee39fc 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address\n> export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL\n>\n> test_expect_success 'prepare repository with topic branches' '\n> - git config core.logAllRefUpdates true &&\n> - echo First >A &&\n> - git update-index --add A &&\n> - git commit -m \"Add A.\" &&\n> + test_commit \"Add A.\" A First First &&\n> git checkout -b force-3way &&\n> echo Dummy >Y &&\n> git update-index --add Y &&\n> @@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '\n> git mv A D/A &&\n> git commit -m \"Move A.\" &&\n> git checkout -b my-topic-branch main &&\n> - echo Second >B &&\n> - git update-index --add B &&\n> - git commit -m \"Add B.\" &&\n> + test_commit \"Add B.\" B Second Second &&\n> git checkout -f main &&\n> echo Third >>A &&\n> git update-index A &&\n> @@ -399,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '\n> git rebase main other\n> '\n>\n> +test_expect_success 'switch to non-branch detaches HEAD' '\n> + git checkout main &&\n> + old_main=$(git rev-parse HEAD) &&\n> + git rebase First Second^0 &&\n> + test_cmp_rev HEAD Second &&\n> + test_cmp_rev main $old_main &&\n> + test_must_fail git symbolic-ref HEAD\n> +'\n> +\n> test_expect_success 'refuse to switch to branch checked out elsewhere' '\n> git checkout main &&\n> git worktree add wt &&\n"},{"id":"451153","messageId":"pull.1226.v2.git.git.1647019492.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":"pull.1226.git.git.1646975144178.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] rebase: update HEAD when is an oid","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-11T17:24:50Z","receivedAt":"2022-03-11T17:24:57Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Fixes a bug [1] reported by Michael McClimon where rebase with oids will\nerroneously update the branch HEAD points to.\n\n 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n\nThis patch has two parts:\n\n * updates rebase setup test to add some tags we will use, while swapping\n out manual commit creation with the test_commit helper\n * sets RESET_HARD_DETACH flag if branch is not a valid refname\n\nchanges since v1:\n\n * only set RESET_HEAD_DETACH if is not a valid branch\n * updated tests to use existing setup\n\nJohn Cai (2):\n rebase: use test_commit helper in setup\n rebase: set REF_HEAD_DETACH in checkout_up_to_date()\n\n builtin/rebase.c | 5 ++++-\n reset.c | 3 +++\n t/t3400-rebase.sh | 18 +++++++++++-------\n 3 files changed, 18 insertions(+), 8 deletions(-)\n\n\nbase-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v2\nPull-Request: https://github.com/git/git/pull/1226\n\nRange-diff vs v1:\n\n -: ----------- > 1: f3f084adfa6 rebase: use test_commit helper in setup\n 1: 2538fd986d2 ! 2: 0e3c73375c1 rebase: set REF_HEAD_DETACH in checkout_up_to_date()\n @@ Commit message\n git rebase $(git rev-parse main) $(git rev-parse topic)\n \n Instead of HEAD being set to $(git rev-parse topic), rebase erroneously\n - dereferences HEAD and sets main to $(git rev-parse topic). This bug was\n - reported by Michael McClimon. See [1].\n + dereferences HEAD and sets main to $(git rev-parse topic). See [1] for\n + the original bug report.\n \n - This is happening because on a fast foward with an oid as a ,\n - update_refs() will only call update_ref() with REF_NO_DEREF if\n - RESET_HEAD_DETACH is set. This change was made in 176f5d96 (built-in rebase\n - --autostash: leave the current branch alone if possible,\n - 2018-11-07). In rebase, we are not setting the RESET_HEAD_DETACH flag,\n - which means that the update_ref() call ends up dereferencing\n - HEAD and updating it to the oid used as .\n + The callstack from checkout_up_to_date() is the following:\n +\n + cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs()\n + -> update_ref()\n +\n + When is not a valid branch but a sha, rebase sets the head_name\n + of rebase_options to NULL. This value gets passed down this call chain\n + through the branch member of reset_head_opts also getting set to NULL\n + all the way to update_refs(). update_refs() checks ropts.branch to\n + decide whether or not to switch brancheds. If ropts.branch is NULL, it\n + calls update_ref() to update HEAD. At this point however, from rebase's\n + point of view, we want a detached HEAD. But, since checkout_up_to_date()\n + does not set the RESET_HEAD_DETACH flag, the update_ref() call will\n + deference HEAD and update the branch its pointing to, which in the above\n + example is main.\n \n The correct behavior is that git rebase should update HEAD to $(git\n rev-parse topic) without dereferencing it.\n \n Fix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date\n - so that once reset_head() calls update_refs(), it calls update_ref() with\n - REF_NO_DEREF which updates HEAD directly intead of deferencing it and\n - updating the branch that HEAD points to.\n + if is not a valid branch. so that once reset_head() calls\n + update_refs(), it calls update_ref() with REF_NO_DEREF which updates\n + HEAD directly intead of deferencing it and updating the branch that HEAD\n + points to.\n \n Also add a test to ensure this behavior.\n \n 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n \n + Reported-by: Michael McClimon \n Signed-off-by: John Cai \n \n ## builtin/rebase.c ##\n @@ builtin/rebase.c: static int checkout_up_to_date(struct rebase_options *options)\n + \t\t getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n \t\t options->switch_to);\n \tropts.oid = &options->orig_head;\n - \tropts.branch = options->head_name;\n --\tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n -+\tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK | RESET_HEAD_DETACH;\n +-\tropts.branch = options->head_name;\n + \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n ++\tif (options->head_name)\n ++\t\tropts.branch = options->head_name;\n ++\telse\n ++\t\tropts.flags |= RESET_HEAD_DETACH;\n \tropts.head_msg = buf.buf;\n \tif (reset_head(the_repository, &ropts) < 0)\n \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\n \n + ## reset.c ##\n +@@ reset.c: int reset_head(struct repository *r, const struct reset_head_opts *opts)\n + \tif (opts->branch_msg && !opts->branch)\n + \t\tBUG(\"branch reflog message given without a branch\");\n + \n ++\tif (switch_to_branch && opts->flags & RESET_HEAD_DETACH)\n ++\t\tBUG(\"attempting to detach HEAD when branch is given\");\n ++\n + \tif (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {\n + \t\tret = -1;\n + \t\tgoto leave_reset_head;\n +\n ## t/t3400-rebase.sh ##\n -@@ t/t3400-rebase.sh: test_expect_success 'rebase when inside worktree subdirectory' '\n - \t)\n +@@ t/t3400-rebase.sh: test_expect_success 'switch to branch not checked out' '\n + \tgit rebase main other\n '\n \n -+test_expect_success 'rebase with oids' '\n -+\tgit init main-wt &&\n -+\t(\n -+\t\tcd main-wt &&\n -+\t\t>file &&\n -+\t\tgit add file &&\n -+\t\tgit commit -m initial &&\n -+\t\tgit checkout -b side &&\n -+\t\techo >>file &&\n -+\t\tgit commit -a -m side &&\n -+\t\tgit checkout main &&\n -+\t\tgit tag hold &&\n -+\t\tgit checkout -B main hold &&\n -+\t\tgit rev-parse main >pre &&\n -+\t\tgit rebase $(git rev-parse main) $(git rev-parse side) &&\n -+\t\tgit rev-parse main >post &&\n -+\t\ttest \"$(git rev-parse side)\" = \"$(cat .git/HEAD)\" &&\n -+\t\ttest_cmp pre post\n -+\t)\n ++test_expect_success 'switch to non-branch detaches HEAD' '\n ++\tgit checkout main &&\n ++\told_main=$(git rev-parse HEAD) &&\n ++\tgit rebase First Second^0 &&\n ++\ttest_cmp_rev HEAD Second &&\n ++\ttest_cmp_rev main $old_main &&\n ++\ttest_must_fail git symbolic-ref HEAD\n +'\n +\n - test_done\n + test_expect_success 'refuse to switch to branch checked out elsewhere' '\n + \tgit checkout main &&\n + \tgit worktree add wt &&\n\n-- \ngitgitgadget\n"},{"id":"451154","messageId":"f3f084adfa616a7bae7e7c94644f65a36f38652b.1647019492.git.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":"pull.1226.v2.git.git.1647019492.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] rebase: use test_commit helper in setup","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-11T17:24:51Z","receivedAt":"2022-03-11T17:24:59Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nTo prepare for the next commit that will test rebase with oids instead\nof branch names, update the rebase setup test to add a couple of tags we\ncan use. This uses the test_commit helper so we can replace some lines\nthat add a commit manually.\n\nHelped-by: Phillip Wood \nSigned-off-by: John Cai \n---\n t/t3400-rebase.sh | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 71b1735e1dd..0643d015255 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address\n export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL\n \n test_expect_success 'prepare repository with topic branches' '\n-\tgit config core.logAllRefUpdates true &&\n-\techo First >A &&\n-\tgit update-index --add A &&\n-\tgit commit -m \"Add A.\" &&\n+\ttest_commit \"Add A.\" A First First &&\n \tgit checkout -b force-3way &&\n \techo Dummy >Y &&\n \tgit update-index --add Y &&\n@@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '\n \tgit mv A D/A &&\n \tgit commit -m \"Move A.\" &&\n \tgit checkout -b my-topic-branch main &&\n-\techo Second >B &&\n-\tgit update-index --add B &&\n-\tgit commit -m \"Add B.\" &&\n+\ttest_commit \"Add B.\" B Second Second &&\n \tgit checkout -f main &&\n \techo Third >>A &&\n \tgit update-index A &&\n-- \ngitgitgadget\n\n"},{"id":"451155","messageId":"0e3c73375c18a470fd5357b09acefeaf5ca4017f.1647019492.git.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":"pull.1226.v2.git.git.1647019492.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-11T17:24:52Z","receivedAt":"2022-03-11T17:25:03Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nFixes a bug whereby rebase updates the deferenced reference HEAD points\nto instead of HEAD directly.\n\nIf HEAD is on main and if the following is a fast-forward operation,\n\ngit rebase $(git rev-parse main) $(git rev-parse topic)\n\nInstead of HEAD being set to $(git rev-parse topic), rebase erroneously\ndereferences HEAD and sets main to $(git rev-parse topic). See [1] for\nthe original bug report.\n\nThe callstack from checkout_up_to_date() is the following:\n\ncmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs()\n -> update_ref()\n\nWhen is not a valid branch but a sha, rebase sets the head_name\nof rebase_options to NULL. This value gets passed down this call chain\nthrough the branch member of reset_head_opts also getting set to NULL\nall the way to update_refs(). update_refs() checks ropts.branch to\ndecide whether or not to switch brancheds. If ropts.branch is NULL, it\ncalls update_ref() to update HEAD. At this point however, from rebase's\npoint of view, we want a detached HEAD. But, since checkout_up_to_date()\ndoes not set the RESET_HEAD_DETACH flag, the update_ref() call will\ndeference HEAD and update the branch its pointing to, which in the above\nexample is main.\n\nThe correct behavior is that git rebase should update HEAD to $(git\nrev-parse topic) without dereferencing it.\n\nFix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date\nif is not a valid branch. so that once reset_head() calls\nupdate_refs(), it calls update_ref() with REF_NO_DEREF which updates\nHEAD directly intead of deferencing it and updating the branch that HEAD\npoints to.\n\nAlso add a test to ensure this behavior.\n\n1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n\nReported-by: Michael McClimon \nSigned-off-by: John Cai \n---\n builtin/rebase.c | 5 ++++-\n reset.c | 3 +++\n t/t3400-rebase.sh | 9 +++++++++\n 3 files changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b29ad2b65e7..5ae7fa2a169 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -827,8 +827,11 @@ static int checkout_up_to_date(struct rebase_options *options)\n \t\t getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n \t\t options->switch_to);\n \tropts.oid = &options->orig_head;\n-\tropts.branch = options->head_name;\n \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n+\tif (options->head_name)\n+\t\tropts.branch = options->head_name;\n+\telse\n+\t\tropts.flags |= RESET_HEAD_DETACH;\n \tropts.head_msg = buf.buf;\n \tif (reset_head(the_repository, &ropts) < 0)\n \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\ndiff --git a/reset.c b/reset.c\nindex e3383a93343..f8e32fcc240 100644\n--- a/reset.c\n+++ b/reset.c\n@@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n \tif (opts->branch_msg && !opts->branch)\n \t\tBUG(\"branch reflog message given without a branch\");\n \n+\tif (switch_to_branch && opts->flags & RESET_HEAD_DETACH)\n+\t\tBUG(\"attempting to detach HEAD when branch is given\");\n+\n \tif (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {\n \t\tret = -1;\n \t\tgoto leave_reset_head;\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 0643d015255..d5a8ee39fc4 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -394,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '\n \tgit rebase main other\n '\n \n+test_expect_success 'switch to non-branch detaches HEAD' '\n+\tgit checkout main &&\n+\told_main=$(git rev-parse HEAD) &&\n+\tgit rebase First Second^0 &&\n+\ttest_cmp_rev HEAD Second &&\n+\ttest_cmp_rev main $old_main &&\n+\ttest_must_fail git symbolic-ref HEAD\n+'\n+\n test_expect_success 'refuse to switch to branch checked out elsewhere' '\n \tgit checkout main &&\n \tgit worktree add wt &&\n-- \ngitgitgadget\n"},{"id":"451159","messageId":"C97F4239-AA5F-4623-A15E-1E90927DA758@gmail.com","threadId":"57536","inReplyTo":"7c1c0b8e-7895-7a0e-6ab0-e45e21ec7329@gmail.com","subject":"Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-03-11T19:42:12Z","receivedAt":"2022-03-11T19:42:18Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"\nOn 11 Mar 2022, at 10:05, Phillip Wood wrote:\n\n> Hi John\n>\n> Thanks for working on this\n>\n> On 11/03/2022 05:05, John Cai via GitGitGadget wrote:\n>> From: John Cai \n>>\n>> Fixes a bug whereby rebase updates the deferenced reference HEAD points\n>> to instead of HEAD directly.\n>>\n>> If HEAD is on main and if the following is a fast-forward operation,\n>>\n>> git rebase $(git rev-parse main) $(git rev-parse topic)\n>>\n>> Instead of HEAD being set to $(git rev-parse topic), rebase erroneously\n>> dereferences HEAD and sets main to $(git rev-parse topic). This bug was\n>> reported by Michael McClimon. See [1].\n>\n> Often we just add a Reported-by: trailer unless the liked email has some useful extra info (which arguably should not be the case with a well written commit message)\n>\n>> This is happening because on a fast foward with an oid as a ,\n>> update_refs() will only call update_ref() with REF_NO_DEREF if\n>> RESET_HEAD_DETACH is set. This change was made in 176f5d96 (built-in rebase\n>> --autostash: leave the current branch alone if possible,\n>> 2018-11-07). In rebase, we are not setting the RESET_HEAD_DETACH flag,\n>> which means that the update_ref() call ends up dereferencing\n>> HEAD and updating it to the oid used as .\n>>\n>> The correct behavior is that git rebase should update HEAD to $(git\n>> rev-parse topic) without dereferencing it.\n>>\n>> Fix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date\n>\n> As Junio points out it is confusing that it is always ok to pass that flag, I think we should only set it if we are not checking out a branch, see below.\n>\n>> so that once reset_head() calls update_refs(), it calls update_ref() with\n>> REF_NO_DEREF which updates HEAD directly intead of deferencing it and\n>> updating the branch that HEAD points to.\n>>\n>> Also add a test to ensure this behavior.\n>>\n>> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n>\n> Maybe\n> Reported-by: Michael McClimon \n> ?\n>\n>> Signed-off-by: John Cai \n>> ---\n>> rebase: update HEAD when is an oid\n>> Fixes a bug [1] reported by Michael McClimon where rebase with oids will\n>> erroneously update the branch HEAD points to.\n>> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n>>\n>> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v1\n>> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v1\n>> Pull-Request: https://github.com/git/git/pull/1226\n>>\n>> builtin/rebase.c | 2 +-\n>> t/t3400-rebase.sh | 21 +++++++++++++++++++++\n>> 2 files changed, 22 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>> index b29ad2b65e7..52afeffcc2e 100644\n>> --- a/builtin/rebase.c\n>> +++ b/builtin/rebase.c\n>> @@ -828,7 +828,7 @@ static int checkout_up_to_date(struct rebase_options *options)\n>> \t\t options->switch_to);\n>> \tropts.oid = &options->orig_head;\n>> \tropts.branch = options->head_name;\n>> -\tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n>> +\tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK | RESET_HEAD_DETACH;\n>\n> I think it would be clearer if the post image ended up as\n\nThis is entirely for my own sake and revealing my ignorance, but what exactly\ndoes \"pre\" and \"post\" image refer to?\n\n>\n> \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK\n> \tif (options->head_name)\n> \t\tropts.branch = option->head_name\n> \telse\n> \t\tropts.flags |= RESET_HEAD_DETACH\n>\n> and we changed reset_head() to BUG() if both branch and RESET_HEAD_DETACH are given.\n>\n>> \tropts.head_msg = buf.buf;\n>> \tif (reset_head(the_repository, &ropts) < 0)\n>> \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\n>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n>> index 71b1735e1dd..0b92e78976c 100755\n>> --- a/t/t3400-rebase.sh\n>> +++ b/t/t3400-rebase.sh\n>> @@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' '\n>> \t)\n>> '\n>> +test_expect_success 'rebase with oids' '\n>> +\tgit init main-wt &&\n>> +\t(\n>> +\t\tcd main-wt &&\n>> +\t\t>file &&\n>> +\t\tgit add file &&\n>> +\t\tgit commit -m initial &&\n>> +\t\tgit checkout -b side &&\n>> +\t\techo >>file &&\n>> +\t\tgit commit -a -m side &&\n>> +\t\tgit checkout main &&\n>> +\t\tgit tag hold &&\n>> +\t\tgit checkout -B main hold &&\n>> +\t\tgit rev-parse main >pre &&\n>> +\t\tgit rebase $(git rev-parse main) $(git rev-parse side) &&\n>> +\t\tgit rev-parse main >post &&\n>> +\t\ttest \"$(git rev-parse side)\" = \"$(cat .git/HEAD)\" &&\n>> +\t\ttest_cmp pre post\n>> +\t)\n>> +'\n>\n> Using a stand alone test for bisecting makes sense but I think we should try and use the existing test setup for the regression test (it certainly does not need to run in its own worktree). The diff below shows how this could be done. Ideally there would be a preparatory commit that modernized the whole of the setup test rather than just the two commits we're using in the new test but that's not essential.\n>\n> Best Wishes\n>\n> Phillip\n>\n> ---- >8 ----\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 71b1735e1d..d5a8ee39fc 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address\n> export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL\n>\n> test_expect_success 'prepare repository with topic branches' '\n> - git config core.logAllRefUpdates true &&\n> - echo First >A &&\n> - git update-index --add A &&\n> - git commit -m \"Add A.\" &&\n> + test_commit \"Add A.\" A First First &&\n> git checkout -b force-3way &&\n> echo Dummy >Y &&\n> git update-index --add Y &&\n> @@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '\n> git mv A D/A &&\n> git commit -m \"Move A.\" &&\n> git checkout -b my-topic-branch main &&\n> - echo Second >B &&\n> - git update-index --add B &&\n> - git commit -m \"Add B.\" &&\n> + test_commit \"Add B.\" B Second Second &&\n> git checkout -f main &&\n> echo Third >>A &&\n> git update-index A &&\n> @@ -399,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '\n> git rebase main other\n> '\n>\n> +test_expect_success 'switch to non-branch detaches HEAD' '\n> + git checkout main &&\n> + old_main=$(git rev-parse HEAD) &&\n> + git rebase First Second^0 &&\n> + test_cmp_rev HEAD Second &&\n> + test_cmp_rev main $old_main &&\n> + test_must_fail git symbolic-ref HEAD\n> +'\n> +\n> test_expect_success 'refuse to switch to branch checked out elsewhere' '\n> git checkout main &&\n> git worktree add wt &&\n"},{"id":"451172","messageId":"16c02a8a-46bd-0ddd-aca7-418b1ed96c6d@gmail.com","threadId":"57536","inReplyTo":"C97F4239-AA5F-4623-A15E-1E90927DA758@gmail.com","subject":"Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-11T21:31:43Z","receivedAt":"2022-03-11T22:50:18Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi John\n\nOn 11/03/2022 19:42, John Cai wrote:\n> [...]\n>>\n>> I think it would be clearer if the post image ended up as\n> \n> This is entirely for my own sake and revealing my ignorance, but what exactly\n> does \"pre\" and \"post\" image refer to?\n\nThe old version and the new version\n\nBest Wishes\n\nPhillip\n"},{"id":"451236","messageId":"xmqqee36watb.fsf@gitster.g","threadId":"57536","inReplyTo":"f3f084adfa616a7bae7e7c94644f65a36f38652b.1647019492.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] rebase: use test_commit helper in setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-13T07:50:24Z","receivedAt":"2022-03-13T07:51:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> To prepare for the next commit that will test rebase with oids instead\n> of branch names, update the rebase setup test to add a couple of tags we\n> can use. This uses the test_commit helper so we can replace some lines\n> that add a commit manually.\n\nOK.\n\n> test_expect_success 'prepare repository with topic branches' '\n> -\tgit config core.logAllRefUpdates true &&\n\nThis lossage is not explained. I do not know if we actually make\nuse of the reflog in the tests, though.\n\n> -\techo First >A &&\n> -\tgit update-index --add A &&\n> -\tgit commit -m \"Add A.\" &&\n> +\ttest_commit \"Add A.\" A First First &&\n> \tgit checkout -b force-3way &&\n> \techo Dummy >Y &&\n> \tgit update-index --add Y &&\n> @@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '\n> \tgit mv A D/A &&\n> \tgit commit -m \"Move A.\" &&\n> \tgit checkout -b my-topic-branch main &&\n> -\techo Second >B &&\n> -\tgit update-index --add B &&\n> -\tgit commit -m \"Add B.\" &&\n> +\ttest_commit \"Add B.\" B Second Second &&\n> \tgit checkout -f main &&\n> \techo Third >>A &&\n> \tgit update-index A &&\n"},{"id":"451237","messageId":"xmqq7d8ywaf0.fsf@gitster.g","threadId":"57536","inReplyTo":"0e3c73375c18a470fd5357b09acefeaf5ca4017f.1647019492.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-13T07:58:59Z","receivedAt":"2022-03-13T07:59:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> diff --git a/reset.c b/reset.c\n> index e3383a93343..f8e32fcc240 100644\n> --- a/reset.c\n> +++ b/reset.c\n> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n> \tif (opts->branch_msg && !opts->branch)\n> \t\tBUG(\"branch reflog message given without a branch\");\n> \n> +\tif (switch_to_branch && opts->flags & RESET_HEAD_DETACH)\n\nIt's just style thing but it probably is easier to read to have\nan extra () around the bitwise-&.\n\n> +\t\tBUG(\"attempting to detach HEAD when branch is given\");\n\nI wonder if there is a valid use case NOT to use RESET_HEAD_DETACH\nwhen switch_to_branch == NULL. If there isn't, it could be that\nwe can get rid of RESET_HEAD_DETACH bit and base this decision\nsolely on switch_to_branch'es NULLness.\n\nBut that is totally outside the scope of this fix. I'd prefer to\nsee a narrowly and sharply focused fix, and to be quite honest, I\nwould be happier if this assertion weren't in this topic.\n\n> \tif (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {\n> \t\tret = -1;\n> \t\tgoto leave_reset_head;\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 0643d015255..d5a8ee39fc4 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -394,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '\n> \tgit rebase main other\n> '\n> \n> +test_expect_success 'switch to non-branch detaches HEAD' '\n> +\tgit checkout main &&\n> +\told_main=$(git rev-parse HEAD) &&\n> +\tgit rebase First Second^0 &&\n\nGood. For reproduction, the fork-point \"First\" does not have to be\na raw object name. \"Second^0\" not being a branch name does matter.\n\n> +\ttest_cmp_rev HEAD Second &&\n> +\ttest_cmp_rev main $old_main &&\n> +\ttest_must_fail git symbolic-ref HEAD\n\nAll correct and to the point. Good.\n\nWill queue. Thanks.\n\n> +'\n> +\n> test_expect_success 'refuse to switch to branch checked out elsewhere' '\n> \tgit checkout main &&\n> \tgit worktree add wt &&\n"},{"id":"451303","messageId":"4072d712-4138-6cf0-e0e0-5a3303ce010a@gmail.com","threadId":"57536","inReplyTo":"xmqqee36watb.fsf@gitster.g","subject":"Re: [PATCH v2 1/2] rebase: use test_commit helper in setup","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-14T10:52:34Z","receivedAt":"2022-03-14T10:52:39Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 13/03/2022 07:50, Junio C Hamano wrote:\n> \"John Cai via GitGitGadget\" writes:\n> \n>> From: John Cai \n>>\n>> To prepare for the next commit that will test rebase with oids instead\n>> of branch names, update the rebase setup test to add a couple of tags we\n>> can use. This uses the test_commit helper so we can replace some lines\n>> that add a commit manually.\n> \n> OK.\n> \n>> test_expect_success 'prepare repository with topic branches' '\n>> -\tgit config core.logAllRefUpdates true &&\n> \n> This lossage is not explained. I do not know if we actually make\n> use of the reflog in the tests, though.\n\nIt is the default these days so we don't need to waste a process setting \nit here.\n\nBest Wishes\n\nPhillip\n"},{"id":"451304","messageId":"b35edfb8-9b48-d5b3-8d77-cb61a22f85ef@gmail.com","threadId":"57536","inReplyTo":"xmqq7d8ywaf0.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-14T10:54:09Z","receivedAt":"2022-03-14T10:54:15Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 13/03/2022 07:58, Junio C Hamano wrote:\n> \"John Cai via GitGitGadget\" writes:\n> \n>> diff --git a/reset.c b/reset.c\n>> index e3383a93343..f8e32fcc240 100644\n>> --- a/reset.c\n>> +++ b/reset.c\n>> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n>> \tif (opts->branch_msg && !opts->branch)\n>> \t\tBUG(\"branch reflog message given without a branch\");\n>> \n>> +\tif (switch_to_branch && opts->flags & RESET_HEAD_DETACH)\n> \n> It's just style thing but it probably is easier to read to have\n> an extra () around the bitwise-&.\n> \n>> +\t\tBUG(\"attempting to detach HEAD when branch is given\");\n> \n> I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH\n> when switch_to_branch == NULL. If there isn't, it could be that\n> we can get rid of RESET_HEAD_DETACH bit and base this decision\n> solely on switch_to_branch'es NULLness.\n\n\"rebase --skip\" and \"rebase --autostash\" are two such uses I think\n\nBest Wishes\n\nPhillip\n"},{"id":"451309","messageId":"d5151784-3218-ab85-8226-f3d941f649b2@gmail.com","threadId":"57536","inReplyTo":"b35edfb8-9b48-d5b3-8d77-cb61a22f85ef@gmail.com","subject":"Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-14T14:05:34Z","receivedAt":"2022-03-14T14:05:45Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 14/03/2022 10:54, Phillip Wood wrote:\n> On 13/03/2022 07:58, Junio C Hamano wrote:\n>> \"John Cai via GitGitGadget\" writes:\n>>\n>>> diff --git a/reset.c b/reset.c\n>>> index e3383a93343..f8e32fcc240 100644\n>>> --- a/reset.c\n>>> +++ b/reset.c\n>>> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct \n>>> reset_head_opts *opts)\n>>>       if (opts->branch_msg && !opts->branch)\n>>>           BUG(\"branch reflog message given without a branch\");\n>>> +    if (switch_to_branch && opts->flags & RESET_HEAD_DETACH)\n>>\n>> It's just style thing but it probably is easier to read to have\n>> an extra () around the bitwise-&.\n>>\n>>> +        BUG(\"attempting to detach HEAD when branch is given\");\n>>\n>> I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH\n>> when switch_to_branch == NULL.  If there isn't, it could be that\n>> we can get rid of RESET_HEAD_DETACH bit and base this decision\n>> solely on switch_to_branch'es NULLness.\n> \n> \"rebase --skip\" and \"rebase --autostash\" are two such uses I think\n\nThose don't update any refs though so are not helpful examples. Possibly \nwhen we fast-forward because HEAD is an ancestor of 'onto' is a \npotential case but at the moment I think we detach HEAD when checking \nout 'onto' and then update and checkout the branch if there is one (I've \nbeen thinking about fixing that so we only detach HEAD if we're going to \nrebase).\n\nBest Wishes\n\nPhillip\n\n> Best Wishes\n> \n> Phillip\n\n"},{"id":"451310","messageId":"4313B834-90B4-41B3-B810-746EF91F8BBB@gmail.com","threadId":"57536","inReplyTo":"xmqq7d8ywaf0.fsf@gitster.g","subject":"Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-03-14T14:11:18Z","receivedAt":"2022-03-14T14:11:24Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 13 Mar 2022, at 3:58, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" writes:\n>\n>> diff --git a/reset.c b/reset.c\n>> index e3383a93343..f8e32fcc240 100644\n>> --- a/reset.c\n>> +++ b/reset.c\n>> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n>> \tif (opts->branch_msg && !opts->branch)\n>> \t\tBUG(\"branch reflog message given without a branch\");\n>>\n>> +\tif (switch_to_branch && opts->flags & RESET_HEAD_DETACH)\n>\n> It's just style thing but it probably is easier to read to have\n> an extra () around the bitwise-&.\n>\n>> +\t\tBUG(\"attempting to detach HEAD when branch is given\");\n>\n> I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH\n> when switch_to_branch == NULL. If there isn't, it could be that\n> we can get rid of RESET_HEAD_DETACH bit and base this decision\n> solely on switch_to_branch'es NULLness.\n>\n> But that is totally outside the scope of this fix. I'd prefer to\n> see a narrowly and sharply focused fix, and to be quite honest, I\n> would be happier if this assertion weren't in this topic.\n\nI'm okay with taking this out, but would be good to get an ack from Phillip.\n\n>\n>> \tif (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {\n>> \t\tret = -1;\n>> \t\tgoto leave_reset_head;\n>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n>> index 0643d015255..d5a8ee39fc4 100755\n>> --- a/t/t3400-rebase.sh\n>> +++ b/t/t3400-rebase.sh\n>> @@ -394,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '\n>> \tgit rebase main other\n>> '\n>>\n>> +test_expect_success 'switch to non-branch detaches HEAD' '\n>> +\tgit checkout main &&\n>> +\told_main=$(git rev-parse HEAD) &&\n>> +\tgit rebase First Second^0 &&\n>\n> Good. For reproduction, the fork-point \"First\" does not have to be\n> a raw object name. \"Second^0\" not being a branch name does matter.\n>\n>> +\ttest_cmp_rev HEAD Second &&\n>> +\ttest_cmp_rev main $old_main &&\n>> +\ttest_must_fail git symbolic-ref HEAD\n>\n> All correct and to the point. Good.\n\nThanks to Phillip for his help on this!\n\n>\n> Will queue. Thanks.\n\nMore of a question about protocol. If there are some minor adjustments that are\nnot really worth more disussion on the ML (like removing the BUG assertion),\nis it still protocol to send another series or just re-roll on the branch without\nsending out another patch series?\n\nThanks!\nJohn\n\n>\n>> +'\n>> +\n>> test_expect_success 'refuse to switch to branch checked out elsewhere' '\n>> \tgit checkout main &&\n>> \tgit worktree add wt &&\n"},{"id":"451311","messageId":"2aa6bd95-caf1-2882-e0a6-2fbf9b0d9741@gmail.com","threadId":"57536","inReplyTo":"4313B834-90B4-41B3-B810-746EF91F8BBB@gmail.com","subject":"Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-14T14:25:00Z","receivedAt":"2022-03-14T14:25:09Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi John\n\nOn 14/03/2022 14:11, John Cai wrote:\n> Hi Junio,\n> \n> On 13 Mar 2022, at 3:58, Junio C Hamano wrote:\n> \n>> \"John Cai via GitGitGadget\" writes:\n>>\n>>> diff --git a/reset.c b/reset.c\n>>> index e3383a93343..f8e32fcc240 100644\n>>> --- a/reset.c\n>>> +++ b/reset.c\n>>> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts)\n>>> \tif (opts->branch_msg && !opts->branch)\n>>> \t\tBUG(\"branch reflog message given without a branch\");\n>>>\n>>> +\tif (switch_to_branch && opts->flags & RESET_HEAD_DETACH)\n>>\n>> It's just style thing but it probably is easier to read to have\n>> an extra () around the bitwise-&.\n>>\n>>> +\t\tBUG(\"attempting to detach HEAD when branch is given\");\n>>\n>> I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH\n>> when switch_to_branch == NULL. If there isn't, it could be that\n>> we can get rid of RESET_HEAD_DETACH bit and base this decision\n>> solely on switch_to_branch'es NULLness.\n>>\n>> But that is totally outside the scope of this fix. I'd prefer to\n>> see a narrowly and sharply focused fix, and to be quite honest, I\n>> would be happier if this assertion weren't in this topic.\n> \n> I'm okay with taking this out, but would be good to get an ack from Phillip.\n\nThat's fine with me. The rest of the patch looks good as far as I'm \nconcerned.\n\nBest Wishes\n\nPhillip\n"},{"id":"451352","messageId":"xmqqk0cwnr4p.fsf@gitster.g","threadId":"57536","inReplyTo":"4072d712-4138-6cf0-e0e0-5a3303ce010a@gmail.com","subject":"Re: [PATCH v2 1/2] rebase: use test_commit helper in setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-14T21:47:18Z","receivedAt":"2022-03-14T21:47:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Phillip Wood writes:\n\n> On 13/03/2022 07:50, Junio C Hamano wrote:\n>> \"John Cai via GitGitGadget\" writes:\n>> \n>>> From: John Cai \n>>>\n>>> To prepare for the next commit that will test rebase with oids instead\n>>> of branch names, update the rebase setup test to add a couple of tags we\n>>> can use. This uses the test_commit helper so we can replace some lines\n>>> that add a commit manually.\n>> OK.\n>> \n>>> test_expect_success 'prepare repository with topic branches' '\n>>> -\tgit config core.logAllRefUpdates true &&\n>> This lossage is not explained. I do not know if we actually make\n>> use of the reflog in the tests, though.\n>\n> It is the default these days so we don't need to waste a process\n> setting it here.\n\nI said \"not explained\" in the log message. I didn't ask anybody to\nexplain it to me or to readers on the list.\n\nThanks.\n"},{"id":"451533","messageId":"bd1c9537ffc503707690ed173b9e6e808d58ce5d.1647487001.git.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":"pull.1226.v3.git.git.1647487001.gitgitgadget@gmail.com","subject":"[PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-17T03:16:41Z","receivedAt":"2022-03-17T03:17:14Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nFixes a bug whereby rebase updates the deferenced reference HEAD points\nto instead of HEAD directly.\n\nIf HEAD is on main and if the following is a fast-forward operation,\n\ngit rebase $(git rev-parse main) $(git rev-parse topic)\n\nInstead of HEAD being set to $(git rev-parse topic), rebase erroneously\ndereferences HEAD and sets main to $(git rev-parse topic). See [1] for\nthe original bug report.\n\nThe callstack from checkout_up_to_date() is the following:\n\ncmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs()\n -> update_ref()\n\nWhen is not a valid branch but a sha, rebase sets the head_name\nof rebase_options to NULL. This value gets passed down this call chain\nthrough the branch member of reset_head_opts also getting set to NULL\nall the way to update_refs(). update_refs() checks ropts.branch to\ndecide whether or not to switch brancheds. If ropts.branch is NULL, it\ncalls update_ref() to update HEAD. At this point however, from rebase's\npoint of view, we want a detached HEAD. But, since checkout_up_to_date()\ndoes not set the RESET_HEAD_DETACH flag, the update_ref() call will\ndeference HEAD and update the branch its pointing to, which in the above\nexample is main.\n\nThe correct behavior is that git rebase should update HEAD to $(git\nrev-parse topic) without dereferencing it.\n\nFix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date\nif is not a valid branch. so that once reset_head() calls\nupdate_refs(), it calls update_ref() with REF_NO_DEREF which updates\nHEAD directly intead of deferencing it and updating the branch that HEAD\npoints to.\n\nAlso add a test to ensure this behavior.\n\n1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n\nReported-by: Michael McClimon \nSigned-off-by: John Cai \n---\n builtin/rebase.c | 5 ++++-\n t/t3400-rebase.sh | 9 +++++++++\n 2 files changed, 13 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b29ad2b65e7..5ae7fa2a169 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -827,8 +827,11 @@ static int checkout_up_to_date(struct rebase_options *options)\n \t\t getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n \t\t options->switch_to);\n \tropts.oid = &options->orig_head;\n-\tropts.branch = options->head_name;\n \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n+\tif (options->head_name)\n+\t\tropts.branch = options->head_name;\n+\telse\n+\t\tropts.flags |= RESET_HEAD_DETACH;\n \tropts.head_msg = buf.buf;\n \tif (reset_head(the_repository, &ropts) < 0)\n \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 0643d015255..d5a8ee39fc4 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -394,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '\n \tgit rebase main other\n '\n \n+test_expect_success 'switch to non-branch detaches HEAD' '\n+\tgit checkout main &&\n+\told_main=$(git rev-parse HEAD) &&\n+\tgit rebase First Second^0 &&\n+\ttest_cmp_rev HEAD Second &&\n+\ttest_cmp_rev main $old_main &&\n+\ttest_must_fail git symbolic-ref HEAD\n+'\n+\n test_expect_success 'refuse to switch to branch checked out elsewhere' '\n \tgit checkout main &&\n \tgit worktree add wt &&\n-- \ngitgitgadget\n"},{"id":"451534","messageId":"pull.1226.v3.git.git.1647487001.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":"pull.1226.v2.git.git.1647019492.gitgitgadget@gmail.com","subject":"[PATCH v3 0/2] rebase: update HEAD when is an oid","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-17T03:16:39Z","receivedAt":"2022-03-17T03:17:17Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Fixes a bug [1] reported by Michael McClimon where rebase with oids will\nerroneously update the branch HEAD points to.\n\n 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n\nThis patch has two parts:\n\n * updates rebase setup test to add some tags we will use, while swapping\n out manual commit creation with the test_commit helper\n * sets RESET_HARD_DETACH flag if branch is not a valid refname\n\nchanges since v2:\n\n * remove BUG assertion\n\nchanges since v1:\n\n * only set RESET_HEAD_DETACH if is not a valid branch\n * updated tests to use existing setup\n\nJohn Cai (2):\n rebase: use test_commit helper in setup\n rebase: set REF_HEAD_DETACH in checkout_up_to_date()\n\n builtin/rebase.c | 5 ++++-\n t/t3400-rebase.sh | 18 +++++++++++-------\n 2 files changed, 15 insertions(+), 8 deletions(-)\n\n\nbase-commit: b896f729e240d250cf56899e6a0073f6aa469f5d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v3\nPull-Request: https://github.com/git/git/pull/1226\n\nRange-diff vs v2:\n\n 1: f3f084adfa6 ! 1: f658eb00bcd rebase: use test_commit helper in setup\n @@ Commit message\n can use. This uses the test_commit helper so we can replace some lines\n that add a commit manually.\n \n + Setting logAllRefUpdates is not necessary because it's on by default for\n + repositories with a working tree.\n +\n Helped-by: Phillip Wood \n Signed-off-by: John Cai \n \n 2: 0e3c73375c1 ! 2: bd1c9537ffc rebase: set REF_HEAD_DETACH in checkout_up_to_date()\n @@ builtin/rebase.c: static int checkout_up_to_date(struct rebase_options *options)\n \tif (reset_head(the_repository, &ropts) < 0)\n \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\n \n - ## reset.c ##\n -@@ reset.c: int reset_head(struct repository *r, const struct reset_head_opts *opts)\n - \tif (opts->branch_msg && !opts->branch)\n - \t\tBUG(\"branch reflog message given without a branch\");\n - \n -+\tif (switch_to_branch && opts->flags & RESET_HEAD_DETACH)\n -+\t\tBUG(\"attempting to detach HEAD when branch is given\");\n -+\n - \tif (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {\n - \t\tret = -1;\n - \t\tgoto leave_reset_head;\n -\n ## t/t3400-rebase.sh ##\n @@ t/t3400-rebase.sh: test_expect_success 'switch to branch not checked out' '\n \tgit rebase main other\n\n-- \ngitgitgadget\n"},{"id":"451535","messageId":"f658eb00bcd2429859bc7b0d3b19bfa8a7791ca8.1647487001.git.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":"pull.1226.v3.git.git.1647487001.gitgitgadget@gmail.com","subject":"[PATCH v3 1/2] rebase: use test_commit helper in setup","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-17T03:16:40Z","receivedAt":"2022-03-17T03:17:21Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nTo prepare for the next commit that will test rebase with oids instead\nof branch names, update the rebase setup test to add a couple of tags we\ncan use. This uses the test_commit helper so we can replace some lines\nthat add a commit manually.\n\nSetting logAllRefUpdates is not necessary because it's on by default for\nrepositories with a working tree.\n\nHelped-by: Phillip Wood \nSigned-off-by: John Cai \n---\n t/t3400-rebase.sh | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 71b1735e1dd..0643d015255 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address\n export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL\n \n test_expect_success 'prepare repository with topic branches' '\n-\tgit config core.logAllRefUpdates true &&\n-\techo First >A &&\n-\tgit update-index --add A &&\n-\tgit commit -m \"Add A.\" &&\n+\ttest_commit \"Add A.\" A First First &&\n \tgit checkout -b force-3way &&\n \techo Dummy >Y &&\n \tgit update-index --add Y &&\n@@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '\n \tgit mv A D/A &&\n \tgit commit -m \"Move A.\" &&\n \tgit checkout -b my-topic-branch main &&\n-\techo Second >B &&\n-\tgit update-index --add B &&\n-\tgit commit -m \"Add B.\" &&\n+\ttest_commit \"Add B.\" B Second Second &&\n \tgit checkout -f main &&\n \techo Third >>A &&\n \tgit update-index A &&\n-- \ngitgitgadget\n\n"},{"id":"451558","messageId":"220317.86v8wcd7ig.gmgdl@evledraar.gmail.com","threadId":"57536","inReplyTo":"f658eb00bcd2429859bc7b0d3b19bfa8a7791ca8.1647487001.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/2] rebase: use test_commit helper in setup","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-17T13:37:06Z","receivedAt":"2022-03-17T13:38:06Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 17 2022, John Cai via GitGitGadget wrote:\n\n> From: John Cai \n>\n> To prepare for the next commit that will test rebase with oids instead\n> of branch names, update the rebase setup test to add a couple of tags we\n> can use. This uses the test_commit helper so we can replace some lines\n> that add a commit manually.\n>\n> Setting logAllRefUpdates is not necessary because it's on by default for\n> repositories with a working tree.\n>\n> Helped-by: Phillip Wood \n> Signed-off-by: John Cai \n> ---\n> t/t3400-rebase.sh | 9 ++-------\n> 1 file changed, 2 insertions(+), 7 deletions(-)\n>\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 71b1735e1dd..0643d015255 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address\n> export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL\n> \n> test_expect_success 'prepare repository with topic branches' '\n> -\tgit config core.logAllRefUpdates true &&\n> -\techo First >A &&\n> -\tgit update-index --add A &&\n> -\tgit commit -m \"Add A.\" &&\n> +\ttest_commit \"Add A.\" A First First &&\n> \tgit checkout -b force-3way &&\n> \techo Dummy >Y &&\n> \tgit update-index --add Y &&\n> @@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '\n> \tgit mv A D/A &&\n> \tgit commit -m \"Move A.\" &&\n> \tgit checkout -b my-topic-branch main &&\n> -\techo Second >B &&\n> -\tgit update-index --add B &&\n> -\tgit commit -m \"Add B.\" &&\n> +\ttest_commit \"Add B.\" B Second Second &&\n> \tgit checkout -f main &&\n> \techo Third >>A &&\n> \tgit update-index A &&\n\nAt least these could also be done, and skimming there's a few more \"git\ncommit\" that could also be converted:\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 0643d015255..6dc8df8be7e 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -35,9 +35,7 @@ test_expect_success 'prepare repository with topic branches' '\n \tgit update-index A &&\n \tgit commit -m \"Modify A.\" &&\n \tgit checkout -b side my-topic-branch &&\n-\techo Side >>C &&\n-\tgit add C &&\n-\tgit commit -m \"Add C\" &&\n+\ttest_commit --no-tag \"Add C\" C Side &&\n \tgit checkout -f my-topic-branch &&\n \tgit tag topic\n '\n@@ -114,10 +112,7 @@ test_expect_success 'rebase off of the previous branch using \"-\"' '\n test_expect_success 'rebase a single mode change' '\n \tgit checkout main &&\n \tgit branch -D topic &&\n-\techo 1 >X &&\n-\tgit add X &&\n-\ttest_tick &&\n-\tgit commit -m prepare &&\n+\ttest_commit prepare X 1 &&\n \tgit checkout -b modechange HEAD^ &&\n \techo 1 >X &&\n \tgit add X &&\n\nMaybe worthwhile squashing in & doing the rest?\n\nNote that --no-tag and --append allow for converting more than could be\nin the past before those (relatively recently added) options.\n"},{"id":"451559","messageId":"220317.86r170d6zs.gmgdl@evledraar.gmail.com","threadId":"57536","inReplyTo":"bd1c9537ffc503707690ed173b9e6e808d58ce5d.1647487001.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-03-17T13:42:53Z","receivedAt":"2022-03-17T13:49:24Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Mar 17 2022, John Cai via GitGitGadget wrote:\n\n> From: John Cai \n>\n> Fixes a bug whereby rebase updates the deferenced reference HEAD points\n> to instead of HEAD directly.\n>\n> If HEAD is on main and if the following is a fast-forward operation,\n>\n> git rebase $(git rev-parse main) $(git rev-parse topic)\n>\n> Instead of HEAD being set to $(git rev-parse topic), rebase erroneously\n> dereferences HEAD and sets main to $(git rev-parse topic). See [1] for\n> the original bug report.\n>\n> The callstack from checkout_up_to_date() is the following:\n>\n> cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs()\n> -> update_ref()\n>\n> When is not a valid branch but a sha, rebase sets the head_name\n\n..but an OID...\n\n> of rebase_options to NULL. This value gets passed down this call chain\n> through the branch member of reset_head_opts also getting set to NULL\n> all the way to update_refs(). update_refs() checks ropts.branch to\n> decide whether or not to switch brancheds. If ropts.branch is NULL, it\n\nbrancheds -> branches.\n\nAnd maybe a new paragraph before \"update_refs()\"? I.e. \"\\n\\nThen\nupdate_refs() checks...\" ?\n\n> calls update_ref() to update HEAD. At this point however, from rebase's\n> point of view, we want a detached HEAD. But, since checkout_up_to_date()\n> does not set the RESET_HEAD_DETACH flag, the update_ref() call will\n> deference HEAD and update the branch its pointing to, which in the above\n> example is main.\n>\n> The correct behavior is that git rebase should update HEAD to $(git\n> rev-parse topic) without dereferencing it.\n>\n> Fix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date\n> if is not a valid branch. so that once reset_head() calls\n> update_refs(), it calls update_ref() with REF_NO_DEREF which updates\n> HEAD directly intead of deferencing it and updating the branch that HEAD\n> points to.\n\nBut on the \"tell\" v.s. show... (more below)...\n>\n> Also add a test to ensure this behavior.\n>\n> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n>\n> Reported-by: Michael McClimon \n> Signed-off-by: John Cai \n> ---\n> builtin/rebase.c | 5 ++++-\n> t/t3400-rebase.sh | 9 +++++++++\n> 2 files changed, 13 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index b29ad2b65e7..5ae7fa2a169 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -827,8 +827,11 @@ static int checkout_up_to_date(struct rebase_options *options)\n> \t\t getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n> \t\t options->switch_to);\n> \tropts.oid = &options->orig_head;\n> -\tropts.branch = options->head_name;\n> \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n> +\tif (options->head_name)\n> +\t\tropts.branch = options->head_name;\n> +\telse\n> +\t\tropts.flags |= RESET_HEAD_DETACH;\n> \tropts.head_msg = buf.buf;\n> \tif (reset_head(the_repository, &ropts) < 0)\n> \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\n\nIn this case a smaller change of:\n\n if (!ropts.branch)\n\t\tropts.flags |= RESET_HEAD_DETACH;\n\nwill do the same.\n\nI wonder if just converting it to a designated initializer while we're\nat it (or a pre-cleanup commit) would be better, i.e.:\n\n\t\n\tdiff --git a/builtin/rebase.c b/builtin/rebase.c\n\tindex 5ae7fa2a169..bf4fd4d2920 100644\n\t--- a/builtin/rebase.c\n\t+++ b/builtin/rebase.c\n\t@@ -820,18 +820,18 @@ static int rebase_config(const char *var, const char *value, void *data)\n\t static int checkout_up_to_date(struct rebase_options *options)\n\t {\n\t \tstruct strbuf buf = STRBUF_INIT;\n\t-\tstruct reset_head_opts ropts = { 0 };\n\t+\tstruct reset_head_opts ropts = {\n\t+\t\t.oid = &options->orig_head,\n\t+\t\t.branch = options->head_name,\n\t+\t\t.flags = (RESET_HEAD_RUN_POST_CHECKOUT_HOOK |\n\t+\t\t\t (options->head_name ? 0 : RESET_HEAD_DETACH)),\n\t+\t};\n\t \tint ret = 0;\n\t \n\t \tstrbuf_addf(&buf, \"%s: checkout %s\",\n\t \t\t getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n\t \t\t options->switch_to);\n\t-\tropts.oid = &options->orig_head;\n\t-\tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n\t-\tif (options->head_name)\n\t-\t\tropts.branch = options->head_name;\n\t-\telse\n\t-\t\tropts.flags |= RESET_HEAD_DETACH;\n\t+\n\t \tropts.head_msg = buf.buf;\n\t \tif (reset_head(the_repository, &ropts) < 0)\n\t \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\n\nBut in any case the functionality will be the same, so this is just\nbikeshedding.\n\t\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 0643d015255..d5a8ee39fc4 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -394,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '\n> \tgit rebase main other\n> '\n> \n> +test_expect_success 'switch to non-branch detaches HEAD' '\n> +\tgit checkout main &&\n> +\told_main=$(git rev-parse HEAD) &&\n> +\tgit rebase First Second^0 &&\n> +\ttest_cmp_rev HEAD Second &&\n> +\ttest_cmp_rev main $old_main &&\n> +\ttest_must_fail git symbolic-ref HEAD\n> +'\n> +\n> test_expect_success 'refuse to switch to branch checked out elsewhere' '\n> \tgit checkout main &&\n> \tgit worktree add wt &&\n\nI think it's *much* easier to review these sorts of changes where\nthere's a preceding commit that positively asserts what we do now, and\nwe'll then in the \"fix\" commit change the behavior.\n\nSo more \"show\" v.s. \"tell\".\n\nI.e. in this case do the \"test_cmp_rev\" to the \"wrong\" tip with a TODO\ncomment or whatever, then in the fix just adjust it, then it's clear\nwhat we had before/after.\n"},{"id":"451563","messageId":"xmqqy218h9t4.fsf@gitster.g","threadId":"57536","inReplyTo":"220317.86r170d6zs.gmgdl@evledraar.gmail.com","subject":"Re: [PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-17T15:34:47Z","receivedAt":"2022-03-17T15:34:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n>> @@ -827,8 +827,11 @@ static int checkout_up_to_date(struct rebase_options *options)\n>> \t\t getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n>> \t\t options->switch_to);\n>> \tropts.oid = &options->orig_head;\n>> -\tropts.branch = options->head_name;\n>> \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n>> +\tif (options->head_name)\n>> +\t\tropts.branch = options->head_name;\n>> +\telse\n>> +\t\tropts.flags |= RESET_HEAD_DETACH;\n>> \tropts.head_msg = buf.buf;\n>> \tif (reset_head(the_repository, &ropts) < 0)\n>> \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\n>\n> In this case a smaller change of:\n>\n> if (!ropts.branch)\n> \t\tropts.flags |= RESET_HEAD_DETACH;\n>\n> will do the same.\n\nThanks. That is much easier to read and simpler to follow.\n\n> I wonder if just converting it to a designated initializer while we're\n> at it (or a pre-cleanup commit) would be better, i.e.:\n\nI do not think it easier to follow than even the original or the\nimprovement above, especially the part that computes .flags member.\n"},{"id":"451589","messageId":"cac51a949eed0fa593247a593aae2b100be6f4f2.1647546828.git.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":"pull.1226.v4.git.git.1647546828.gitgitgadget@gmail.com","subject":"[PATCH v4 1/3] rebase: test showing bug in rebase with non-branch","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-17T19:53:46Z","receivedAt":"2022-03-17T19:53:59Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nCurrently when rebase is used with a non branch, and is up to\ndate with base:\n\ngit rebase base \n\nIt will update the ref that HEAD is pointing at to , and leave HEAD\nunmodified.\n\nThis is a bug. The expected behavior is that the branch HEAD points at\nremains unmodified while HEAD is updated to point to in detached\nHEAD mode.\n\nSigned-off-by: John Cai \nReported-by: Michael McClimon \n---\n t/t3400-rebase.sh | 9 +++++++++\n 1 file changed, 9 insertions(+)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 71b1735e1dd..5c4073f06d6 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -399,6 +399,15 @@ test_expect_success 'switch to branch not checked out' '\n \tgit rebase main other\n '\n \n+test_expect_success 'switch to non-branch changes branch HEAD points to' '\n+\tgit checkout main &&\n+\told_main=$(git rev-parse HEAD) &&\n+\tgit rebase First Second^0 &&\n+\ttest_cmp_rev HEAD main &&\n+\ttest_cmp_rev main $(git rev-parse Second) &&\n+\tgit symbolic-ref HEAD\n+'\n+\n test_expect_success 'refuse to switch to branch checked out elsewhere' '\n \tgit checkout main &&\n \tgit worktree add wt &&\n-- \ngitgitgadget\n\n"},{"id":"451590","messageId":"pull.1226.v4.git.git.1647546828.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":"pull.1226.v3.git.git.1647487001.gitgitgadget@gmail.com","subject":"[PATCH v4 0/3] rebase: update HEAD when is an oid","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-17T19:53:45Z","receivedAt":"2022-03-17T19:54:00Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Fixes a bug [1] reported by Michael McClimon where rebase with oids will\nerroneously update the branch HEAD points to.\n\n 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n\nThis patch has three parts:\n\n * a test to show the incorrect buggy behavior of rebase with a non-branch\n argument\n * updates rebase setup test to add some tags we will use, while swapping\n out manual commit creation with the test_commit helper\n * sets RESET_HARD_DETACH flag if branch is not a valid refname\n\nchanges since v3:\n\n * fixed typos in commit message\n * added a test assertion to show bug behavior\n * included more replacements with test_commit\n\nchanges since v2:\n\n * remove BUG assertion\n\nchanges since v1:\n\n * only set RESET_HEAD_DETACH if is not a valid branch\n * updated tests to use existing setup\n\nJohn Cai (3):\n rebase: test showing bug in rebase with non-branch\n rebase: use test_commit helper in setup\n rebase: set REF_HEAD_DETACH in checkout_up_to_date()\n\n builtin/rebase.c | 2 ++\n t/t3400-rebase.sh | 27 +++++++++++++--------------\n 2 files changed, 15 insertions(+), 14 deletions(-)\n\n\nbase-commit: b896f729e240d250cf56899e6a0073f6aa469f5d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v4\nPull-Request: https://github.com/git/git/pull/1226\n\nRange-diff vs v3:\n\n -: ----------- > 1: cac51a949ee rebase: test showing bug in rebase with non-branch\n 1: f658eb00bcd ! 2: 5c40e116eba rebase: use test_commit helper in setup\n @@ t/t3400-rebase.sh: test_expect_success 'prepare repository with topic branches'\n \tgit checkout -f main &&\n \techo Third >>A &&\n \tgit update-index A &&\n + \tgit commit -m \"Modify A.\" &&\n + \tgit checkout -b side my-topic-branch &&\n +-\techo Side >>C &&\n +-\tgit add C &&\n +-\tgit commit -m \"Add C\" &&\n ++\ttest_commit --no-tag \"Add C\" C Side &&\n + \tgit checkout -f my-topic-branch &&\n + \tgit tag topic\n + '\n +@@ t/t3400-rebase.sh: test_expect_success 'rebase off of the previous branch using \"-\"' '\n + test_expect_success 'rebase a single mode change' '\n + \tgit checkout main &&\n + \tgit branch -D topic &&\n +-\techo 1 >X &&\n +-\tgit add X &&\n +-\ttest_tick &&\n +-\tgit commit -m prepare &&\n ++\ttest_commit prepare X 1 &&\n + \tgit checkout -b modechange HEAD^ &&\n + \techo 1 >X &&\n + \tgit add X &&\n 2: bd1c9537ffc ! 3: 13c5955c317 rebase: set REF_HEAD_DETACH in checkout_up_to_date()\n @@ Commit message\n cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs()\n -> update_ref()\n \n - When is not a valid branch but a sha, rebase sets the head_name\n + When is not a valid branch but an oid, rebase sets the head_name\n of rebase_options to NULL. This value gets passed down this call chain\n through the branch member of reset_head_opts also getting set to NULL\n - all the way to update_refs(). update_refs() checks ropts.branch to\n - decide whether or not to switch brancheds. If ropts.branch is NULL, it\n - calls update_ref() to update HEAD. At this point however, from rebase's\n - point of view, we want a detached HEAD. But, since checkout_up_to_date()\n - does not set the RESET_HEAD_DETACH flag, the update_ref() call will\n - deference HEAD and update the branch its pointing to, which in the above\n - example is main.\n + all the way to update_refs().\n +\n + Then update_refs() checks ropts.branch to decide whether or not to switch\n + branches. If ropts.branch is NULL, it calls update_ref() to update HEAD.\n + At this point however, from rebase's point of view, we want a detached\n + HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH\n + flag, the update_ref() call will deference HEAD and update the branch its\n + pointing to, which in the above example is main.\n \n The correct behavior is that git rebase should update HEAD to $(git\n rev-parse topic) without dereferencing it.\n @@ Commit message\n HEAD directly intead of deferencing it and updating the branch that HEAD\n points to.\n \n - Also add a test to ensure this behavior.\n + Also add a test to ensure the correct behavior.\n \n 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n \n - Reported-by: Michael McClimon \n Signed-off-by: John Cai \n \n ## builtin/rebase.c ##\n @@ builtin/rebase.c: static int checkout_up_to_date(struct rebase_options *options)\n - \t\t getenv(GIT_REFLOG_ACTION_ENVIRONMENT),\n - \t\t options->switch_to);\n \tropts.oid = &options->orig_head;\n --\tropts.branch = options->head_name;\n + \tropts.branch = options->head_name;\n \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n -+\tif (options->head_name)\n -+\t\tropts.branch = options->head_name;\n -+\telse\n ++\tif (!ropts.branch)\n +\t\tropts.flags |= RESET_HEAD_DETACH;\n \tropts.head_msg = buf.buf;\n \tif (reset_head(the_repository, &ropts) < 0)\n @@ t/t3400-rebase.sh: test_expect_success 'switch to branch not checked out' '\n \tgit rebase main other\n '\n \n +-test_expect_success 'switch to non-branch changes branch HEAD points to' '\n +test_expect_success 'switch to non-branch detaches HEAD' '\n -+\tgit checkout main &&\n -+\told_main=$(git rev-parse HEAD) &&\n -+\tgit rebase First Second^0 &&\n + \tgit checkout main &&\n + \told_main=$(git rev-parse HEAD) &&\n + \tgit rebase First Second^0 &&\n +-\ttest_cmp_rev HEAD main &&\n +-\ttest_cmp_rev main $(git rev-parse Second) &&\n +-\tgit symbolic-ref HEAD\n +\ttest_cmp_rev HEAD Second &&\n +\ttest_cmp_rev main $old_main &&\n +\ttest_must_fail git symbolic-ref HEAD\n -+'\n -+\n + '\n + \n test_expect_success 'refuse to switch to branch checked out elsewhere' '\n - \tgit checkout main &&\n - \tgit worktree add wt &&\n\n-- \ngitgitgadget\n"},{"id":"451591","messageId":"5c40e116eba00b5b1a64191c6adf211d326e7f96.1647546828.git.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":"pull.1226.v4.git.git.1647546828.gitgitgadget@gmail.com","subject":"[PATCH v4 2/3] rebase: use test_commit helper in setup","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-17T19:53:47Z","receivedAt":"2022-03-17T19:54:02Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nTo prepare for the next commit that will test rebase with oids instead\nof branch names, update the rebase setup test to add a couple of tags we\ncan use. This uses the test_commit helper so we can replace some lines\nthat add a commit manually.\n\nSetting logAllRefUpdates is not necessary because it's on by default for\nrepositories with a working tree.\n\nHelped-by: Phillip Wood \nSigned-off-by: John Cai \n---\n t/t3400-rebase.sh | 18 ++++--------------\n 1 file changed, 4 insertions(+), 14 deletions(-)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 5c4073f06d6..2fb3fabe60e 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address\n export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL\n \n test_expect_success 'prepare repository with topic branches' '\n-\tgit config core.logAllRefUpdates true &&\n-\techo First >A &&\n-\tgit update-index --add A &&\n-\tgit commit -m \"Add A.\" &&\n+\ttest_commit \"Add A.\" A First First &&\n \tgit checkout -b force-3way &&\n \techo Dummy >Y &&\n \tgit update-index --add Y &&\n@@ -32,17 +29,13 @@ test_expect_success 'prepare repository with topic branches' '\n \tgit mv A D/A &&\n \tgit commit -m \"Move A.\" &&\n \tgit checkout -b my-topic-branch main &&\n-\techo Second >B &&\n-\tgit update-index --add B &&\n-\tgit commit -m \"Add B.\" &&\n+\ttest_commit \"Add B.\" B Second Second &&\n \tgit checkout -f main &&\n \techo Third >>A &&\n \tgit update-index A &&\n \tgit commit -m \"Modify A.\" &&\n \tgit checkout -b side my-topic-branch &&\n-\techo Side >>C &&\n-\tgit add C &&\n-\tgit commit -m \"Add C\" &&\n+\ttest_commit --no-tag \"Add C\" C Side &&\n \tgit checkout -f my-topic-branch &&\n \tgit tag topic\n '\n@@ -119,10 +112,7 @@ test_expect_success 'rebase off of the previous branch using \"-\"' '\n test_expect_success 'rebase a single mode change' '\n \tgit checkout main &&\n \tgit branch -D topic &&\n-\techo 1 >X &&\n-\tgit add X &&\n-\ttest_tick &&\n-\tgit commit -m prepare &&\n+\ttest_commit prepare X 1 &&\n \tgit checkout -b modechange HEAD^ &&\n \techo 1 >X &&\n \tgit add X &&\n-- \ngitgitgadget\n\n"},{"id":"451592","messageId":"13c5955c317713bbc6a91b9f44081395880abb67.1647546828.git.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":"pull.1226.v4.git.git.1647546828.gitgitgadget@gmail.com","subject":"[PATCH v4 3/3] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-17T19:53:48Z","receivedAt":"2022-03-17T19:54:02Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nFixes a bug whereby rebase updates the deferenced reference HEAD points\nto instead of HEAD directly.\n\nIf HEAD is on main and if the following is a fast-forward operation,\n\ngit rebase $(git rev-parse main) $(git rev-parse topic)\n\nInstead of HEAD being set to $(git rev-parse topic), rebase erroneously\ndereferences HEAD and sets main to $(git rev-parse topic). See [1] for\nthe original bug report.\n\nThe callstack from checkout_up_to_date() is the following:\n\ncmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs()\n -> update_ref()\n\nWhen is not a valid branch but an oid, rebase sets the head_name\nof rebase_options to NULL. This value gets passed down this call chain\nthrough the branch member of reset_head_opts also getting set to NULL\nall the way to update_refs().\n\nThen update_refs() checks ropts.branch to decide whether or not to switch\nbranches. If ropts.branch is NULL, it calls update_ref() to update HEAD.\nAt this point however, from rebase's point of view, we want a detached\nHEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH\nflag, the update_ref() call will deference HEAD and update the branch its\npointing to, which in the above example is main.\n\nThe correct behavior is that git rebase should update HEAD to $(git\nrev-parse topic) without dereferencing it.\n\nFix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date\nif is not a valid branch. so that once reset_head() calls\nupdate_refs(), it calls update_ref() with REF_NO_DEREF which updates\nHEAD directly intead of deferencing it and updating the branch that HEAD\npoints to.\n\nAlso add a test to ensure the correct behavior.\n\n1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n\nSigned-off-by: John Cai \n---\n builtin/rebase.c | 2 ++\n t/t3400-rebase.sh | 8 ++++----\n 2 files changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b29ad2b65e7..27fde7bf281 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options)\n \tropts.oid = &options->orig_head;\n \tropts.branch = options->head_name;\n \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n+\tif (!ropts.branch)\n+\t\tropts.flags |= RESET_HEAD_DETACH;\n \tropts.head_msg = buf.buf;\n \tif (reset_head(the_repository, &ropts) < 0)\n \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 2fb3fabe60e..cf55b017ffc 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -389,13 +389,13 @@ test_expect_success 'switch to branch not checked out' '\n \tgit rebase main other\n '\n \n-test_expect_success 'switch to non-branch changes branch HEAD points to' '\n+test_expect_success 'switch to non-branch detaches HEAD' '\n \tgit checkout main &&\n \told_main=$(git rev-parse HEAD) &&\n \tgit rebase First Second^0 &&\n-\ttest_cmp_rev HEAD main &&\n-\ttest_cmp_rev main $(git rev-parse Second) &&\n-\tgit symbolic-ref HEAD\n+\ttest_cmp_rev HEAD Second &&\n+\ttest_cmp_rev main $old_main &&\n+\ttest_must_fail git symbolic-ref HEAD\n '\n \n test_expect_success 'refuse to switch to branch checked out elsewhere' '\n-- \ngitgitgadget\n"},{"id":"451594","messageId":"xmqqo824e145.fsf@gitster.g","threadId":"57536","inReplyTo":"cac51a949eed0fa593247a593aae2b100be6f4f2.1647546828.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-17T21:10:50Z","receivedAt":"2022-03-17T21:10:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> Currently when rebase is used with a non branch, and is up to\n> date with base:\n>\n> git rebase base \n>\n> It will update the ref that HEAD is pointing at to , and leave HEAD\n> unmodified.\n>\n> This is a bug. The expected behavior is that the branch HEAD points at\n> remains unmodified while HEAD is updated to point to in detached\n> HEAD mode.\n\nNever do tests this way.\n\nThe primary reason why we do not want to write our tests the way\nthis patch does is because we do not _care_ how it is broken in the\nbehaviour of the original code. 'main' moving out of $old_main is\nthe bug we care about. It is still buggy if it did not move to\nSecond, but some other commit. Yet this patch insists that 'main'\nto move to Second and nothing else. What we want is 'main' to stay\nat $old_main in the end anyway, and we should directly test that\ncondition.\n\nIf you insist to have two commits (which I strongly recommend\nagainst in this case), you write a test that makes sure that 'main'\nstays at $old_main, but mark the test with test_expect_failure. And\nthen later in the step that fixes the code, flip \"expect_failure\" to\n\"expect_success\".\n\nBut it is not ideal, either. Imagine what you see in \"git show\"\noutput of the commit that fixed the problem. Most of the test that\nshows the behaviour that the commit _cares_ about will be outside\npost-context of the hunk that flips test_expect_failure to\ntest_expect_success.\n\nThe best and the simplest way, for a simple case like this, to write\ntest is to add the test to expect what we want to see in the end,\nand do so in the same commit as the one that corrects the behaviour\nof the code. If somebody wants to see what the breakage looks like,\nit is easy to\n\n (1) checkout the commit that fixes the code and adds such a test,\n\n (2) tentatively revert everything outside t/, and\n\n (3) run the test with \"-i -v\" options.\n\nThen test_expect_success that wants to see 'main' to stay at\n$old_main will show that 'main' moved by a test failure. Working\nfrom a patch is the same way, i.e. you can apply only the parts\ninside t/ and run the current code to see the breakage, and then\napply the rest to see the fix.\n\n> +test_expect_success 'switch to non-branch changes branch HEAD points to' '\n> +\tgit checkout main &&\n> +\told_main=$(git rev-parse HEAD) &&\n> +\tgit rebase First Second^0 &&\n\n> +\ttest_cmp_rev HEAD main &&\n> +\ttest_cmp_rev main $(git rev-parse Second) &&\n> +\tgit symbolic-ref HEAD\n\nI already said that the second one should expect main to be at\n$old_main, but the \"HEAD and main are the same\" and \"HEAD is a\nsymolic-ref\" test can be replaced with a single test that is \"HEAD\nis a symbolic-ref to 'main'\", which would be more strict. I.e.\n\n\ttest \"$(git symbolic-ref HEAD)\" = refs/heads/main &&\n\ttest_cmp_rev main \"$old_main\"\n\nAnd such a test that expects the correct behaviour we want to have\nin the end should be added in [PATCH 3/3] when the code is fixed,\nnot here in a separate commit.\n\n> +'\n\nThanks.\n"},{"id":"451595","messageId":"xmqq8rt8dzxr.fsf@gitster.g","threadId":"57536","inReplyTo":"13c5955c317713bbc6a91b9f44081395880abb67.1647546828.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 3/3] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-17T21:36:16Z","receivedAt":"2022-03-17T21:36:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> Fixes a bug whereby rebase updates the deferenced reference HEAD points\n> to instead of HEAD directly.\n\nPerhaps\n\n \"git rebase A B\", where B is not a commit, should behave as if\n the HEAD got detached at B and then the detached HEAD got\n rebased on top of A. A bug however overwrites the current\n branch to point at B, when B is a descendant of A (i.e. the\n rebase ends up being a fast-forward).\n\n> ... See [1] for\n> the original bug report.\n\nOK (URL is wrong; see below).\n\nThe explanation of how the bug occurs (elided) in the patch looked\nall reasonable. It read well.\n\n> ...\n> Also add a test to ensure the correct behavior.\n\nYup. _Add_ a test to ensure that. Not replace a misleading test\nthat expected to see a wrong behaviour.\n\n> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n\nThis is not the original bug report. It was an early hint for\ndiagnosis.\n\n[1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/\n\nwould be a more appropriate pointer.\n\n> \tropts.oid = &options->orig_head;\n> \tropts.branch = options->head_name;\n> \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n> +\tif (!ropts.branch)\n> +\t\tropts.flags |= RESET_HEAD_DETACH;\n> \tropts.head_msg = buf.buf;\n\nOK. If head_name is not set, we do not want to touch the branch\nthe HEAD happens to be pointing at, so we want to detach.\n\n> +test_expect_success 'switch to non-branch detaches HEAD' '\n> \tgit checkout main &&\n> \told_main=$(git rev-parse HEAD) &&\n> \tgit rebase First Second^0 &&\n> -\ttest_cmp_rev HEAD main &&\n> -\ttest_cmp_rev main $(git rev-parse Second) &&\n> -\tgit symbolic-ref HEAD\n> +\ttest_cmp_rev HEAD Second &&\n> +\ttest_cmp_rev main $old_main &&\n> +\ttest_must_fail git symbolic-ref HEAD\n\nAs we want (1) HEAD (detached) is pointing at Second, (2) 'main'\nstayed at $old_main, and (3) HEAD is detched, these three conditions\nlook sane.\n\nThanks.\n\nFor reference, I discarded [1/3], queued [2/3] and replaced this\n[3/3] with the following for now.\n\n---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ----\nFrom: John Cai \nSubject: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()\n\n\"git rebase A B\" where B is not a commit should behave as if the\nHEAD got detached at B and then the detached HEAD got rebased on top\nof A. A bug however overwrites the current branch to point at B,\nwhen B is a descendant of A (i.e. the rebase ends up being a\nfast-forward). See [1] for the original bug report.\n\nThe callstack from checkout_up_to_date() is the following:\n\ncmd_rebase()\n-> checkout_up_to_date()\n -> reset_head()\n -> update_refs()\n -> update_ref()\n\nWhen B is not a valid branch but an oid, rebase sets the head_name\nof rebase_options to NULL. This value gets passed down this call\nchain through the branch member of reset_head_opts also getting set\nto NULL all the way to update_refs().\n\nThen update_refs() checks ropts.branch to decide whether or not to switch\nbranches. If ropts.branch is NULL, it calls update_ref() to update HEAD.\nAt this point however, from rebase's point of view, we want a detached\nHEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH\nflag, the update_ref() call will deference HEAD and update the branch its\npointing to. We want the HEAD detached at B instead.\n\nFix this bug by adding the RESET_HEAD_DETACH flag in\ncheckout_up_to_date if B is not a valid branch, so that once\nreset_head() calls update_refs(), it calls update_ref() with\nREF_NO_DEREF which updates HEAD directly intead of deferencing it\nand updating the branch that HEAD points to.\n\nAlso add a test to ensure the correct behavior.\n\n[1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/\n\nReported-by: Michael McClimon \nSigned-off-by: John Cai \nSigned-off-by: Junio C Hamano \n---\n builtin/rebase.c | 2 ++\n t/t3400-rebase.sh | 9 +++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b29ad2b65e..27fde7bf28 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options)\n \tropts.oid = &options->orig_head;\n \tropts.branch = options->head_name;\n \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n+\tif (!ropts.branch)\n+\t\tropts.flags |= RESET_HEAD_DETACH;\n \tropts.head_msg = buf.buf;\n \tif (reset_head(the_repository, &ropts) < 0)\n \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 6dc8df8be7..cf55b017ff 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -389,6 +389,15 @@ test_expect_success 'switch to branch not checked out' '\n \tgit rebase main other\n '\n \n+test_expect_success 'switch to non-branch detaches HEAD' '\n+\tgit checkout main &&\n+\told_main=$(git rev-parse HEAD) &&\n+\tgit rebase First Second^0 &&\n+\ttest_cmp_rev HEAD Second &&\n+\ttest_cmp_rev main $old_main &&\n+\ttest_must_fail git symbolic-ref HEAD\n+'\n+\n test_expect_success 'refuse to switch to branch checked out elsewhere' '\n \tgit checkout main &&\n \tgit worktree add wt &&\n-- \n2.35.1-757-g4266a5c05c\n\n\n"},{"id":"451596","messageId":"xmqq4k3wdzvt.fsf@gitster.g","threadId":"57536","inReplyTo":"xmqqo824e145.fsf@gitster.g","subject":"Re: [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-17T21:37:26Z","receivedAt":"2022-03-17T21:37:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> +\ttest_cmp_rev HEAD main &&\n>> +\ttest_cmp_rev main $(git rev-parse Second) &&\n>> +\tgit symbolic-ref HEAD\n>\n> I already said that the second one should expect main to be at\n> $old_main, but the \"HEAD and main are the same\" and \"HEAD is a\n> symolic-ref\" test can be replaced with a single test that is \"HEAD\n> is a symbolic-ref to 'main'\", which would be more strict. I.e.\n>\n> \ttest \"$(git symbolic-ref HEAD)\" = refs/heads/main &&\n> \ttest_cmp_rev main \"$old_main\"\n\nScratch that. No, we do not want HEAD to be pointing at 'main'\nafter rebase, so the above is totally wrong. What you have at the\nend of the series is the right version, as I said in my review of\nthe [PATCH 3/3].\n\nThanks for working on this topic.\n"},{"id":"451601","messageId":"1BEA9176-EB83-4E7C-A592-037667D0D9ED@gmail.com","threadId":"57536","inReplyTo":"xmqqo824e145.fsf@gitster.g","subject":"Re: [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-03-17T22:44:44Z","receivedAt":"2022-03-17T22:44:48Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 17 Mar 2022, at 17:10, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" writes:\n>\n>> From: John Cai \n>>\n>> Currently when rebase is used with a non branch, and is up to\n>> date with base:\n>>\n>> git rebase base \n>>\n>> It will update the ref that HEAD is pointing at to , and leave HEAD\n>> unmodified.\n>>\n>> This is a bug. The expected behavior is that the branch HEAD points at\n>> remains unmodified while HEAD is updated to point to in detached\n>> HEAD mode.\n>\n> Never do tests this way.\n>\n> The primary reason why we do not want to write our tests the way\n> this patch does is because we do not _care_ how it is broken in the\n> behaviour of the original code. 'main' moving out of $old_main is\n> the bug we care about. It is still buggy if it did not move to\n> Second, but some other commit. Yet this patch insists that 'main'\n> to move to Second and nothing else. What we want is 'main' to stay\n> at $old_main in the end anyway, and we should directly test that\n> condition.\n\nI was attemping to follow the advice to \"show\" vs \"tell\" in [1]. All this\nmake sense to me however.\n\n>\n> If you insist to have two commits (which I strongly recommend\n> against in this case), you write a test that makes sure that 'main'\n> stays at $old_main, but mark the test with test_expect_failure. And\n> then later in the step that fixes the code, flip \"expect_failure\" to\n> \"expect_success\".\n>\n> But it is not ideal, either. Imagine what you see in \"git show\"\n> output of the commit that fixed the problem. Most of the test that\n> shows the behaviour that the commit _cares_ about will be outside\n> post-context of the hunk that flips test_expect_failure to\n> test_expect_success.\n>\n> The best and the simplest way, for a simple case like this, to write\n> test is to add the test to expect what we want to see in the end,\n> and do so in the same commit as the one that corrects the behaviour\n> of the code. If somebody wants to see what the breakage looks like,\n> it is easy to\n>\n> (1) checkout the commit that fixes the code and adds such a test,\n>\n> (2) tentatively revert everything outside t/, and\n>\n> (3) run the test with \"-i -v\" options.\n>\n> Then test_expect_success that wants to see 'main' to stay at\n> $old_main will show that 'main' moved by a test failure. Working\n> from a patch is the same way, i.e. you can apply only the parts\n> inside t/ and run the current code to see the breakage, and then\n> apply the rest to see the fix.\n>\n>> +test_expect_success 'switch to non-branch changes branch HEAD points to' '\n>> +\tgit checkout main &&\n>> +\told_main=$(git rev-parse HEAD) &&\n>> +\tgit rebase First Second^0 &&\n>\n>> +\ttest_cmp_rev HEAD main &&\n>> +\ttest_cmp_rev main $(git rev-parse Second) &&\n>> +\tgit symbolic-ref HEAD\n>\n> I already said that the second one should expect main to be at\n> $old_main, but the \"HEAD and main are the same\" and \"HEAD is a\n> symolic-ref\" test can be replaced with a single test that is \"HEAD\n> is a symbolic-ref to 'main'\", which would be more strict. I.e.\n>\n> \ttest \"$(git symbolic-ref HEAD)\" = refs/heads/main &&\n> \ttest_cmp_rev main \"$old_main\"\n>\n> And such a test that expects the correct behaviour we want to have\n> in the end should be added in [PATCH 3/3] when the code is fixed,\n> not here in a separate commit.\n\n1. https://lore.kernel.org/git/220317.86r170d6zs.gmgdl@evledraar.gmail.com/\n\n>\n>> +'\n>\n> Thanks.\n"},{"id":"451602","messageId":"C271A4C3-2299-4D9D-BAA8-28968BFBE6C6@gmail.com","threadId":"57536","inReplyTo":"xmqq8rt8dzxr.fsf@gitster.g","subject":"Re: [PATCH v4 3/3] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-03-18T00:30:16Z","receivedAt":"2022-03-18T00:30:21Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Junio,\n\nOn 17 Mar 2022, at 17:36, Junio C Hamano wrote:\n\n> \"John Cai via GitGitGadget\" writes:\n>\n>> From: John Cai \n>>\n>> Fixes a bug whereby rebase updates the deferenced reference HEAD points\n>> to instead of HEAD directly.\n>\n> Perhaps\n>\n> \"git rebase A B\", where B is not a commit, should behave as if\n> the HEAD got detached at B and then the detached HEAD got\n> rebased on top of A. A bug however overwrites the current\n> branch to point at B, when B is a descendant of A (i.e. the\n> rebase ends up being a fast-forward).\n>\n>> ... See [1] for\n>> the original bug report.\n>\n> OK (URL is wrong; see below).\n>\n> The explanation of how the bug occurs (elided) in the patch looked\n> all reasonable. It read well.\n>\n>> ...\n>> Also add a test to ensure the correct behavior.\n>\n> Yup. _Add_ a test to ensure that. Not replace a misleading test\n> that expected to see a wrong behaviour.\n>\n>> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n>\n> This is not the original bug report. It was an early hint for\n> diagnosis.\n>\n> [1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/\n>\n> would be a more appropriate pointer.\n>\n>> \tropts.oid = &options->orig_head;\n>> \tropts.branch = options->head_name;\n>> \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n>> +\tif (!ropts.branch)\n>> +\t\tropts.flags |= RESET_HEAD_DETACH;\n>> \tropts.head_msg = buf.buf;\n>\n> OK. If head_name is not set, we do not want to touch the branch\n> the HEAD happens to be pointing at, so we want to detach.\n>\n>> +test_expect_success 'switch to non-branch detaches HEAD' '\n>> \tgit checkout main &&\n>> \told_main=$(git rev-parse HEAD) &&\n>> \tgit rebase First Second^0 &&\n>> -\ttest_cmp_rev HEAD main &&\n>> -\ttest_cmp_rev main $(git rev-parse Second) &&\n>> -\tgit symbolic-ref HEAD\n>> +\ttest_cmp_rev HEAD Second &&\n>> +\ttest_cmp_rev main $old_main &&\n>> +\ttest_must_fail git symbolic-ref HEAD\n>\n> As we want (1) HEAD (detached) is pointing at Second, (2) 'main'\n> stayed at $old_main, and (3) HEAD is detched, these three conditions\n> look sane.\n>\n> Thanks.\n>\n> For reference, I discarded [1/3], queued [2/3] and replaced this\n> [3/3] with the following for now.\n\nSounds good--this is what I was about to do anyways!\n\n>\n> ---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ----\n> From: John Cai \n> Subject: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()\n>\n> \"git rebase A B\" where B is not a commit should behave as if the\n> HEAD got detached at B and then the detached HEAD got rebased on top\n> of A. A bug however overwrites the current branch to point at B,\n> when B is a descendant of A (i.e. the rebase ends up being a\n> fast-forward). See [1] for the original bug report.\n>\n> The callstack from checkout_up_to_date() is the following:\n>\n> cmd_rebase()\n> -> checkout_up_to_date()\n> -> reset_head()\n> -> update_refs()\n> -> update_ref()\n>\n> When B is not a valid branch but an oid, rebase sets the head_name\n> of rebase_options to NULL. This value gets passed down this call\n> chain through the branch member of reset_head_opts also getting set\n> to NULL all the way to update_refs().\n>\n> Then update_refs() checks ropts.branch to decide whether or not to switch\n> branches. If ropts.branch is NULL, it calls update_ref() to update HEAD.\n> At this point however, from rebase's point of view, we want a detached\n> HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH\n> flag, the update_ref() call will deference HEAD and update the branch its\n> pointing to. We want the HEAD detached at B instead.\n>\n> Fix this bug by adding the RESET_HEAD_DETACH flag in\n> checkout_up_to_date if B is not a valid branch, so that once\n> reset_head() calls update_refs(), it calls update_ref() with\n> REF_NO_DEREF which updates HEAD directly intead of deferencing it\n> and updating the branch that HEAD points to.\n>\n> Also add a test to ensure the correct behavior.\n>\n> [1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/\n>\n> Reported-by: Michael McClimon \n> Signed-off-by: John Cai \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/rebase.c | 2 ++\n> t/t3400-rebase.sh | 9 +++++++++\n> 2 files changed, 11 insertions(+)\n>\n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index b29ad2b65e..27fde7bf28 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options)\n> \tropts.oid = &options->orig_head;\n> \tropts.branch = options->head_name;\n> \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n> +\tif (!ropts.branch)\n> +\t\tropts.flags |= RESET_HEAD_DETACH;\n> \tropts.head_msg = buf.buf;\n> \tif (reset_head(the_repository, &ropts) < 0)\n> \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 6dc8df8be7..cf55b017ff 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -389,6 +389,15 @@ test_expect_success 'switch to branch not checked out' '\n> \tgit rebase main other\n> '\n>\n> +test_expect_success 'switch to non-branch detaches HEAD' '\n> +\tgit checkout main &&\n> +\told_main=$(git rev-parse HEAD) &&\n> +\tgit rebase First Second^0 &&\n> +\ttest_cmp_rev HEAD Second &&\n> +\ttest_cmp_rev main $old_main &&\n> +\ttest_must_fail git symbolic-ref HEAD\n> +'\n> +\n> test_expect_success 'refuse to switch to branch checked out elsewhere' '\n> \tgit checkout main &&\n> \tgit worktree add wt &&\n> --\n> 2.35.1-757-g4266a5c05c\n\nThanks\nJohn\n"},{"id":"451619","messageId":"7f76e3e3-20d5-d249-c376-962dcc73869f@gmail.com","threadId":"57536","inReplyTo":"5c40e116eba00b5b1a64191c6adf211d326e7f96.1647546828.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 2/3] rebase: use test_commit helper in setup","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2022-03-18T11:14:58Z","receivedAt":"2022-03-18T11:15:04Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi John\n\nOn 17/03/2022 19:53, John Cai via GitGitGadget wrote:\n> From: John Cai \n> \n> To prepare for the next commit that will test rebase with oids instead\n> of branch names, update the rebase setup test to add a couple of tags we\n> can use. This uses the test_commit helper so we can replace some lines\n> that add a commit manually.\n> \n> Setting logAllRefUpdates is not necessary because it's on by default for\n> repositories with a working tree.\n> \n> Helped-by: Phillip Wood \n> Signed-off-by: John Cai \n> ---\n> t/t3400-rebase.sh | 18 ++++--------------\n> 1 file changed, 4 insertions(+), 14 deletions(-)\n> \n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 5c4073f06d6..2fb3fabe60e 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address\n> export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL\n> \n> test_expect_success 'prepare repository with topic branches' '\n> -\tgit config core.logAllRefUpdates true &&\n> -\techo First >A &&\n> -\tgit update-index --add A &&\n> -\tgit commit -m \"Add A.\" &&\n> +\ttest_commit \"Add A.\" A First First &&\n> \tgit checkout -b force-3way &&\n> \techo Dummy >Y &&\n> \tgit update-index --add Y &&\n> @@ -32,17 +29,13 @@ test_expect_success 'prepare repository with topic branches' '\n> \tgit mv A D/A &&\n> \tgit commit -m \"Move A.\" &&\n> \tgit checkout -b my-topic-branch main &&\n> -\techo Second >B &&\n> -\tgit update-index --add B &&\n> -\tgit commit -m \"Add B.\" &&\n> +\ttest_commit \"Add B.\" B Second Second &&\n> \tgit checkout -f main &&\n> \techo Third >>A &&\n> \tgit update-index A &&\n> \tgit commit -m \"Modify A.\" &&\n> \tgit checkout -b side my-topic-branch &&\n\nThis version has added some more conversions that are not mentioned it \nthe commit message. If you want to covert the whole setup to use \ntest_commit then that's great but I think you need to do the whole thing \nand say in the commit message that you're modernizing everything. As it \nstands a reader is left wondering why some of the setup that is not used \nin the next commit has been converted but other bits such as the \n\"Modify A.\" above are left as is. I think the two possibilities that \nmake sense are (a) convert only what we need for the next commit, or (b) \nmodernize the test and convert everything.\n\nBest Wishes\n\nPhillip\n\n> -\techo Side >>C &&\n> -\tgit add C &&\n> -\tgit commit -m \"Add C\" &&\n> +\ttest_commit --no-tag \"Add C\" C Side &&\n> \tgit checkout -f my-topic-branch &&\n> \tgit tag topic\n> '\n> @@ -119,10 +112,7 @@ test_expect_success 'rebase off of the previous branch using \"-\"' '\n> test_expect_success 'rebase a single mode change' '\n> \tgit checkout main &&\n> \tgit branch -D topic &&\n> -\techo 1 >X &&\n> -\tgit add X &&\n> -\ttest_tick &&\n> -\tgit commit -m prepare &&\n> +\ttest_commit prepare X 1 &&\n> \tgit checkout -b modechange HEAD^ &&\n> \techo 1 >X &&\n> \tgit add X &&\n\n"},{"id":"451621","messageId":"11678816-1524-401C-9AFB-813866BB94F2@gmail.com","threadId":"57536","inReplyTo":"7f76e3e3-20d5-d249-c376-962dcc73869f@gmail.com","subject":"Re: [PATCH v4 2/3] rebase: use test_commit helper in setup","fromName":"John Cai","fromEmail":"johncai86@gmail.com","sentAt":"2022-03-18T13:06:23Z","receivedAt":"2022-03-18T13:06:42Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Hi Phillip,\n\nOn 18 Mar 2022, at 7:14, Phillip Wood wrote:\n\n> Hi John\n>\n> On 17/03/2022 19:53, John Cai via GitGitGadget wrote:\n>> From: John Cai \n>>\n>> To prepare for the next commit that will test rebase with oids instead\n>> of branch names, update the rebase setup test to add a couple of tags we\n>> can use. This uses the test_commit helper so we can replace some lines\n>> that add a commit manually.\n>>\n>> Setting logAllRefUpdates is not necessary because it's on by default for\n>> repositories with a working tree.\n>>\n>> Helped-by: Phillip Wood \n>> Signed-off-by: John Cai \n>> ---\n>> t/t3400-rebase.sh | 18 ++++--------------\n>> 1 file changed, 4 insertions(+), 14 deletions(-)\n>>\n>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n>> index 5c4073f06d6..2fb3fabe60e 100755\n>> --- a/t/t3400-rebase.sh\n>> +++ b/t/t3400-rebase.sh\n>> @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address\n>> export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL\n>> test_expect_success 'prepare repository with topic branches' '\n>> -\tgit config core.logAllRefUpdates true &&\n>> -\techo First >A &&\n>> -\tgit update-index --add A &&\n>> -\tgit commit -m \"Add A.\" &&\n>> +\ttest_commit \"Add A.\" A First First &&\n>> \tgit checkout -b force-3way &&\n>> \techo Dummy >Y &&\n>> \tgit update-index --add Y &&\n>> @@ -32,17 +29,13 @@ test_expect_success 'prepare repository with topic branches' '\n>> \tgit mv A D/A &&\n>> \tgit commit -m \"Move A.\" &&\n>> \tgit checkout -b my-topic-branch main &&\n>> -\techo Second >B &&\n>> -\tgit update-index --add B &&\n>> -\tgit commit -m \"Add B.\" &&\n>> +\ttest_commit \"Add B.\" B Second Second &&\n>> \tgit checkout -f main &&\n>> \techo Third >>A &&\n>> \tgit update-index A &&\n>> \tgit commit -m \"Modify A.\" &&\n>> \tgit checkout -b side my-topic-branch &&\n>\n> This version has added some more conversions that are not mentioned it the commit message. If you want to covert the whole setup to use test_commit then that's great but I think you need to do the whole thing and say in the commit message that you're modernizing everything. As it stands a reader is left wondering why some of the setup that is not used in the next commit has been converted but other bits such as the \"Modify A.\" above are left as is. I think the two possibilities that make sense are (a) convert only what we need for the next commit, or (b) modernize the test and convert everything.\n\nI see your point. Then for the sake of a smaller series for the patch, I'll opt\nto only update the parts we need. Then maybe we can have a separate series to\nmodernize this suite.\n\nI'll re-roll this series with the clarifications to the commit message that\nJunio made.\n\n>\n> Best Wishes\n>\n> Phillip\n\nThanks!\nJohn\n\n>\n>> -\techo Side >>C &&\n>> -\tgit add C &&\n>> -\tgit commit -m \"Add C\" &&\n>> +\ttest_commit --no-tag \"Add C\" C Side &&\n>> \tgit checkout -f my-topic-branch &&\n>> \tgit tag topic\n>> '\n>> @@ -119,10 +112,7 @@ test_expect_success 'rebase off of the previous branch using \"-\"' '\n>> test_expect_success 'rebase a single mode change' '\n>> \tgit checkout main &&\n>> \tgit branch -D topic &&\n>> -\techo 1 >X &&\n>> -\tgit add X &&\n>> -\ttest_tick &&\n>> -\tgit commit -m prepare &&\n>> +\ttest_commit prepare X 1 &&\n>> \tgit checkout -b modechange HEAD^ &&\n>> \techo 1 >X &&\n>> \tgit add X &&\n"},{"id":"451623","messageId":"pull.1226.v5.git.git.1647611643.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":"pull.1226.v4.git.git.1647546828.gitgitgadget@gmail.com","subject":"[PATCH v5 0/2] rebase: update HEAD when is an oid","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-18T13:54:01Z","receivedAt":"2022-03-18T13:54:10Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"Fixes a bug [1] reported by Michael McClimon where rebase with oids will\nerroneously update the branch HEAD points to.\n\n 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n\nThis patch has two parts:\n\n * updates rebase setup test to add some tags we will use, while swapping\n out manual commit creation with the test_commit helper\n * sets RESET_HARD_DETACH flag if branch is not a valid refname\n\nchanges since v4:\n\n * got rid of test assertion that shows bug behavior\n * clarified commit message\n\nchanges since v3:\n\n * fixed typos in commit message\n * added a test assertion to show bug behavior\n * included more replacements with test_commit\n\nchanges since v2:\n\n * remove BUG assertion\n\nchanges since v1:\n\n * only set RESET_HEAD_DETACH if is not a valid branch\n * updated tests to use existing setup\n\nJohn Cai (2):\n rebase: use test_commit helper in setup\n rebase: set REF_HEAD_DETACH in checkout_up_to_date()\n\n builtin/rebase.c | 2 ++\n t/t3400-rebase.sh | 18 +++++++++++-------\n 2 files changed, 13 insertions(+), 7 deletions(-)\n\n\nbase-commit: b896f729e240d250cf56899e6a0073f6aa469f5d\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v5\nPull-Request: https://github.com/git/git/pull/1226\n\nRange-diff vs v4:\n\n 1: cac51a949ee < -: ----------- rebase: test showing bug in rebase with non-branch\n 2: 5c40e116eba ! 1: 9dbd2ba430a rebase: use test_commit helper in setup\n @@ t/t3400-rebase.sh: test_expect_success 'prepare repository with topic branches'\n \tgit checkout -f main &&\n \techo Third >>A &&\n \tgit update-index A &&\n - \tgit commit -m \"Modify A.\" &&\n - \tgit checkout -b side my-topic-branch &&\n --\techo Side >>C &&\n --\tgit add C &&\n --\tgit commit -m \"Add C\" &&\n -+\ttest_commit --no-tag \"Add C\" C Side &&\n - \tgit checkout -f my-topic-branch &&\n - \tgit tag topic\n - '\n -@@ t/t3400-rebase.sh: test_expect_success 'rebase off of the previous branch using \"-\"' '\n - test_expect_success 'rebase a single mode change' '\n - \tgit checkout main &&\n - \tgit branch -D topic &&\n --\techo 1 >X &&\n --\tgit add X &&\n --\ttest_tick &&\n --\tgit commit -m prepare &&\n -+\ttest_commit prepare X 1 &&\n - \tgit checkout -b modechange HEAD^ &&\n - \techo 1 >X &&\n - \tgit add X &&\n 3: 13c5955c317 ! 2: 1dd5bb21012 rebase: set REF_HEAD_DETACH in checkout_up_to_date()\n @@ Metadata\n ## Commit message ##\n rebase: set REF_HEAD_DETACH in checkout_up_to_date()\n \n - Fixes a bug whereby rebase updates the deferenced reference HEAD points\n - to instead of HEAD directly.\n -\n - If HEAD is on main and if the following is a fast-forward operation,\n -\n - git rebase $(git rev-parse main) $(git rev-parse topic)\n -\n - Instead of HEAD being set to $(git rev-parse topic), rebase erroneously\n - dereferences HEAD and sets main to $(git rev-parse topic). See [1] for\n - the original bug report.\n + \"git rebase A B\" where B is not a commit should behave as if the\n + HEAD got detached at B and then the detached HEAD got rebased on top\n + of A. A bug however overwrites the current branch to point at B,\n + when B is a descendant of A (i.e. the rebase ends up being a\n + fast-forward). See [1] for the original bug report.\n \n The callstack from checkout_up_to_date() is the following:\n \n - cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs()\n - -> update_ref()\n + cmd_rebase()\n + -> checkout_up_to_date()\n + -> reset_head()\n + -> update_refs()\n + -> update_ref()\n \n - When is not a valid branch but an oid, rebase sets the head_name\n - of rebase_options to NULL. This value gets passed down this call chain\n - through the branch member of reset_head_opts also getting set to NULL\n - all the way to update_refs().\n + When B is not a valid branch but an oid, rebase sets the head_name\n + of rebase_options to NULL. This value gets passed down this call\n + chain through the branch member of reset_head_opts also getting set\n + to NULL all the way to update_refs().\n \n Then update_refs() checks ropts.branch to decide whether or not to switch\n branches. If ropts.branch is NULL, it calls update_ref() to update HEAD.\n At this point however, from rebase's point of view, we want a detached\n HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH\n flag, the update_ref() call will deference HEAD and update the branch its\n - pointing to, which in the above example is main.\n -\n - The correct behavior is that git rebase should update HEAD to $(git\n - rev-parse topic) without dereferencing it.\n + pointing to. We want the HEAD detached at B instead.\n \n - Fix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date\n - if is not a valid branch. so that once reset_head() calls\n - update_refs(), it calls update_ref() with REF_NO_DEREF which updates\n - HEAD directly intead of deferencing it and updating the branch that HEAD\n - points to.\n + Fix this bug by adding the RESET_HEAD_DETACH flag in\n + checkout_up_to_date if B is not a valid branch, so that once\n + reset_head() calls update_refs(), it calls update_ref() with\n + REF_NO_DEREF which updates HEAD directly intead of deferencing it\n + and updating the branch that HEAD points to.\n \n Also add a test to ensure the correct behavior.\n \n - 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/\n + [1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/\n \n + Reported-by: Michael McClimon \n Signed-off-by: John Cai \n + Signed-off-by: Junio C Hamano \n \n ## builtin/rebase.c ##\n @@ builtin/rebase.c: static int checkout_up_to_date(struct rebase_options *options)\n @@ t/t3400-rebase.sh: test_expect_success 'switch to branch not checked out' '\n \tgit rebase main other\n '\n \n --test_expect_success 'switch to non-branch changes branch HEAD points to' '\n +test_expect_success 'switch to non-branch detaches HEAD' '\n - \tgit checkout main &&\n - \told_main=$(git rev-parse HEAD) &&\n - \tgit rebase First Second^0 &&\n --\ttest_cmp_rev HEAD main &&\n --\ttest_cmp_rev main $(git rev-parse Second) &&\n --\tgit symbolic-ref HEAD\n ++\tgit checkout main &&\n ++\told_main=$(git rev-parse HEAD) &&\n ++\tgit rebase First Second^0 &&\n +\ttest_cmp_rev HEAD Second &&\n +\ttest_cmp_rev main $old_main &&\n +\ttest_must_fail git symbolic-ref HEAD\n - '\n - \n ++'\n ++\n test_expect_success 'refuse to switch to branch checked out elsewhere' '\n + \tgit checkout main &&\n + \tgit worktree add wt &&\n\n-- \ngitgitgadget\n"},{"id":"451624","messageId":"9dbd2ba430aa481bc7de69dd51bd89aeafd9e208.1647611643.git.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":"pull.1226.v5.git.git.1647611643.gitgitgadget@gmail.com","subject":"[PATCH v5 1/2] rebase: use test_commit helper in setup","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-18T13:54:02Z","receivedAt":"2022-03-18T13:54:13Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\nTo prepare for the next commit that will test rebase with oids instead\nof branch names, update the rebase setup test to add a couple of tags we\ncan use. This uses the test_commit helper so we can replace some lines\nthat add a commit manually.\n\nSetting logAllRefUpdates is not necessary because it's on by default for\nrepositories with a working tree.\n\nHelped-by: Phillip Wood \nSigned-off-by: John Cai \n---\n t/t3400-rebase.sh | 9 ++-------\n 1 file changed, 2 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 71b1735e1dd..0643d015255 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address\n export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL\n \n test_expect_success 'prepare repository with topic branches' '\n-\tgit config core.logAllRefUpdates true &&\n-\techo First >A &&\n-\tgit update-index --add A &&\n-\tgit commit -m \"Add A.\" &&\n+\ttest_commit \"Add A.\" A First First &&\n \tgit checkout -b force-3way &&\n \techo Dummy >Y &&\n \tgit update-index --add Y &&\n@@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '\n \tgit mv A D/A &&\n \tgit commit -m \"Move A.\" &&\n \tgit checkout -b my-topic-branch main &&\n-\techo Second >B &&\n-\tgit update-index --add B &&\n-\tgit commit -m \"Add B.\" &&\n+\ttest_commit \"Add B.\" B Second Second &&\n \tgit checkout -f main &&\n \techo Third >>A &&\n \tgit update-index A &&\n-- \ngitgitgadget\n\n"},{"id":"451625","messageId":"1dd5bb210124480132a78e5ac64384713d734299.1647611643.git.gitgitgadget@gmail.com","threadId":"57536","inReplyTo":"pull.1226.v5.git.git.1647611643.gitgitgadget@gmail.com","subject":"[PATCH v5 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()","fromName":"John Cai via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2022-03-18T13:54:03Z","receivedAt":"2022-03-18T13:54:14Z","isPatch":true,"sender":{"key":"johncai86@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2354211?v=4"},"body":"From: John Cai \n\n\"git rebase A B\" where B is not a commit should behave as if the\nHEAD got detached at B and then the detached HEAD got rebased on top\nof A. A bug however overwrites the current branch to point at B,\nwhen B is a descendant of A (i.e. the rebase ends up being a\nfast-forward). See [1] for the original bug report.\n\nThe callstack from checkout_up_to_date() is the following:\n\ncmd_rebase()\n-> checkout_up_to_date()\n -> reset_head()\n -> update_refs()\n -> update_ref()\n\nWhen B is not a valid branch but an oid, rebase sets the head_name\nof rebase_options to NULL. This value gets passed down this call\nchain through the branch member of reset_head_opts also getting set\nto NULL all the way to update_refs().\n\nThen update_refs() checks ropts.branch to decide whether or not to switch\nbranches. If ropts.branch is NULL, it calls update_ref() to update HEAD.\nAt this point however, from rebase's point of view, we want a detached\nHEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH\nflag, the update_ref() call will deference HEAD and update the branch its\npointing to. We want the HEAD detached at B instead.\n\nFix this bug by adding the RESET_HEAD_DETACH flag in\ncheckout_up_to_date if B is not a valid branch, so that once\nreset_head() calls update_refs(), it calls update_ref() with\nREF_NO_DEREF which updates HEAD directly intead of deferencing it\nand updating the branch that HEAD points to.\n\nAlso add a test to ensure the correct behavior.\n\n[1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/\n\nReported-by: Michael McClimon \nSigned-off-by: John Cai \nSigned-off-by: Junio C Hamano \n---\n builtin/rebase.c | 2 ++\n t/t3400-rebase.sh | 9 +++++++++\n 2 files changed, 11 insertions(+)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex b29ad2b65e7..27fde7bf281 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options)\n \tropts.oid = &options->orig_head;\n \tropts.branch = options->head_name;\n \tropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;\n+\tif (!ropts.branch)\n+\t\tropts.flags |= RESET_HEAD_DETACH;\n \tropts.head_msg = buf.buf;\n \tif (reset_head(the_repository, &ropts) < 0)\n \t\tret = error(_(\"could not switch to %s\"), options->switch_to);\ndiff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\nindex 0643d015255..d5a8ee39fc4 100755\n--- a/t/t3400-rebase.sh\n+++ b/t/t3400-rebase.sh\n@@ -394,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '\n \tgit rebase main other\n '\n \n+test_expect_success 'switch to non-branch detaches HEAD' '\n+\tgit checkout main &&\n+\told_main=$(git rev-parse HEAD) &&\n+\tgit rebase First Second^0 &&\n+\ttest_cmp_rev HEAD Second &&\n+\ttest_cmp_rev main $old_main &&\n+\ttest_must_fail git symbolic-ref HEAD\n+'\n+\n test_expect_success 'refuse to switch to branch checked out elsewhere' '\n \tgit checkout main &&\n \tgit worktree add wt &&\n-- \ngitgitgadget\n"},{"id":"451633","messageId":"xmqq7d8rb3vx.fsf@gitster.g","threadId":"57536","inReplyTo":"9dbd2ba430aa481bc7de69dd51bd89aeafd9e208.1647611643.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 1/2] rebase: use test_commit helper in setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-18T16:51:30Z","receivedAt":"2022-03-18T16:51:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> From: John Cai \n>\n> To prepare for the next commit that will test rebase with oids instead\n> of branch names, update the rebase setup test to add a couple of tags we\n> can use. This uses the test_commit helper so we can replace some lines\n> that add a commit manually.\n>\n> Setting logAllRefUpdates is not necessary because it's on by default for\n> repositories with a working tree.\n>\n> Helped-by: Phillip Wood \n> Signed-off-by: John Cai \n> ---\n> t/t3400-rebase.sh | 9 ++-------\n> 1 file changed, 2 insertions(+), 7 deletions(-)\n\nLooks much simpler and obviously correct ;-)\n\n> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh\n> index 71b1735e1dd..0643d015255 100755\n> --- a/t/t3400-rebase.sh\n> +++ b/t/t3400-rebase.sh\n> @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address\n> export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL\n> \n> test_expect_success 'prepare repository with topic branches' '\n> -\tgit config core.logAllRefUpdates true &&\n> -\techo First >A &&\n> -\tgit update-index --add A &&\n> -\tgit commit -m \"Add A.\" &&\n> +\ttest_commit \"Add A.\" A First First &&\n> \tgit checkout -b force-3way &&\n> \techo Dummy >Y &&\n> \tgit update-index --add Y &&\n> @@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '\n> \tgit mv A D/A &&\n> \tgit commit -m \"Move A.\" &&\n> \tgit checkout -b my-topic-branch main &&\n> -\techo Second >B &&\n> -\tgit update-index --add B &&\n> -\tgit commit -m \"Add B.\" &&\n> +\ttest_commit \"Add B.\" B Second Second &&\n> \tgit checkout -f main &&\n> \techo Third >>A &&\n> \tgit update-index A &&\n"},{"id":"451634","messageId":"xmqq35jfb3p1.fsf@gitster.g","threadId":"57536","inReplyTo":"pull.1226.v5.git.git.1647611643.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 0/2] rebase: update HEAD when is an oid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-03-18T16:55:38Z","receivedAt":"2022-03-18T16:55:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"John Cai via GitGitGadget\" writes:\n\n> Fixes a bug [1] reported by Michael McClimon where rebase with oids will\n> erroneously update the branch HEAD points to.\n\nLooking good. I am tempted to say that we should declare victory.\n\nThanks for working on this fix.\n\n"}]}