| Message ID | pull.1226.git.git.1646975144178.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: set REF_HEAD_DETACH in checkout_up_to_date() | expand |
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index 71b1735e1dd..0b92e78976c 100755 > --- a/t/t3400-rebase.sh > +++ b/t/t3400-rebase.sh > @@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' ' > ) > ' > > +test_expect_success 'rebase with oids' ' > + git init main-wt && > + ( > + cd main-wt && > + >file && > + git add file && > + git commit -m initial && > + git checkout -b side && > + echo >>file && > + git commit -a -m side && > + git checkout main && The above is sufficient set-up. > + git tag hold && > + git checkout -B main hold && These two are unnecessary. It was there in my bisect "runme" script only because I originally used an out-of-line repository that is prepared beforehand, so that "move main back to hold and rerun the rebase" can be done regardless of the bug in the previous version checked during "bisect run". > + git rev-parse main >pre && > + git rebase $(git rev-parse main) $(git rev-parse side) && > + git rev-parse main >post && > + test "$(git rev-parse side)" = "$(cat .git/HEAD)" && You may want to prepare for segfaulting "git rev-parse" in the above three lines. Never "cat .git/HEAD". use "git rev-parse". > + test_cmp pre post > + ) > +' > + > test_done > > base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b
"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > This is happening because on a fast foward with an oid as a <branch>, > update_refs() will only call update_ref() with REF_NO_DEREF if > RESET_HEAD_DETACH is set. This change was made in 176f5d96 (built-in rebase > --autostash: leave the current branch alone if possible, > 2018-11-07). In rebase, we are not setting the RESET_HEAD_DETACH flag, > which means that the update_ref() call ends up dereferencing > HEAD and updating it to the oid used as <branch>. > > The correct behavior is that git rebase should update HEAD to $(git > rev-parse topic) without dereferencing it. It is unintuitive that unconditionally setting the RESET_HEAD_DETACH bit is the right solution. If the command weren't "rebase master side^0" but "rebase master side", i.e. "please rebase the side branch itself, not an unnamed branch created out of the side branch, on master", according to <reset.h>, we ought to end up being on a detached HEAD, as reset_head() with the bit /* Request a detached checkout */ #define RESET_HEAD_DETACH (1<<0) requests a detached checkout. But that apparently is not what would happen with your patch applied. Puzzled. The solution to the puzzle probably deserves to be in the proposed log message. Thanks.
Hi Junio, On 11 Mar 2022, at 0:55, Junio C Hamano wrote: > "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> This is happening because on a fast foward with an oid as a <branch>, >> update_refs() will only call update_ref() with REF_NO_DEREF if >> RESET_HEAD_DETACH is set. This change was made in 176f5d96 (built-in rebase >> --autostash: leave the current branch alone if possible, >> 2018-11-07). In rebase, we are not setting the RESET_HEAD_DETACH flag, >> which means that the update_ref() call ends up dereferencing >> HEAD and updating it to the oid used as <branch>. >> >> The correct behavior is that git rebase should update HEAD to $(git >> rev-parse topic) without dereferencing it. > > It is unintuitive that unconditionally setting the RESET_HEAD_DETACH > bit is the right solution. > > If the command weren't "rebase master side^0" but "rebase master > side", i.e. "please rebase the side branch itself, not an unnamed > branch created out of the side branch, on master", according to > <reset.h>, we ought to end up being on a detached HEAD, as > reset_head() with the bit > > /* Request a detached checkout */ > #define RESET_HEAD_DETACH (1<<0) > > requests a detached checkout. But that apparently is not what would > happen with your patch applied. > > Puzzled. The solution to the puzzle probably deserves to be in the > proposed log message. Good point. Thinking aloud, here is the callstack. checkout_up_to_date() -> reset_head() -> update_refs() -> update_ref() if <branch> is not a valid ref, rebase_options head_name is set to NULL. This eventually leads update_refs() to decide that it doesn't need to switch to a branch via its switch_to_branch variable. reset.c: if (!switch_to_branch) ret = update_ref(reflog_head, "HEAD", oid, head, detach_head ? REF_NO_DEREF : 0, UPDATE_REFS_MSG_ON_ERR); else { ret = update_ref(reflog_branch ? reflog_branch : reflog_head, switch_to_branch, oid, NULL, 0, UPDATE_REFS_MSG_ON_ERR); if (!ret) ret = create_symref("HEAD", switch_to_branch, reflog_head); } since the flags do not include RESET_HEAD_DETACH, detach_head is set to false and we get a deferenced HEAD update. The solution I came up with works because when <branch> __is__ a valid branch, udpate_refs() takes a different code path that calls create_symref() with a branch, which is why we don't end up with a detached HEAD. I see why this is confusing though. From checkout_up_to_date's perspective it looks like we are unconditionally detaching HEAD. So what we could do is only set the flag in checkout_up_to_date() when, from checkout_up_to_date's perspective, we will end up with a detached head. something like this: diff --git a/builtin/rebase.c b/builtin/rebase.c index b29ad2b65e72..f0403fb12421 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -827,8 +827,10 @@ static int checkout_up_to_date(struct rebase_options *options) getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options->switch_to); ropts.oid = &options->orig_head; - ropts.branch = options->head_name; ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; + ropts.branch = options->head_name; + if (!ropts.branch) + ropts.flags |= RESET_HEAD_DETACH; ropts.head_msg = buf.buf; if (reset_head(the_repository, &ropts) < 0) ret = error(_("could not switch to %s"), options->switch_to); Otherwise, checkout_up_to_date() has to implicitly know the downstream logic in update_refs(). I believe that's the main source of the confusion--is that right? > > Thanks. Thanks John
Hi John Thanks for working on this On 11/03/2022 05:05, John Cai via GitGitGadget wrote: > From: John Cai <johncai86@gmail.com> > > Fixes a bug whereby rebase updates the deferenced reference HEAD points > to instead of HEAD directly. > > If HEAD is on main and if the following is a fast-forward operation, > > git rebase $(git rev-parse main) $(git rev-parse topic) > > Instead of HEAD being set to $(git rev-parse topic), rebase erroneously > dereferences HEAD and sets main to $(git rev-parse topic). This bug was > reported by Michael McClimon. See [1]. 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) > This is happening because on a fast foward with an oid as a <branch>, > update_refs() will only call update_ref() with REF_NO_DEREF if > RESET_HEAD_DETACH is set. This change was made in 176f5d96 (built-in rebase > --autostash: leave the current branch alone if possible, > 2018-11-07). In rebase, we are not setting the RESET_HEAD_DETACH flag, > which means that the update_ref() call ends up dereferencing > HEAD and updating it to the oid used as <branch>. > > The correct behavior is that git rebase should update HEAD to $(git > rev-parse topic) without dereferencing it. > > Fix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date 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. > so that once reset_head() calls update_refs(), it calls update_ref() with > REF_NO_DEREF which updates HEAD directly intead of deferencing it and > updating the branch that HEAD points to. > > Also add a test to ensure this behavior. > > 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/ Maybe Reported-by: Michael McClimon <michael@mcclimon.org> ? > Signed-off-by: John Cai <johncai86@gmail.com> > --- > rebase: update HEAD when is an oid > > Fixes a bug [1] reported by Michael McClimon where rebase with oids will > erroneously update the branch HEAD points to. > > 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/ > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v1 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v1 > Pull-Request: https://github.com/git/git/pull/1226 > > builtin/rebase.c | 2 +- > t/t3400-rebase.sh | 21 +++++++++++++++++++++ > 2 files changed, 22 insertions(+), 1 deletion(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index b29ad2b65e7..52afeffcc2e 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -828,7 +828,7 @@ static int checkout_up_to_date(struct rebase_options *options) > options->switch_to); > ropts.oid = &options->orig_head; > ropts.branch = options->head_name; > - ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; > + ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK | RESET_HEAD_DETACH; I think it would be clearer if the post image ended up as ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK if (options->head_name) ropts.branch = option->head_name else ropts.flags |= RESET_HEAD_DETACH and we changed reset_head() to BUG() if both branch and RESET_HEAD_DETACH are given. > ropts.head_msg = buf.buf; > if (reset_head(the_repository, &ropts) < 0) > ret = error(_("could not switch to %s"), options->switch_to); > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index 71b1735e1dd..0b92e78976c 100755 > --- a/t/t3400-rebase.sh > +++ b/t/t3400-rebase.sh > @@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' ' > ) > ' > > +test_expect_success 'rebase with oids' ' > + git init main-wt && > + ( > + cd main-wt && > + >file && > + git add file && > + git commit -m initial && > + git checkout -b side && > + echo >>file && > + git commit -a -m side && > + git checkout main && > + git tag hold && > + git checkout -B main hold && > + git rev-parse main >pre && > + git rebase $(git rev-parse main) $(git rev-parse side) && > + git rev-parse main >post && > + test "$(git rev-parse side)" = "$(cat .git/HEAD)" && > + test_cmp pre post > + ) > +' 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. Best Wishes Phillip ---- >8 ---- diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 71b1735e1d..d5a8ee39fc 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL test_expect_success 'prepare repository with topic branches' ' - git config core.logAllRefUpdates true && - echo First >A && - git update-index --add A && - git commit -m "Add A." && + test_commit "Add A." A First First && git checkout -b force-3way && echo Dummy >Y && git update-index --add Y && @@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' ' git mv A D/A && git commit -m "Move A." && git checkout -b my-topic-branch main && - echo Second >B && - git update-index --add B && - git commit -m "Add B." && + test_commit "Add B." B Second Second && git checkout -f main && echo Third >>A && git update-index A && @@ -399,6 +394,15 @@ test_expect_success 'switch to branch not checked out' ' git rebase main other ' +test_expect_success 'switch to non-branch detaches HEAD' ' + git checkout main && + old_main=$(git rev-parse HEAD) && + git rebase First Second^0 && + test_cmp_rev HEAD Second && + test_cmp_rev main $old_main && + test_must_fail git symbolic-ref HEAD +' + test_expect_success 'refuse to switch to branch checked out elsewhere' ' git checkout main && git worktree add wt &&
Hi Phillip, On 11 Mar 2022, at 10:05, Phillip Wood wrote: > Hi John > > Thanks for working on this > > On 11/03/2022 05:05, John Cai via GitGitGadget wrote: >> From: John Cai <johncai86@gmail.com> >> >> Fixes a bug whereby rebase updates the deferenced reference HEAD points >> to instead of HEAD directly. >> >> If HEAD is on main and if the following is a fast-forward operation, >> >> git rebase $(git rev-parse main) $(git rev-parse topic) >> >> Instead of HEAD being set to $(git rev-parse topic), rebase erroneously >> dereferences HEAD and sets main to $(git rev-parse topic). This bug was >> reported by Michael McClimon. See [1]. > > 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) Thanks, will adjust. > >> This is happening because on a fast foward with an oid as a <branch>, >> update_refs() will only call update_ref() with REF_NO_DEREF if >> RESET_HEAD_DETACH is set. This change was made in 176f5d96 (built-in rebase >> --autostash: leave the current branch alone if possible, >> 2018-11-07). In rebase, we are not setting the RESET_HEAD_DETACH flag, >> which means that the update_ref() call ends up dereferencing >> HEAD and updating it to the oid used as <branch>. >> >> The correct behavior is that git rebase should update HEAD to $(git >> rev-parse topic) without dereferencing it. >> >> Fix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date > > 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. > >> so that once reset_head() calls update_refs(), it calls update_ref() with >> REF_NO_DEREF which updates HEAD directly intead of deferencing it and >> updating the branch that HEAD points to. >> >> Also add a test to ensure this behavior. >> >> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/ > > Maybe > Reported-by: Michael McClimon <michael@mcclimon.org> > ? > >> Signed-off-by: John Cai <johncai86@gmail.com> >> --- >> rebase: update HEAD when is an oid >> Fixes a bug [1] reported by Michael McClimon where rebase with oids will >> erroneously update the branch HEAD points to. >> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/ >> >> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v1 >> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v1 >> Pull-Request: https://github.com/git/git/pull/1226 >> >> builtin/rebase.c | 2 +- >> t/t3400-rebase.sh | 21 +++++++++++++++++++++ >> 2 files changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/builtin/rebase.c b/builtin/rebase.c >> index b29ad2b65e7..52afeffcc2e 100644 >> --- a/builtin/rebase.c >> +++ b/builtin/rebase.c >> @@ -828,7 +828,7 @@ static int checkout_up_to_date(struct rebase_options *options) >> options->switch_to); >> ropts.oid = &options->orig_head; >> ropts.branch = options->head_name; >> - ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; >> + ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK | RESET_HEAD_DETACH; > > I think it would be clearer if the post image ended up as > > ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK > if (options->head_name) > ropts.branch = option->head_name > else > ropts.flags |= RESET_HEAD_DETACH Yes, this is what I had in mind as well :). > > and we changed reset_head() to BUG() if both branch and RESET_HEAD_DETACH are given. I didn't consider this though, thanks for the suggestion. > >> ropts.head_msg = buf.buf; >> if (reset_head(the_repository, &ropts) < 0) >> ret = error(_("could not switch to %s"), options->switch_to); >> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh >> index 71b1735e1dd..0b92e78976c 100755 >> --- a/t/t3400-rebase.sh >> +++ b/t/t3400-rebase.sh >> @@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' ' >> ) >> ' >> +test_expect_success 'rebase with oids' ' >> + git init main-wt && >> + ( >> + cd main-wt && >> + >file && >> + git add file && >> + git commit -m initial && >> + git checkout -b side && >> + echo >>file && >> + git commit -a -m side && >> + git checkout main && >> + git tag hold && >> + git checkout -B main hold && >> + git rev-parse main >pre && >> + git rebase $(git rev-parse main) $(git rev-parse side) && >> + git rev-parse main >post && >> + test "$(git rev-parse side)" = "$(cat .git/HEAD)" && >> + test_cmp pre post >> + ) >> +' > > 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. sounds good to me, might as well clean things up while we're at it. > > Best Wishes > > Phillip > > ---- >8 ---- > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index 71b1735e1d..d5a8ee39fc 100755 > --- a/t/t3400-rebase.sh > +++ b/t/t3400-rebase.sh > @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address > export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL > > test_expect_success 'prepare repository with topic branches' ' > - git config core.logAllRefUpdates true && > - echo First >A && > - git update-index --add A && > - git commit -m "Add A." && > + test_commit "Add A." A First First && > git checkout -b force-3way && > echo Dummy >Y && > git update-index --add Y && > @@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' ' > git mv A D/A && > git commit -m "Move A." && > git checkout -b my-topic-branch main && > - echo Second >B && > - git update-index --add B && > - git commit -m "Add B." && > + test_commit "Add B." B Second Second && > git checkout -f main && > echo Third >>A && > git update-index A && > @@ -399,6 +394,15 @@ test_expect_success 'switch to branch not checked out' ' > git rebase main other > ' > > +test_expect_success 'switch to non-branch detaches HEAD' ' > + git checkout main && > + old_main=$(git rev-parse HEAD) && > + git rebase First Second^0 && > + test_cmp_rev HEAD Second && > + test_cmp_rev main $old_main && > + test_must_fail git symbolic-ref HEAD > +' > + > test_expect_success 'refuse to switch to branch checked out elsewhere' ' > git checkout main && > git worktree add wt &&
On 11 Mar 2022, at 10:05, Phillip Wood wrote: > Hi John > > Thanks for working on this > > On 11/03/2022 05:05, John Cai via GitGitGadget wrote: >> From: John Cai <johncai86@gmail.com> >> >> Fixes a bug whereby rebase updates the deferenced reference HEAD points >> to instead of HEAD directly. >> >> If HEAD is on main and if the following is a fast-forward operation, >> >> git rebase $(git rev-parse main) $(git rev-parse topic) >> >> Instead of HEAD being set to $(git rev-parse topic), rebase erroneously >> dereferences HEAD and sets main to $(git rev-parse topic). This bug was >> reported by Michael McClimon. See [1]. > > 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) > >> This is happening because on a fast foward with an oid as a <branch>, >> update_refs() will only call update_ref() with REF_NO_DEREF if >> RESET_HEAD_DETACH is set. This change was made in 176f5d96 (built-in rebase >> --autostash: leave the current branch alone if possible, >> 2018-11-07). In rebase, we are not setting the RESET_HEAD_DETACH flag, >> which means that the update_ref() call ends up dereferencing >> HEAD and updating it to the oid used as <branch>. >> >> The correct behavior is that git rebase should update HEAD to $(git >> rev-parse topic) without dereferencing it. >> >> Fix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date > > 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. > >> so that once reset_head() calls update_refs(), it calls update_ref() with >> REF_NO_DEREF which updates HEAD directly intead of deferencing it and >> updating the branch that HEAD points to. >> >> Also add a test to ensure this behavior. >> >> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/ > > Maybe > Reported-by: Michael McClimon <michael@mcclimon.org> > ? > >> Signed-off-by: John Cai <johncai86@gmail.com> >> --- >> rebase: update HEAD when is an oid >> Fixes a bug [1] reported by Michael McClimon where rebase with oids will >> erroneously update the branch HEAD points to. >> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/ >> >> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v1 >> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v1 >> Pull-Request: https://github.com/git/git/pull/1226 >> >> builtin/rebase.c | 2 +- >> t/t3400-rebase.sh | 21 +++++++++++++++++++++ >> 2 files changed, 22 insertions(+), 1 deletion(-) >> >> diff --git a/builtin/rebase.c b/builtin/rebase.c >> index b29ad2b65e7..52afeffcc2e 100644 >> --- a/builtin/rebase.c >> +++ b/builtin/rebase.c >> @@ -828,7 +828,7 @@ static int checkout_up_to_date(struct rebase_options *options) >> options->switch_to); >> ropts.oid = &options->orig_head; >> ropts.branch = options->head_name; >> - ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; >> + ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK | RESET_HEAD_DETACH; > > I think it would be clearer if the post image ended up as This is entirely for my own sake and revealing my ignorance, but what exactly does "pre" and "post" image refer to? > > ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK > if (options->head_name) > ropts.branch = option->head_name > else > ropts.flags |= RESET_HEAD_DETACH > > and we changed reset_head() to BUG() if both branch and RESET_HEAD_DETACH are given. > >> ropts.head_msg = buf.buf; >> if (reset_head(the_repository, &ropts) < 0) >> ret = error(_("could not switch to %s"), options->switch_to); >> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh >> index 71b1735e1dd..0b92e78976c 100755 >> --- a/t/t3400-rebase.sh >> +++ b/t/t3400-rebase.sh >> @@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' ' >> ) >> ' >> +test_expect_success 'rebase with oids' ' >> + git init main-wt && >> + ( >> + cd main-wt && >> + >file && >> + git add file && >> + git commit -m initial && >> + git checkout -b side && >> + echo >>file && >> + git commit -a -m side && >> + git checkout main && >> + git tag hold && >> + git checkout -B main hold && >> + git rev-parse main >pre && >> + git rebase $(git rev-parse main) $(git rev-parse side) && >> + git rev-parse main >post && >> + test "$(git rev-parse side)" = "$(cat .git/HEAD)" && >> + test_cmp pre post >> + ) >> +' > > 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. > > Best Wishes > > Phillip > > ---- >8 ---- > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index 71b1735e1d..d5a8ee39fc 100755 > --- a/t/t3400-rebase.sh > +++ b/t/t3400-rebase.sh > @@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address > export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL > > test_expect_success 'prepare repository with topic branches' ' > - git config core.logAllRefUpdates true && > - echo First >A && > - git update-index --add A && > - git commit -m "Add A." && > + test_commit "Add A." A First First && > git checkout -b force-3way && > echo Dummy >Y && > git update-index --add Y && > @@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' ' > git mv A D/A && > git commit -m "Move A." && > git checkout -b my-topic-branch main && > - echo Second >B && > - git update-index --add B && > - git commit -m "Add B." && > + test_commit "Add B." B Second Second && > git checkout -f main && > echo Third >>A && > git update-index A && > @@ -399,6 +394,15 @@ test_expect_success 'switch to branch not checked out' ' > git rebase main other > ' > > +test_expect_success 'switch to non-branch detaches HEAD' ' > + git checkout main && > + old_main=$(git rev-parse HEAD) && > + git rebase First Second^0 && > + test_cmp_rev HEAD Second && > + test_cmp_rev main $old_main && > + test_must_fail git symbolic-ref HEAD > +' > + > test_expect_success 'refuse to switch to branch checked out elsewhere' ' > git checkout main && > git worktree add wt &&
Hi John On 11/03/2022 19:42, John Cai wrote: > [...] >> >> I think it would be clearer if the post image ended up as > > This is entirely for my own sake and revealing my ignorance, but what exactly > does "pre" and "post" image refer to? The old version and the new version Best Wishes Phillip
diff --git a/builtin/rebase.c b/builtin/rebase.c index b29ad2b65e7..52afeffcc2e 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -828,7 +828,7 @@ static int checkout_up_to_date(struct rebase_options *options) options->switch_to); ropts.oid = &options->orig_head; ropts.branch = options->head_name; - ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; + ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK | RESET_HEAD_DETACH; ropts.head_msg = buf.buf; if (reset_head(the_repository, &ropts) < 0) ret = error(_("could not switch to %s"), options->switch_to); diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 71b1735e1dd..0b92e78976c 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' ' ) ' +test_expect_success 'rebase with oids' ' + git init main-wt && + ( + cd main-wt && + >file && + git add file && + git commit -m initial && + git checkout -b side && + echo >>file && + git commit -a -m side && + git checkout main && + git tag hold && + git checkout -B main hold && + git rev-parse main >pre && + git rebase $(git rev-parse main) $(git rev-parse side) && + git rev-parse main >post && + test "$(git rev-parse side)" = "$(cat .git/HEAD)" && + test_cmp pre post + ) +' + test_done