* [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() @ 2022-03-11 5:05 John Cai via GitGitGadget 2022-03-11 5:33 ` Junio C Hamano ` (3 more replies) 0 siblings, 4 replies; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-11 5:05 UTC (permalink / raw) To: git; +Cc: John Cai, John Cai 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]. 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 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/ 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; 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 base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b -- gitgitgadget ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-11 5:05 [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget @ 2022-03-11 5:33 ` Junio C Hamano 2022-03-11 5:55 ` Junio C Hamano ` (2 subsequent siblings) 3 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2022-03-11 5:33 UTC (permalink / raw) To: John Cai via GitGitGadget; +Cc: git, John Cai "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 ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-11 5:05 [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget 2022-03-11 5:33 ` Junio C Hamano @ 2022-03-11 5:55 ` Junio C Hamano 2022-03-11 14:14 ` John Cai 2022-03-11 15:05 ` Phillip Wood 2022-03-11 17:24 ` [PATCH v2 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 3 siblings, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2022-03-11 5:55 UTC (permalink / raw) To: John Cai via GitGitGadget; +Cc: git, John Cai "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. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-11 5:55 ` Junio C Hamano @ 2022-03-11 14:14 ` John Cai 0 siblings, 0 replies; 41+ messages in thread From: John Cai @ 2022-03-11 14:14 UTC (permalink / raw) To: Junio C Hamano; +Cc: John Cai via GitGitGadget, git 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 ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-11 5:05 [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget 2022-03-11 5:33 ` Junio C Hamano 2022-03-11 5:55 ` Junio C Hamano @ 2022-03-11 15:05 ` Phillip Wood 2022-03-11 15:28 ` John Cai 2022-03-11 19:42 ` John Cai 2022-03-11 17:24 ` [PATCH v2 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 3 siblings, 2 replies; 41+ messages in thread From: Phillip Wood @ 2022-03-11 15:05 UTC (permalink / raw) To: John Cai via GitGitGadget, git; +Cc: John Cai, Junio C Hamano 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 && ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-11 15:05 ` Phillip Wood @ 2022-03-11 15:28 ` John Cai 2022-03-11 19:42 ` John Cai 1 sibling, 0 replies; 41+ messages in thread From: John Cai @ 2022-03-11 15:28 UTC (permalink / raw) To: phillip.wood; +Cc: John Cai via GitGitGadget, git, Junio C Hamano 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 && ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-11 15:05 ` Phillip Wood 2022-03-11 15:28 ` John Cai @ 2022-03-11 19:42 ` John Cai 2022-03-11 21:31 ` Phillip Wood 1 sibling, 1 reply; 41+ messages in thread From: John Cai @ 2022-03-11 19:42 UTC (permalink / raw) To: phillip.wood; +Cc: John Cai via GitGitGadget, git, Junio C Hamano 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 && ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-11 19:42 ` John Cai @ 2022-03-11 21:31 ` Phillip Wood 0 siblings, 0 replies; 41+ messages in thread From: Phillip Wood @ 2022-03-11 21:31 UTC (permalink / raw) To: John Cai, phillip.wood; +Cc: John Cai via GitGitGadget, git, Junio C Hamano 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 ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 0/2] rebase: update HEAD when is an oid 2022-03-11 5:05 [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget ` (2 preceding siblings ...) 2022-03-11 15:05 ` Phillip Wood @ 2022-03-11 17:24 ` John Cai via GitGitGadget 2022-03-11 17:24 ` [PATCH v2 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget ` (2 more replies) 3 siblings, 3 replies; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-11 17:24 UTC (permalink / raw) To: git; +Cc: Phillip Wood, Junio C Hamano, John Cai 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/ This patch has two parts: * updates rebase setup test to add some tags we will use, while swapping out manual commit creation with the test_commit helper * sets RESET_HARD_DETACH flag if branch is not a valid refname changes since v1: * only set RESET_HEAD_DETACH if is not a valid branch * updated tests to use existing setup John Cai (2): rebase: use test_commit helper in setup rebase: set REF_HEAD_DETACH in checkout_up_to_date() builtin/rebase.c | 5 ++++- reset.c | 3 +++ t/t3400-rebase.sh | 18 +++++++++++------- 3 files changed, 18 insertions(+), 8 deletions(-) base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v2 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v2 Pull-Request: https://github.com/git/git/pull/1226 Range-diff vs v1: -: ----------- > 1: f3f084adfa6 rebase: use test_commit helper in setup 1: 2538fd986d2 ! 2: 0e3c73375c1 rebase: set REF_HEAD_DETACH in checkout_up_to_date() @@ Commit message 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]. + dereferences HEAD and sets main to $(git rev-parse topic). See [1] for + the original bug report. - 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 callstack from checkout_up_to_date() is the following: + + cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs() + -> update_ref() + + When <branch> is not a valid branch but a sha, rebase sets the head_name + of rebase_options to NULL. This value gets passed down this call chain + through the branch member of reset_head_opts also getting set to NULL + all the way to update_refs(). update_refs() checks ropts.branch to + decide whether or not to switch brancheds. If ropts.branch is NULL, it + calls update_ref() to update HEAD. At this point however, from rebase's + point of view, we want a detached HEAD. But, since checkout_up_to_date() + does not set the RESET_HEAD_DETACH flag, the update_ref() call will + deference HEAD and update the branch its pointing to, which in the above + example is main. 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 - 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. + if <branch> is not a valid branch. 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/ + Reported-by: Michael McClimon <michael@mcclimon.org> Signed-off-by: John Cai <johncai86@gmail.com> ## builtin/rebase.c ## @@ builtin/rebase.c: 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.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK | RESET_HEAD_DETACH; +- ropts.branch = options->head_name; + ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; ++ if (options->head_name) ++ ropts.branch = options->head_name; ++ else ++ 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); + ## reset.c ## +@@ reset.c: int reset_head(struct repository *r, const struct reset_head_opts *opts) + if (opts->branch_msg && !opts->branch) + BUG("branch reflog message given without a branch"); + ++ if (switch_to_branch && opts->flags & RESET_HEAD_DETACH) ++ BUG("attempting to detach HEAD when branch is given"); ++ + if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) { + ret = -1; + goto leave_reset_head; + ## t/t3400-rebase.sh ## -@@ t/t3400-rebase.sh: test_expect_success 'rebase when inside worktree subdirectory' ' - ) +@@ t/t3400-rebase.sh: test_expect_success 'switch to branch not checked out' ' + git rebase main other ' -+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_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_done + test_expect_success 'refuse to switch to branch checked out elsewhere' ' + git checkout main && + git worktree add wt && -- gitgitgadget ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 1/2] rebase: use test_commit helper in setup 2022-03-11 17:24 ` [PATCH v2 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget @ 2022-03-11 17:24 ` John Cai via GitGitGadget 2022-03-13 7:50 ` Junio C Hamano 2022-03-11 17:24 ` [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget 2022-03-17 3:16 ` [PATCH v3 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 2 siblings, 1 reply; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-11 17:24 UTC (permalink / raw) To: git; +Cc: Phillip Wood, Junio C Hamano, John Cai, John Cai From: John Cai <johncai86@gmail.com> To prepare for the next commit that will test rebase with oids instead of branch names, update the rebase setup test to add a couple of tags we can use. This uses the test_commit helper so we can replace some lines that add a commit manually. Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: John Cai <johncai86@gmail.com> --- t/t3400-rebase.sh | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 71b1735e1dd..0643d015255 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 && -- gitgitgadget ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/2] rebase: use test_commit helper in setup 2022-03-11 17:24 ` [PATCH v2 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget @ 2022-03-13 7:50 ` Junio C Hamano 2022-03-14 10:52 ` Phillip Wood 0 siblings, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2022-03-13 7:50 UTC (permalink / raw) To: John Cai via GitGitGadget; +Cc: git, Phillip Wood, John Cai "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: John Cai <johncai86@gmail.com> > > To prepare for the next commit that will test rebase with oids instead > of branch names, update the rebase setup test to add a couple of tags we > can use. This uses the test_commit helper so we can replace some lines > that add a commit manually. OK. > test_expect_success 'prepare repository with topic branches' ' > - git config core.logAllRefUpdates true && This lossage is not explained. I do not know if we actually make use of the reflog in the tests, though. > - 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 && ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/2] rebase: use test_commit helper in setup 2022-03-13 7:50 ` Junio C Hamano @ 2022-03-14 10:52 ` Phillip Wood 2022-03-14 21:47 ` Junio C Hamano 0 siblings, 1 reply; 41+ messages in thread From: Phillip Wood @ 2022-03-14 10:52 UTC (permalink / raw) To: Junio C Hamano, John Cai via GitGitGadget; +Cc: git, John Cai On 13/03/2022 07:50, Junio C Hamano wrote: > "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: John Cai <johncai86@gmail.com> >> >> To prepare for the next commit that will test rebase with oids instead >> of branch names, update the rebase setup test to add a couple of tags we >> can use. This uses the test_commit helper so we can replace some lines >> that add a commit manually. > > OK. > >> test_expect_success 'prepare repository with topic branches' ' >> - git config core.logAllRefUpdates true && > > This lossage is not explained. I do not know if we actually make > use of the reflog in the tests, though. It is the default these days so we don't need to waste a process setting it here. Best Wishes Phillip ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 1/2] rebase: use test_commit helper in setup 2022-03-14 10:52 ` Phillip Wood @ 2022-03-14 21:47 ` Junio C Hamano 0 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2022-03-14 21:47 UTC (permalink / raw) To: Phillip Wood; +Cc: John Cai via GitGitGadget, git, John Cai Phillip Wood <phillip.wood123@gmail.com> writes: > On 13/03/2022 07:50, Junio C Hamano wrote: >> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> From: John Cai <johncai86@gmail.com> >>> >>> To prepare for the next commit that will test rebase with oids instead >>> of branch names, update the rebase setup test to add a couple of tags we >>> can use. This uses the test_commit helper so we can replace some lines >>> that add a commit manually. >> OK. >> >>> test_expect_success 'prepare repository with topic branches' ' >>> - git config core.logAllRefUpdates true && >> This lossage is not explained. I do not know if we actually make >> use of the reflog in the tests, though. > > It is the default these days so we don't need to waste a process > setting it here. I said "not explained" in the log message. I didn't ask anybody to explain it to me or to readers on the list. Thanks. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-11 17:24 ` [PATCH v2 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 2022-03-11 17:24 ` [PATCH v2 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget @ 2022-03-11 17:24 ` John Cai via GitGitGadget 2022-03-13 7:58 ` Junio C Hamano 2022-03-17 3:16 ` [PATCH v3 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 2 siblings, 1 reply; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-11 17:24 UTC (permalink / raw) To: git; +Cc: Phillip Wood, Junio C Hamano, John Cai, John Cai 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). See [1] for the original bug report. The callstack from checkout_up_to_date() is the following: cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs() -> update_ref() When <branch> is not a valid branch but a sha, rebase sets the head_name of rebase_options to NULL. This value gets passed down this call chain through the branch member of reset_head_opts also getting set to NULL all the way to update_refs(). update_refs() checks ropts.branch to decide whether or not to switch brancheds. If ropts.branch is NULL, it calls update_ref() to update HEAD. At this point however, from rebase's point of view, we want a detached HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH flag, the update_ref() call will deference HEAD and update the branch its pointing to, which in the above example is main. 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 if <branch> is not a valid branch. 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/ Reported-by: Michael McClimon <michael@mcclimon.org> Signed-off-by: John Cai <johncai86@gmail.com> --- builtin/rebase.c | 5 ++++- reset.c | 3 +++ t/t3400-rebase.sh | 9 +++++++++ 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index b29ad2b65e7..5ae7fa2a169 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -827,8 +827,11 @@ 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; + if (options->head_name) + ropts.branch = options->head_name; + else + 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); diff --git a/reset.c b/reset.c index e3383a93343..f8e32fcc240 100644 --- a/reset.c +++ b/reset.c @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts) if (opts->branch_msg && !opts->branch) BUG("branch reflog message given without a branch"); + if (switch_to_branch && opts->flags & RESET_HEAD_DETACH) + BUG("attempting to detach HEAD when branch is given"); + if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) { ret = -1; goto leave_reset_head; diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 0643d015255..d5a8ee39fc4 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -394,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 && -- gitgitgadget ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-11 17:24 ` [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget @ 2022-03-13 7:58 ` Junio C Hamano 2022-03-14 10:54 ` Phillip Wood 2022-03-14 14:11 ` John Cai 0 siblings, 2 replies; 41+ messages in thread From: Junio C Hamano @ 2022-03-13 7:58 UTC (permalink / raw) To: John Cai via GitGitGadget; +Cc: git, Phillip Wood, John Cai "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > diff --git a/reset.c b/reset.c > index e3383a93343..f8e32fcc240 100644 > --- a/reset.c > +++ b/reset.c > @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts) > if (opts->branch_msg && !opts->branch) > BUG("branch reflog message given without a branch"); > > + if (switch_to_branch && opts->flags & RESET_HEAD_DETACH) It's just style thing but it probably is easier to read to have an extra () around the bitwise-&. > + BUG("attempting to detach HEAD when branch is given"); I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH when switch_to_branch == NULL. If there isn't, it could be that we can get rid of RESET_HEAD_DETACH bit and base this decision solely on switch_to_branch'es NULLness. But that is totally outside the scope of this fix. I'd prefer to see a narrowly and sharply focused fix, and to be quite honest, I would be happier if this assertion weren't in this topic. > if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) { > ret = -1; > goto leave_reset_head; > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index 0643d015255..d5a8ee39fc4 100755 > --- a/t/t3400-rebase.sh > +++ b/t/t3400-rebase.sh > @@ -394,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 && Good. For reproduction, the fork-point "First" does not have to be a raw object name. "Second^0" not being a branch name does matter. > + test_cmp_rev HEAD Second && > + test_cmp_rev main $old_main && > + test_must_fail git symbolic-ref HEAD All correct and to the point. Good. Will queue. Thanks. > +' > + > test_expect_success 'refuse to switch to branch checked out elsewhere' ' > git checkout main && > git worktree add wt && ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-13 7:58 ` Junio C Hamano @ 2022-03-14 10:54 ` Phillip Wood 2022-03-14 14:05 ` Phillip Wood 2022-03-14 14:11 ` John Cai 1 sibling, 1 reply; 41+ messages in thread From: Phillip Wood @ 2022-03-14 10:54 UTC (permalink / raw) To: Junio C Hamano, John Cai via GitGitGadget; +Cc: git, John Cai On 13/03/2022 07:58, Junio C Hamano wrote: > "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> diff --git a/reset.c b/reset.c >> index e3383a93343..f8e32fcc240 100644 >> --- a/reset.c >> +++ b/reset.c >> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts) >> if (opts->branch_msg && !opts->branch) >> BUG("branch reflog message given without a branch"); >> >> + if (switch_to_branch && opts->flags & RESET_HEAD_DETACH) > > It's just style thing but it probably is easier to read to have > an extra () around the bitwise-&. > >> + BUG("attempting to detach HEAD when branch is given"); > > I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH > when switch_to_branch == NULL. If there isn't, it could be that > we can get rid of RESET_HEAD_DETACH bit and base this decision > solely on switch_to_branch'es NULLness. "rebase --skip" and "rebase --autostash" are two such uses I think Best Wishes Phillip ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-14 10:54 ` Phillip Wood @ 2022-03-14 14:05 ` Phillip Wood 0 siblings, 0 replies; 41+ messages in thread From: Phillip Wood @ 2022-03-14 14:05 UTC (permalink / raw) To: Junio C Hamano, John Cai via GitGitGadget; +Cc: git, John Cai On 14/03/2022 10:54, Phillip Wood wrote: > On 13/03/2022 07:58, Junio C Hamano wrote: >> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> diff --git a/reset.c b/reset.c >>> index e3383a93343..f8e32fcc240 100644 >>> --- a/reset.c >>> +++ b/reset.c >>> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct >>> reset_head_opts *opts) >>> if (opts->branch_msg && !opts->branch) >>> BUG("branch reflog message given without a branch"); >>> + if (switch_to_branch && opts->flags & RESET_HEAD_DETACH) >> >> It's just style thing but it probably is easier to read to have >> an extra () around the bitwise-&. >> >>> + BUG("attempting to detach HEAD when branch is given"); >> >> I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH >> when switch_to_branch == NULL. If there isn't, it could be that >> we can get rid of RESET_HEAD_DETACH bit and base this decision >> solely on switch_to_branch'es NULLness. > > "rebase --skip" and "rebase --autostash" are two such uses I think Those don't update any refs though so are not helpful examples. Possibly when we fast-forward because HEAD is an ancestor of 'onto' is a potential case but at the moment I think we detach HEAD when checking out 'onto' and then update and checkout the branch if there is one (I've been thinking about fixing that so we only detach HEAD if we're going to rebase). Best Wishes Phillip > Best Wishes > > Phillip ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-13 7:58 ` Junio C Hamano 2022-03-14 10:54 ` Phillip Wood @ 2022-03-14 14:11 ` John Cai 2022-03-14 14:25 ` Phillip Wood 1 sibling, 1 reply; 41+ messages in thread From: John Cai @ 2022-03-14 14:11 UTC (permalink / raw) To: Junio C Hamano; +Cc: John Cai via GitGitGadget, git, Phillip Wood Hi Junio, On 13 Mar 2022, at 3:58, Junio C Hamano wrote: > "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> diff --git a/reset.c b/reset.c >> index e3383a93343..f8e32fcc240 100644 >> --- a/reset.c >> +++ b/reset.c >> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts) >> if (opts->branch_msg && !opts->branch) >> BUG("branch reflog message given without a branch"); >> >> + if (switch_to_branch && opts->flags & RESET_HEAD_DETACH) > > It's just style thing but it probably is easier to read to have > an extra () around the bitwise-&. > >> + BUG("attempting to detach HEAD when branch is given"); > > I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH > when switch_to_branch == NULL. If there isn't, it could be that > we can get rid of RESET_HEAD_DETACH bit and base this decision > solely on switch_to_branch'es NULLness. > > But that is totally outside the scope of this fix. I'd prefer to > see a narrowly and sharply focused fix, and to be quite honest, I > would be happier if this assertion weren't in this topic. I'm okay with taking this out, but would be good to get an ack from Phillip. > >> if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) { >> ret = -1; >> goto leave_reset_head; >> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh >> index 0643d015255..d5a8ee39fc4 100755 >> --- a/t/t3400-rebase.sh >> +++ b/t/t3400-rebase.sh >> @@ -394,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 && > > Good. For reproduction, the fork-point "First" does not have to be > a raw object name. "Second^0" not being a branch name does matter. > >> + test_cmp_rev HEAD Second && >> + test_cmp_rev main $old_main && >> + test_must_fail git symbolic-ref HEAD > > All correct and to the point. Good. Thanks to Phillip for his help on this! > > Will queue. Thanks. More of a question about protocol. If there are some minor adjustments that are not really worth more disussion on the ML (like removing the BUG assertion), is it still protocol to send another series or just re-roll on the branch without sending out another patch series? Thanks! John > >> +' >> + >> test_expect_success 'refuse to switch to branch checked out elsewhere' ' >> git checkout main && >> git worktree add wt && ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-14 14:11 ` John Cai @ 2022-03-14 14:25 ` Phillip Wood 0 siblings, 0 replies; 41+ messages in thread From: Phillip Wood @ 2022-03-14 14:25 UTC (permalink / raw) To: John Cai, Junio C Hamano; +Cc: John Cai via GitGitGadget, git Hi John On 14/03/2022 14:11, John Cai wrote: > Hi Junio, > > On 13 Mar 2022, at 3:58, Junio C Hamano wrote: > >> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: >> >>> diff --git a/reset.c b/reset.c >>> index e3383a93343..f8e32fcc240 100644 >>> --- a/reset.c >>> +++ b/reset.c >>> @@ -101,6 +101,9 @@ int reset_head(struct repository *r, const struct reset_head_opts *opts) >>> if (opts->branch_msg && !opts->branch) >>> BUG("branch reflog message given without a branch"); >>> >>> + if (switch_to_branch && opts->flags & RESET_HEAD_DETACH) >> >> It's just style thing but it probably is easier to read to have >> an extra () around the bitwise-&. >> >>> + BUG("attempting to detach HEAD when branch is given"); >> >> I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH >> when switch_to_branch == NULL. If there isn't, it could be that >> we can get rid of RESET_HEAD_DETACH bit and base this decision >> solely on switch_to_branch'es NULLness. >> >> But that is totally outside the scope of this fix. I'd prefer to >> see a narrowly and sharply focused fix, and to be quite honest, I >> would be happier if this assertion weren't in this topic. > > I'm okay with taking this out, but would be good to get an ack from Phillip. That's fine with me. The rest of the patch looks good as far as I'm concerned. Best Wishes Phillip ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v3 0/2] rebase: update HEAD when is an oid 2022-03-11 17:24 ` [PATCH v2 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 2022-03-11 17:24 ` [PATCH v2 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget 2022-03-11 17:24 ` [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget @ 2022-03-17 3:16 ` John Cai via GitGitGadget 2022-03-17 3:16 ` [PATCH v3 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget ` (2 more replies) 2 siblings, 3 replies; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-17 3:16 UTC (permalink / raw) To: git; +Cc: Phillip Wood, Junio C Hamano, John Cai 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/ This patch has two parts: * updates rebase setup test to add some tags we will use, while swapping out manual commit creation with the test_commit helper * sets RESET_HARD_DETACH flag if branch is not a valid refname changes since v2: * remove BUG assertion changes since v1: * only set RESET_HEAD_DETACH if is not a valid branch * updated tests to use existing setup John Cai (2): rebase: use test_commit helper in setup rebase: set REF_HEAD_DETACH in checkout_up_to_date() builtin/rebase.c | 5 ++++- t/t3400-rebase.sh | 18 +++++++++++------- 2 files changed, 15 insertions(+), 8 deletions(-) base-commit: b896f729e240d250cf56899e6a0073f6aa469f5d Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v3 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v3 Pull-Request: https://github.com/git/git/pull/1226 Range-diff vs v2: 1: f3f084adfa6 ! 1: f658eb00bcd rebase: use test_commit helper in setup @@ Commit message can use. This uses the test_commit helper so we can replace some lines that add a commit manually. + Setting logAllRefUpdates is not necessary because it's on by default for + repositories with a working tree. + Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: John Cai <johncai86@gmail.com> 2: 0e3c73375c1 ! 2: bd1c9537ffc rebase: set REF_HEAD_DETACH in checkout_up_to_date() @@ builtin/rebase.c: static int checkout_up_to_date(struct rebase_options *options) if (reset_head(the_repository, &ropts) < 0) ret = error(_("could not switch to %s"), options->switch_to); - ## reset.c ## -@@ reset.c: int reset_head(struct repository *r, const struct reset_head_opts *opts) - if (opts->branch_msg && !opts->branch) - BUG("branch reflog message given without a branch"); - -+ if (switch_to_branch && opts->flags & RESET_HEAD_DETACH) -+ BUG("attempting to detach HEAD when branch is given"); -+ - if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) { - ret = -1; - goto leave_reset_head; - ## t/t3400-rebase.sh ## @@ t/t3400-rebase.sh: test_expect_success 'switch to branch not checked out' ' git rebase main other -- gitgitgadget ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v3 1/2] rebase: use test_commit helper in setup 2022-03-17 3:16 ` [PATCH v3 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget @ 2022-03-17 3:16 ` John Cai via GitGitGadget 2022-03-17 13:37 ` Ævar Arnfjörð Bjarmason 2022-03-17 3:16 ` [PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget 2022-03-17 19:53 ` [PATCH v4 0/3] rebase: update HEAD when is an oid John Cai via GitGitGadget 2 siblings, 1 reply; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-17 3:16 UTC (permalink / raw) To: git; +Cc: Phillip Wood, Junio C Hamano, John Cai, John Cai From: John Cai <johncai86@gmail.com> To prepare for the next commit that will test rebase with oids instead of branch names, update the rebase setup test to add a couple of tags we can use. This uses the test_commit helper so we can replace some lines that add a commit manually. Setting logAllRefUpdates is not necessary because it's on by default for repositories with a working tree. Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: John Cai <johncai86@gmail.com> --- t/t3400-rebase.sh | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 71b1735e1dd..0643d015255 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 && -- gitgitgadget ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v3 1/2] rebase: use test_commit helper in setup 2022-03-17 3:16 ` [PATCH v3 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget @ 2022-03-17 13:37 ` Ævar Arnfjörð Bjarmason 0 siblings, 0 replies; 41+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-03-17 13:37 UTC (permalink / raw) To: John Cai via GitGitGadget; +Cc: git, Phillip Wood, Junio C Hamano, John Cai On Thu, Mar 17 2022, John Cai via GitGitGadget wrote: > From: John Cai <johncai86@gmail.com> > > To prepare for the next commit that will test rebase with oids instead > of branch names, update the rebase setup test to add a couple of tags we > can use. This uses the test_commit helper so we can replace some lines > that add a commit manually. > > Setting logAllRefUpdates is not necessary because it's on by default for > repositories with a working tree. > > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Signed-off-by: John Cai <johncai86@gmail.com> > --- > t/t3400-rebase.sh | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) > > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index 71b1735e1dd..0643d015255 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 && At least these could also be done, and skimming there's a few more "git commit" that could also be converted: diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 0643d015255..6dc8df8be7e 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -35,9 +35,7 @@ test_expect_success 'prepare repository with topic branches' ' git update-index A && git commit -m "Modify A." && git checkout -b side my-topic-branch && - echo Side >>C && - git add C && - git commit -m "Add C" && + test_commit --no-tag "Add C" C Side && git checkout -f my-topic-branch && git tag topic ' @@ -114,10 +112,7 @@ test_expect_success 'rebase off of the previous branch using "-"' ' test_expect_success 'rebase a single mode change' ' git checkout main && git branch -D topic && - echo 1 >X && - git add X && - test_tick && - git commit -m prepare && + test_commit prepare X 1 && git checkout -b modechange HEAD^ && echo 1 >X && git add X && Maybe worthwhile squashing in & doing the rest? Note that --no-tag and --append allow for converting more than could be in the past before those (relatively recently added) options. ^ permalink raw reply related [flat|nested] 41+ messages in thread
* [PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-17 3:16 ` [PATCH v3 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 2022-03-17 3:16 ` [PATCH v3 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget @ 2022-03-17 3:16 ` John Cai via GitGitGadget 2022-03-17 13:42 ` Ævar Arnfjörð Bjarmason 2022-03-17 19:53 ` [PATCH v4 0/3] rebase: update HEAD when is an oid John Cai via GitGitGadget 2 siblings, 1 reply; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-17 3:16 UTC (permalink / raw) To: git; +Cc: Phillip Wood, Junio C Hamano, John Cai, John Cai 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). See [1] for the original bug report. The callstack from checkout_up_to_date() is the following: cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs() -> update_ref() When <branch> is not a valid branch but a sha, rebase sets the head_name of rebase_options to NULL. This value gets passed down this call chain through the branch member of reset_head_opts also getting set to NULL all the way to update_refs(). update_refs() checks ropts.branch to decide whether or not to switch brancheds. If ropts.branch is NULL, it calls update_ref() to update HEAD. At this point however, from rebase's point of view, we want a detached HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH flag, the update_ref() call will deference HEAD and update the branch its pointing to, which in the above example is main. 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 if <branch> is not a valid branch. 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/ Reported-by: Michael McClimon <michael@mcclimon.org> Signed-off-by: John Cai <johncai86@gmail.com> --- builtin/rebase.c | 5 ++++- t/t3400-rebase.sh | 9 +++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index b29ad2b65e7..5ae7fa2a169 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -827,8 +827,11 @@ 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; + if (options->head_name) + ropts.branch = options->head_name; + else + 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); diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 0643d015255..d5a8ee39fc4 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -394,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 && -- gitgitgadget ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-17 3:16 ` [PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget @ 2022-03-17 13:42 ` Ævar Arnfjörð Bjarmason 2022-03-17 15:34 ` Junio C Hamano 0 siblings, 1 reply; 41+ messages in thread From: Ævar Arnfjörð Bjarmason @ 2022-03-17 13:42 UTC (permalink / raw) To: John Cai via GitGitGadget; +Cc: git, Phillip Wood, Junio C Hamano, John Cai On Thu, Mar 17 2022, 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). See [1] for > the original bug report. > > The callstack from checkout_up_to_date() is the following: > > cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs() > -> update_ref() > > When <branch> is not a valid branch but a sha, rebase sets the head_name ..but an OID... > of rebase_options to NULL. This value gets passed down this call chain > through the branch member of reset_head_opts also getting set to NULL > all the way to update_refs(). update_refs() checks ropts.branch to > decide whether or not to switch brancheds. If ropts.branch is NULL, it brancheds -> branches. And maybe a new paragraph before "update_refs()"? I.e. "\n\nThen update_refs() checks..." ? > calls update_ref() to update HEAD. At this point however, from rebase's > point of view, we want a detached HEAD. But, since checkout_up_to_date() > does not set the RESET_HEAD_DETACH flag, the update_ref() call will > deference HEAD and update the branch its pointing to, which in the above > example is main. > > 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 > if <branch> is not a valid branch. 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. But on the "tell" v.s. show... (more below)... > > Also add a test to ensure this behavior. > > 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/ > > Reported-by: Michael McClimon <michael@mcclimon.org> > Signed-off-by: John Cai <johncai86@gmail.com> > --- > builtin/rebase.c | 5 ++++- > t/t3400-rebase.sh | 9 +++++++++ > 2 files changed, 13 insertions(+), 1 deletion(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index b29ad2b65e7..5ae7fa2a169 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -827,8 +827,11 @@ 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; > + if (options->head_name) > + ropts.branch = options->head_name; > + else > + 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); In this case a smaller change of: if (!ropts.branch) ropts.flags |= RESET_HEAD_DETACH; will do the same. I wonder if just converting it to a designated initializer while we're at it (or a pre-cleanup commit) would be better, i.e.: diff --git a/builtin/rebase.c b/builtin/rebase.c index 5ae7fa2a169..bf4fd4d2920 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -820,18 +820,18 @@ static int rebase_config(const char *var, const char *value, void *data) static int checkout_up_to_date(struct rebase_options *options) { struct strbuf buf = STRBUF_INIT; - struct reset_head_opts ropts = { 0 }; + struct reset_head_opts ropts = { + .oid = &options->orig_head, + .branch = options->head_name, + .flags = (RESET_HEAD_RUN_POST_CHECKOUT_HOOK | + (options->head_name ? 0 : RESET_HEAD_DETACH)), + }; int ret = 0; strbuf_addf(&buf, "%s: checkout %s", getenv(GIT_REFLOG_ACTION_ENVIRONMENT), options->switch_to); - ropts.oid = &options->orig_head; - ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; - if (options->head_name) - ropts.branch = options->head_name; - else - 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); But in any case the functionality will be the same, so this is just bikeshedding. > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index 0643d015255..d5a8ee39fc4 100755 > --- a/t/t3400-rebase.sh > +++ b/t/t3400-rebase.sh > @@ -394,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 && I think it's *much* easier to review these sorts of changes where there's a preceding commit that positively asserts what we do now, and we'll then in the "fix" commit change the behavior. So more "show" v.s. "tell". I.e. in this case do the "test_cmp_rev" to the "wrong" tip with a TODO comment or whatever, then in the fix just adjust it, then it's clear what we had before/after. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-17 13:42 ` Ævar Arnfjörð Bjarmason @ 2022-03-17 15:34 ` Junio C Hamano 0 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2022-03-17 15:34 UTC (permalink / raw) To: Ævar Arnfjörð Bjarmason Cc: John Cai via GitGitGadget, git, Phillip Wood, John Cai Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: >> @@ -827,8 +827,11 @@ 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; >> + if (options->head_name) >> + ropts.branch = options->head_name; >> + else >> + 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); > > In this case a smaller change of: > > if (!ropts.branch) > ropts.flags |= RESET_HEAD_DETACH; > > will do the same. Thanks. That is much easier to read and simpler to follow. > I wonder if just converting it to a designated initializer while we're > at it (or a pre-cleanup commit) would be better, i.e.: I do not think it easier to follow than even the original or the improvement above, especially the part that computes .flags member. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v4 0/3] rebase: update HEAD when is an oid 2022-03-17 3:16 ` [PATCH v3 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 2022-03-17 3:16 ` [PATCH v3 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget 2022-03-17 3:16 ` [PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget @ 2022-03-17 19:53 ` John Cai via GitGitGadget 2022-03-17 19:53 ` [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch John Cai via GitGitGadget ` (3 more replies) 2 siblings, 4 replies; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-17 19:53 UTC (permalink / raw) To: git Cc: Phillip Wood, Junio C Hamano, Ævar Arnfjörð Bjarmason, John Cai 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/ This patch has three parts: * a test to show the incorrect buggy behavior of rebase with a non-branch argument * updates rebase setup test to add some tags we will use, while swapping out manual commit creation with the test_commit helper * sets RESET_HARD_DETACH flag if branch is not a valid refname changes since v3: * fixed typos in commit message * added a test assertion to show bug behavior * included more replacements with test_commit changes since v2: * remove BUG assertion changes since v1: * only set RESET_HEAD_DETACH if is not a valid branch * updated tests to use existing setup John Cai (3): rebase: test showing bug in rebase with non-branch rebase: use test_commit helper in setup rebase: set REF_HEAD_DETACH in checkout_up_to_date() builtin/rebase.c | 2 ++ t/t3400-rebase.sh | 27 +++++++++++++-------------- 2 files changed, 15 insertions(+), 14 deletions(-) base-commit: b896f729e240d250cf56899e6a0073f6aa469f5d Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v4 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v4 Pull-Request: https://github.com/git/git/pull/1226 Range-diff vs v3: -: ----------- > 1: cac51a949ee rebase: test showing bug in rebase with non-branch 1: f658eb00bcd ! 2: 5c40e116eba rebase: use test_commit helper in setup @@ t/t3400-rebase.sh: test_expect_success 'prepare repository with topic branches' git checkout -f main && echo Third >>A && git update-index A && + git commit -m "Modify A." && + git checkout -b side my-topic-branch && +- echo Side >>C && +- git add C && +- git commit -m "Add C" && ++ test_commit --no-tag "Add C" C Side && + git checkout -f my-topic-branch && + git tag topic + ' +@@ t/t3400-rebase.sh: test_expect_success 'rebase off of the previous branch using "-"' ' + test_expect_success 'rebase a single mode change' ' + git checkout main && + git branch -D topic && +- echo 1 >X && +- git add X && +- test_tick && +- git commit -m prepare && ++ test_commit prepare X 1 && + git checkout -b modechange HEAD^ && + echo 1 >X && + git add X && 2: bd1c9537ffc ! 3: 13c5955c317 rebase: set REF_HEAD_DETACH in checkout_up_to_date() @@ Commit message cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs() -> update_ref() - When <branch> is not a valid branch but a sha, rebase sets the head_name + When <branch> is not a valid branch but an oid, rebase sets the head_name of rebase_options to NULL. This value gets passed down this call chain through the branch member of reset_head_opts also getting set to NULL - all the way to update_refs(). update_refs() checks ropts.branch to - decide whether or not to switch brancheds. If ropts.branch is NULL, it - calls update_ref() to update HEAD. At this point however, from rebase's - point of view, we want a detached HEAD. But, since checkout_up_to_date() - does not set the RESET_HEAD_DETACH flag, the update_ref() call will - deference HEAD and update the branch its pointing to, which in the above - example is main. + all the way to update_refs(). + + Then update_refs() checks ropts.branch to decide whether or not to switch + branches. If ropts.branch is NULL, it calls update_ref() to update HEAD. + At this point however, from rebase's point of view, we want a detached + HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH + flag, the update_ref() call will deference HEAD and update the branch its + pointing to, which in the above example is main. The correct behavior is that git rebase should update HEAD to $(git rev-parse topic) without dereferencing it. @@ Commit message HEAD directly intead of deferencing it and updating the branch that HEAD points to. - Also add a test to ensure this behavior. + Also add a test to ensure the correct behavior. 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/ - Reported-by: Michael McClimon <michael@mcclimon.org> Signed-off-by: John Cai <johncai86@gmail.com> ## builtin/rebase.c ## @@ builtin/rebase.c: 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.branch = options->head_name; ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; -+ if (options->head_name) -+ ropts.branch = options->head_name; -+ else ++ if (!ropts.branch) + ropts.flags |= RESET_HEAD_DETACH; ropts.head_msg = buf.buf; if (reset_head(the_repository, &ropts) < 0) @@ t/t3400-rebase.sh: test_expect_success 'switch to branch not checked out' ' git rebase main other ' +-test_expect_success 'switch to non-branch changes branch HEAD points to' ' +test_expect_success 'switch to non-branch detaches HEAD' ' -+ git checkout main && -+ old_main=$(git rev-parse HEAD) && -+ git rebase First Second^0 && + git checkout main && + old_main=$(git rev-parse HEAD) && + git rebase First Second^0 && +- test_cmp_rev HEAD main && +- test_cmp_rev main $(git rev-parse Second) && +- git symbolic-ref HEAD + 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 && -- gitgitgadget ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch 2022-03-17 19:53 ` [PATCH v4 0/3] rebase: update HEAD when is an oid John Cai via GitGitGadget @ 2022-03-17 19:53 ` John Cai via GitGitGadget 2022-03-17 21:10 ` Junio C Hamano 2022-03-17 19:53 ` [PATCH v4 2/3] rebase: use test_commit helper in setup John Cai via GitGitGadget ` (2 subsequent siblings) 3 siblings, 1 reply; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-17 19:53 UTC (permalink / raw) To: git Cc: Phillip Wood, Junio C Hamano, Ævar Arnfjörð Bjarmason, John Cai, John Cai From: John Cai <johncai86@gmail.com> Currently when rebase is used with a non branch, and <oid> is up to date with base: git rebase base <oid> It will update the ref that HEAD is pointing at to <oid>, and leave HEAD unmodified. This is a bug. The expected behavior is that the branch HEAD points at remains unmodified while HEAD is updated to point to <oid> in detached HEAD mode. Signed-off-by: John Cai <johncai86@gmail.com> Reported-by: Michael McClimon <michael@mcclimon.org> --- t/t3400-rebase.sh | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 71b1735e1dd..5c4073f06d6 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -399,6 +399,15 @@ test_expect_success 'switch to branch not checked out' ' git rebase main other ' +test_expect_success 'switch to non-branch changes branch HEAD points to' ' + git checkout main && + old_main=$(git rev-parse HEAD) && + git rebase First Second^0 && + test_cmp_rev HEAD main && + test_cmp_rev main $(git rev-parse Second) && + git symbolic-ref HEAD +' + test_expect_success 'refuse to switch to branch checked out elsewhere' ' git checkout main && git worktree add wt && -- gitgitgadget ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch 2022-03-17 19:53 ` [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch John Cai via GitGitGadget @ 2022-03-17 21:10 ` Junio C Hamano 2022-03-17 21:37 ` Junio C Hamano 2022-03-17 22:44 ` John Cai 0 siblings, 2 replies; 41+ messages in thread From: Junio C Hamano @ 2022-03-17 21:10 UTC (permalink / raw) To: John Cai via GitGitGadget Cc: git, Phillip Wood, Ævar Arnfjörð Bjarmason, John Cai "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: John Cai <johncai86@gmail.com> > > Currently when rebase is used with a non branch, and <oid> is up to > date with base: > > git rebase base <oid> > > It will update the ref that HEAD is pointing at to <oid>, and leave HEAD > unmodified. > > This is a bug. The expected behavior is that the branch HEAD points at > remains unmodified while HEAD is updated to point to <oid> in detached > HEAD mode. Never do tests this way. The primary reason why we do not want to write our tests the way this patch does is because we do not _care_ how it is broken in the behaviour of the original code. 'main' moving out of $old_main is the bug we care about. It is still buggy if it did not move to Second, but some other commit. Yet this patch insists that 'main' to move to Second and nothing else. What we want is 'main' to stay at $old_main in the end anyway, and we should directly test that condition. If you insist to have two commits (which I strongly recommend against in this case), you write a test that makes sure that 'main' stays at $old_main, but mark the test with test_expect_failure. And then later in the step that fixes the code, flip "expect_failure" to "expect_success". But it is not ideal, either. Imagine what you see in "git show" output of the commit that fixed the problem. Most of the test that shows the behaviour that the commit _cares_ about will be outside post-context of the hunk that flips test_expect_failure to test_expect_success. The best and the simplest way, for a simple case like this, to write test is to add the test to expect what we want to see in the end, and do so in the same commit as the one that corrects the behaviour of the code. If somebody wants to see what the breakage looks like, it is easy to (1) checkout the commit that fixes the code and adds such a test, (2) tentatively revert everything outside t/, and (3) run the test with "-i -v" options. Then test_expect_success that wants to see 'main' to stay at $old_main will show that 'main' moved by a test failure. Working from a patch is the same way, i.e. you can apply only the parts inside t/ and run the current code to see the breakage, and then apply the rest to see the fix. > +test_expect_success 'switch to non-branch changes branch HEAD points to' ' > + git checkout main && > + old_main=$(git rev-parse HEAD) && > + git rebase First Second^0 && > + test_cmp_rev HEAD main && > + test_cmp_rev main $(git rev-parse Second) && > + git symbolic-ref HEAD I already said that the second one should expect main to be at $old_main, but the "HEAD and main are the same" and "HEAD is a symolic-ref" test can be replaced with a single test that is "HEAD is a symbolic-ref to 'main'", which would be more strict. I.e. test "$(git symbolic-ref HEAD)" = refs/heads/main && test_cmp_rev main "$old_main" And such a test that expects the correct behaviour we want to have in the end should be added in [PATCH 3/3] when the code is fixed, not here in a separate commit. > +' Thanks. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch 2022-03-17 21:10 ` Junio C Hamano @ 2022-03-17 21:37 ` Junio C Hamano 2022-03-17 22:44 ` John Cai 1 sibling, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2022-03-17 21:37 UTC (permalink / raw) To: John Cai via GitGitGadget Cc: git, Phillip Wood, Ævar Arnfjörð Bjarmason, John Cai Junio C Hamano <gitster@pobox.com> writes: >> + test_cmp_rev HEAD main && >> + test_cmp_rev main $(git rev-parse Second) && >> + git symbolic-ref HEAD > > I already said that the second one should expect main to be at > $old_main, but the "HEAD and main are the same" and "HEAD is a > symolic-ref" test can be replaced with a single test that is "HEAD > is a symbolic-ref to 'main'", which would be more strict. I.e. > > test "$(git symbolic-ref HEAD)" = refs/heads/main && > test_cmp_rev main "$old_main" Scratch that. No, we do not want HEAD to be pointing at 'main' after rebase, so the above is totally wrong. What you have at the end of the series is the right version, as I said in my review of the [PATCH 3/3]. Thanks for working on this topic. ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch 2022-03-17 21:10 ` Junio C Hamano 2022-03-17 21:37 ` Junio C Hamano @ 2022-03-17 22:44 ` John Cai 1 sibling, 0 replies; 41+ messages in thread From: John Cai @ 2022-03-17 22:44 UTC (permalink / raw) To: Junio C Hamano Cc: John Cai via GitGitGadget, git, Phillip Wood, Ævar Arnfjörð Bjarmason Hi Junio, On 17 Mar 2022, at 17:10, Junio C Hamano wrote: > "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: John Cai <johncai86@gmail.com> >> >> Currently when rebase is used with a non branch, and <oid> is up to >> date with base: >> >> git rebase base <oid> >> >> It will update the ref that HEAD is pointing at to <oid>, and leave HEAD >> unmodified. >> >> This is a bug. The expected behavior is that the branch HEAD points at >> remains unmodified while HEAD is updated to point to <oid> in detached >> HEAD mode. > > Never do tests this way. > > The primary reason why we do not want to write our tests the way > this patch does is because we do not _care_ how it is broken in the > behaviour of the original code. 'main' moving out of $old_main is > the bug we care about. It is still buggy if it did not move to > Second, but some other commit. Yet this patch insists that 'main' > to move to Second and nothing else. What we want is 'main' to stay > at $old_main in the end anyway, and we should directly test that > condition. I was attemping to follow the advice to "show" vs "tell" in [1]. All this make sense to me however. > > If you insist to have two commits (which I strongly recommend > against in this case), you write a test that makes sure that 'main' > stays at $old_main, but mark the test with test_expect_failure. And > then later in the step that fixes the code, flip "expect_failure" to > "expect_success". > > But it is not ideal, either. Imagine what you see in "git show" > output of the commit that fixed the problem. Most of the test that > shows the behaviour that the commit _cares_ about will be outside > post-context of the hunk that flips test_expect_failure to > test_expect_success. > > The best and the simplest way, for a simple case like this, to write > test is to add the test to expect what we want to see in the end, > and do so in the same commit as the one that corrects the behaviour > of the code. If somebody wants to see what the breakage looks like, > it is easy to > > (1) checkout the commit that fixes the code and adds such a test, > > (2) tentatively revert everything outside t/, and > > (3) run the test with "-i -v" options. > > Then test_expect_success that wants to see 'main' to stay at > $old_main will show that 'main' moved by a test failure. Working > from a patch is the same way, i.e. you can apply only the parts > inside t/ and run the current code to see the breakage, and then > apply the rest to see the fix. > >> +test_expect_success 'switch to non-branch changes branch HEAD points to' ' >> + git checkout main && >> + old_main=$(git rev-parse HEAD) && >> + git rebase First Second^0 && > >> + test_cmp_rev HEAD main && >> + test_cmp_rev main $(git rev-parse Second) && >> + git symbolic-ref HEAD > > I already said that the second one should expect main to be at > $old_main, but the "HEAD and main are the same" and "HEAD is a > symolic-ref" test can be replaced with a single test that is "HEAD > is a symbolic-ref to 'main'", which would be more strict. I.e. > > test "$(git symbolic-ref HEAD)" = refs/heads/main && > test_cmp_rev main "$old_main" > > And such a test that expects the correct behaviour we want to have > in the end should be added in [PATCH 3/3] when the code is fixed, > not here in a separate commit. 1. https://lore.kernel.org/git/220317.86r170d6zs.gmgdl@evledraar.gmail.com/ > >> +' > > Thanks. ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v4 2/3] rebase: use test_commit helper in setup 2022-03-17 19:53 ` [PATCH v4 0/3] rebase: update HEAD when is an oid John Cai via GitGitGadget 2022-03-17 19:53 ` [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch John Cai via GitGitGadget @ 2022-03-17 19:53 ` John Cai via GitGitGadget 2022-03-18 11:14 ` Phillip Wood 2022-03-17 19:53 ` [PATCH v4 3/3] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget 2022-03-18 13:54 ` [PATCH v5 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 3 siblings, 1 reply; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-17 19:53 UTC (permalink / raw) To: git Cc: Phillip Wood, Junio C Hamano, Ævar Arnfjörð Bjarmason, John Cai, John Cai From: John Cai <johncai86@gmail.com> To prepare for the next commit that will test rebase with oids instead of branch names, update the rebase setup test to add a couple of tags we can use. This uses the test_commit helper so we can replace some lines that add a commit manually. Setting logAllRefUpdates is not necessary because it's on by default for repositories with a working tree. Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: John Cai <johncai86@gmail.com> --- t/t3400-rebase.sh | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 5c4073f06d6..2fb3fabe60e 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,17 +29,13 @@ 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 && git commit -m "Modify A." && git checkout -b side my-topic-branch && - echo Side >>C && - git add C && - git commit -m "Add C" && + test_commit --no-tag "Add C" C Side && git checkout -f my-topic-branch && git tag topic ' @@ -119,10 +112,7 @@ test_expect_success 'rebase off of the previous branch using "-"' ' test_expect_success 'rebase a single mode change' ' git checkout main && git branch -D topic && - echo 1 >X && - git add X && - test_tick && - git commit -m prepare && + test_commit prepare X 1 && git checkout -b modechange HEAD^ && echo 1 >X && git add X && -- gitgitgadget ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v4 2/3] rebase: use test_commit helper in setup 2022-03-17 19:53 ` [PATCH v4 2/3] rebase: use test_commit helper in setup John Cai via GitGitGadget @ 2022-03-18 11:14 ` Phillip Wood 2022-03-18 13:06 ` John Cai 0 siblings, 1 reply; 41+ messages in thread From: Phillip Wood @ 2022-03-18 11:14 UTC (permalink / raw) To: John Cai via GitGitGadget, git Cc: Junio C Hamano, Ævar Arnfjörð Bjarmason, John Cai Hi John On 17/03/2022 19:53, John Cai via GitGitGadget wrote: > From: John Cai <johncai86@gmail.com> > > To prepare for the next commit that will test rebase with oids instead > of branch names, update the rebase setup test to add a couple of tags we > can use. This uses the test_commit helper so we can replace some lines > that add a commit manually. > > Setting logAllRefUpdates is not necessary because it's on by default for > repositories with a working tree. > > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Signed-off-by: John Cai <johncai86@gmail.com> > --- > t/t3400-rebase.sh | 18 ++++-------------- > 1 file changed, 4 insertions(+), 14 deletions(-) > > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index 5c4073f06d6..2fb3fabe60e 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,17 +29,13 @@ 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 && > git commit -m "Modify A." && > git checkout -b side my-topic-branch && 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. Best Wishes Phillip > - echo Side >>C && > - git add C && > - git commit -m "Add C" && > + test_commit --no-tag "Add C" C Side && > git checkout -f my-topic-branch && > git tag topic > ' > @@ -119,10 +112,7 @@ test_expect_success 'rebase off of the previous branch using "-"' ' > test_expect_success 'rebase a single mode change' ' > git checkout main && > git branch -D topic && > - echo 1 >X && > - git add X && > - test_tick && > - git commit -m prepare && > + test_commit prepare X 1 && > git checkout -b modechange HEAD^ && > echo 1 >X && > git add X && ^ permalink raw reply [flat|nested] 41+ messages in thread
* Re: [PATCH v4 2/3] rebase: use test_commit helper in setup 2022-03-18 11:14 ` Phillip Wood @ 2022-03-18 13:06 ` John Cai 0 siblings, 0 replies; 41+ messages in thread From: John Cai @ 2022-03-18 13:06 UTC (permalink / raw) To: phillip.wood Cc: John Cai via GitGitGadget, git, Junio C Hamano, Ævar Arnfjörð Bjarmason Hi Phillip, On 18 Mar 2022, at 7:14, Phillip Wood wrote: > Hi John > > On 17/03/2022 19:53, John Cai via GitGitGadget wrote: >> From: John Cai <johncai86@gmail.com> >> >> To prepare for the next commit that will test rebase with oids instead >> of branch names, update the rebase setup test to add a couple of tags we >> can use. This uses the test_commit helper so we can replace some lines >> that add a commit manually. >> >> Setting logAllRefUpdates is not necessary because it's on by default for >> repositories with a working tree. >> >> Helped-by: Phillip Wood <phillip.wood123@gmail.com> >> Signed-off-by: John Cai <johncai86@gmail.com> >> --- >> t/t3400-rebase.sh | 18 ++++-------------- >> 1 file changed, 4 insertions(+), 14 deletions(-) >> >> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh >> index 5c4073f06d6..2fb3fabe60e 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,17 +29,13 @@ 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 && >> git commit -m "Modify A." && >> git checkout -b side my-topic-branch && > > 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. I see your point. Then for the sake of a smaller series for the patch, I'll opt to only update the parts we need. Then maybe we can have a separate series to modernize this suite. I'll re-roll this series with the clarifications to the commit message that Junio made. > > Best Wishes > > Phillip Thanks! John > >> - echo Side >>C && >> - git add C && >> - git commit -m "Add C" && >> + test_commit --no-tag "Add C" C Side && >> git checkout -f my-topic-branch && >> git tag topic >> ' >> @@ -119,10 +112,7 @@ test_expect_success 'rebase off of the previous branch using "-"' ' >> test_expect_success 'rebase a single mode change' ' >> git checkout main && >> git branch -D topic && >> - echo 1 >X && >> - git add X && >> - test_tick && >> - git commit -m prepare && >> + test_commit prepare X 1 && >> git checkout -b modechange HEAD^ && >> echo 1 >X && >> git add X && ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v4 3/3] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-17 19:53 ` [PATCH v4 0/3] rebase: update HEAD when is an oid John Cai via GitGitGadget 2022-03-17 19:53 ` [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch John Cai via GitGitGadget 2022-03-17 19:53 ` [PATCH v4 2/3] rebase: use test_commit helper in setup John Cai via GitGitGadget @ 2022-03-17 19:53 ` John Cai via GitGitGadget 2022-03-17 21:36 ` Junio C Hamano 2022-03-18 13:54 ` [PATCH v5 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 3 siblings, 1 reply; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-17 19:53 UTC (permalink / raw) To: git Cc: Phillip Wood, Junio C Hamano, Ævar Arnfjörð Bjarmason, John Cai, John Cai 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). See [1] for the original bug report. The callstack from checkout_up_to_date() is the following: cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs() -> update_ref() When <branch> is not a valid branch but an oid, rebase sets the head_name of rebase_options to NULL. This value gets passed down this call chain through the branch member of reset_head_opts also getting set to NULL all the way to update_refs(). Then update_refs() checks ropts.branch to decide whether or not to switch branches. If ropts.branch is NULL, it calls update_ref() to update HEAD. At this point however, from rebase's point of view, we want a detached HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH flag, the update_ref() call will deference HEAD and update the branch its pointing to, which in the above example is main. 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 if <branch> is not a valid branch. 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 the correct behavior. 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/ Signed-off-by: John Cai <johncai86@gmail.com> --- builtin/rebase.c | 2 ++ t/t3400-rebase.sh | 8 ++++---- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index b29ad2b65e7..27fde7bf281 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options) ropts.oid = &options->orig_head; ropts.branch = options->head_name; ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; + 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); diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 2fb3fabe60e..cf55b017ffc 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -389,13 +389,13 @@ test_expect_success 'switch to branch not checked out' ' git rebase main other ' -test_expect_success 'switch to non-branch changes branch HEAD points to' ' +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 main && - test_cmp_rev main $(git rev-parse Second) && - git symbolic-ref HEAD + 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' ' -- gitgitgadget ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v4 3/3] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-17 19:53 ` [PATCH v4 3/3] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget @ 2022-03-17 21:36 ` Junio C Hamano 2022-03-18 0:30 ` John Cai 0 siblings, 1 reply; 41+ messages in thread From: Junio C Hamano @ 2022-03-17 21:36 UTC (permalink / raw) To: John Cai via GitGitGadget Cc: git, Phillip Wood, Ævar Arnfjörð Bjarmason, John Cai "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: John Cai <johncai86@gmail.com> > > Fixes a bug whereby rebase updates the deferenced reference HEAD points > to instead of HEAD directly. Perhaps "git rebase A B", where B is not a commit, should behave as if the HEAD got detached at B and then the detached HEAD got rebased on top of A. A bug however overwrites the current branch to point at B, when B is a descendant of A (i.e. the rebase ends up being a fast-forward). > ... See [1] for > the original bug report. OK (URL is wrong; see below). The explanation of how the bug occurs (elided) in the patch looked all reasonable. It read well. > ... > Also add a test to ensure the correct behavior. Yup. _Add_ a test to ensure that. Not replace a misleading test that expected to see a wrong behaviour. > 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/ This is not the original bug report. It was an early hint for diagnosis. [1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/ would be a more appropriate pointer. > ropts.oid = &options->orig_head; > ropts.branch = options->head_name; > ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; > + if (!ropts.branch) > + ropts.flags |= RESET_HEAD_DETACH; > ropts.head_msg = buf.buf; OK. If head_name is not set, we do not want to touch the branch the HEAD happens to be pointing at, so we want to detach. > +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 main && > - test_cmp_rev main $(git rev-parse Second) && > - git symbolic-ref HEAD > + test_cmp_rev HEAD Second && > + test_cmp_rev main $old_main && > + test_must_fail git symbolic-ref HEAD As we want (1) HEAD (detached) is pointing at Second, (2) 'main' stayed at $old_main, and (3) HEAD is detched, these three conditions look sane. Thanks. For reference, I discarded [1/3], queued [2/3] and replaced this [3/3] with the following for now. ---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ---- From: John Cai <johncai86@gmail.com> Subject: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() "git rebase A B" where B is not a commit should behave as if the HEAD got detached at B and then the detached HEAD got rebased on top of A. A bug however overwrites the current branch to point at B, when B is a descendant of A (i.e. the rebase ends up being a fast-forward). See [1] for the original bug report. The callstack from checkout_up_to_date() is the following: cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs() -> update_ref() When B is not a valid branch but an oid, rebase sets the head_name of rebase_options to NULL. This value gets passed down this call chain through the branch member of reset_head_opts also getting set to NULL all the way to update_refs(). Then update_refs() checks ropts.branch to decide whether or not to switch branches. If ropts.branch is NULL, it calls update_ref() to update HEAD. At this point however, from rebase's point of view, we want a detached HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH flag, the update_ref() call will deference HEAD and update the branch its pointing to. We want the HEAD detached at B instead. Fix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date if B is not a valid branch, 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 the correct behavior. [1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/ Reported-by: Michael McClimon <michael@mcclimon.org> Signed-off-by: John Cai <johncai86@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/rebase.c | 2 ++ t/t3400-rebase.sh | 9 +++++++++ 2 files changed, 11 insertions(+) diff --git a/builtin/rebase.c b/builtin/rebase.c index b29ad2b65e..27fde7bf28 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options) ropts.oid = &options->orig_head; ropts.branch = options->head_name; ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; + 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); diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 6dc8df8be7..cf55b017ff 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -389,6 +389,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 && -- 2.35.1-757-g4266a5c05c ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v4 3/3] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-17 21:36 ` Junio C Hamano @ 2022-03-18 0:30 ` John Cai 0 siblings, 0 replies; 41+ messages in thread From: John Cai @ 2022-03-18 0:30 UTC (permalink / raw) To: Junio C Hamano Cc: John Cai via GitGitGadget, git, Phillip Wood, Ævar Arnfjörð Bjarmason Hi Junio, On 17 Mar 2022, at 17:36, Junio C Hamano wrote: > "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: John Cai <johncai86@gmail.com> >> >> Fixes a bug whereby rebase updates the deferenced reference HEAD points >> to instead of HEAD directly. > > Perhaps > > "git rebase A B", where B is not a commit, should behave as if > the HEAD got detached at B and then the detached HEAD got > rebased on top of A. A bug however overwrites the current > branch to point at B, when B is a descendant of A (i.e. the > rebase ends up being a fast-forward). > >> ... See [1] for >> the original bug report. > > OK (URL is wrong; see below). > > The explanation of how the bug occurs (elided) in the patch looked > all reasonable. It read well. > >> ... >> Also add a test to ensure the correct behavior. > > Yup. _Add_ a test to ensure that. Not replace a misleading test > that expected to see a wrong behaviour. > >> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/ > > This is not the original bug report. It was an early hint for > diagnosis. > > [1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/ > > would be a more appropriate pointer. > >> ropts.oid = &options->orig_head; >> ropts.branch = options->head_name; >> ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; >> + if (!ropts.branch) >> + ropts.flags |= RESET_HEAD_DETACH; >> ropts.head_msg = buf.buf; > > OK. If head_name is not set, we do not want to touch the branch > the HEAD happens to be pointing at, so we want to detach. > >> +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 main && >> - test_cmp_rev main $(git rev-parse Second) && >> - git symbolic-ref HEAD >> + test_cmp_rev HEAD Second && >> + test_cmp_rev main $old_main && >> + test_must_fail git symbolic-ref HEAD > > As we want (1) HEAD (detached) is pointing at Second, (2) 'main' > stayed at $old_main, and (3) HEAD is detched, these three conditions > look sane. > > Thanks. > > For reference, I discarded [1/3], queued [2/3] and replaced this > [3/3] with the following for now. Sounds good--this is what I was about to do anyways! > > ---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ---- > From: John Cai <johncai86@gmail.com> > Subject: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() > > "git rebase A B" where B is not a commit should behave as if the > HEAD got detached at B and then the detached HEAD got rebased on top > of A. A bug however overwrites the current branch to point at B, > when B is a descendant of A (i.e. the rebase ends up being a > fast-forward). See [1] for the original bug report. > > The callstack from checkout_up_to_date() is the following: > > cmd_rebase() > -> checkout_up_to_date() > -> reset_head() > -> update_refs() > -> update_ref() > > When B is not a valid branch but an oid, rebase sets the head_name > of rebase_options to NULL. This value gets passed down this call > chain through the branch member of reset_head_opts also getting set > to NULL all the way to update_refs(). > > Then update_refs() checks ropts.branch to decide whether or not to switch > branches. If ropts.branch is NULL, it calls update_ref() to update HEAD. > At this point however, from rebase's point of view, we want a detached > HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH > flag, the update_ref() call will deference HEAD and update the branch its > pointing to. We want the HEAD detached at B instead. > > Fix this bug by adding the RESET_HEAD_DETACH flag in > checkout_up_to_date if B is not a valid branch, 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 the correct behavior. > > [1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/ > > Reported-by: Michael McClimon <michael@mcclimon.org> > Signed-off-by: John Cai <johncai86@gmail.com> > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > builtin/rebase.c | 2 ++ > t/t3400-rebase.sh | 9 +++++++++ > 2 files changed, 11 insertions(+) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index b29ad2b65e..27fde7bf28 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options) > ropts.oid = &options->orig_head; > ropts.branch = options->head_name; > ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; > + 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); > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index 6dc8df8be7..cf55b017ff 100755 > --- a/t/t3400-rebase.sh > +++ b/t/t3400-rebase.sh > @@ -389,6 +389,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 && > -- > 2.35.1-757-g4266a5c05c Thanks John ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v5 0/2] rebase: update HEAD when is an oid 2022-03-17 19:53 ` [PATCH v4 0/3] rebase: update HEAD when is an oid John Cai via GitGitGadget ` (2 preceding siblings ...) 2022-03-17 19:53 ` [PATCH v4 3/3] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget @ 2022-03-18 13:54 ` John Cai via GitGitGadget 2022-03-18 13:54 ` [PATCH v5 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget ` (2 more replies) 3 siblings, 3 replies; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-18 13:54 UTC (permalink / raw) To: git Cc: Phillip Wood, Junio C Hamano, Ævar Arnfjörð Bjarmason, Michael McClimon, John Cai 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/ This patch has two parts: * updates rebase setup test to add some tags we will use, while swapping out manual commit creation with the test_commit helper * sets RESET_HARD_DETACH flag if branch is not a valid refname changes since v4: * got rid of test assertion that shows bug behavior * clarified commit message changes since v3: * fixed typos in commit message * added a test assertion to show bug behavior * included more replacements with test_commit changes since v2: * remove BUG assertion changes since v1: * only set RESET_HEAD_DETACH if is not a valid branch * updated tests to use existing setup John Cai (2): rebase: use test_commit helper in setup rebase: set REF_HEAD_DETACH in checkout_up_to_date() builtin/rebase.c | 2 ++ t/t3400-rebase.sh | 18 +++++++++++------- 2 files changed, 13 insertions(+), 7 deletions(-) base-commit: b896f729e240d250cf56899e6a0073f6aa469f5d Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v5 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v5 Pull-Request: https://github.com/git/git/pull/1226 Range-diff vs v4: 1: cac51a949ee < -: ----------- rebase: test showing bug in rebase with non-branch 2: 5c40e116eba ! 1: 9dbd2ba430a rebase: use test_commit helper in setup @@ t/t3400-rebase.sh: test_expect_success 'prepare repository with topic branches' git checkout -f main && echo Third >>A && git update-index A && - git commit -m "Modify A." && - git checkout -b side my-topic-branch && -- echo Side >>C && -- git add C && -- git commit -m "Add C" && -+ test_commit --no-tag "Add C" C Side && - git checkout -f my-topic-branch && - git tag topic - ' -@@ t/t3400-rebase.sh: test_expect_success 'rebase off of the previous branch using "-"' ' - test_expect_success 'rebase a single mode change' ' - git checkout main && - git branch -D topic && -- echo 1 >X && -- git add X && -- test_tick && -- git commit -m prepare && -+ test_commit prepare X 1 && - git checkout -b modechange HEAD^ && - echo 1 >X && - git add X && 3: 13c5955c317 ! 2: 1dd5bb21012 rebase: set REF_HEAD_DETACH in checkout_up_to_date() @@ Metadata ## Commit message ## rebase: set REF_HEAD_DETACH in checkout_up_to_date() - 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). See [1] for - the original bug report. + "git rebase A B" where B is not a commit should behave as if the + HEAD got detached at B and then the detached HEAD got rebased on top + of A. A bug however overwrites the current branch to point at B, + when B is a descendant of A (i.e. the rebase ends up being a + fast-forward). See [1] for the original bug report. The callstack from checkout_up_to_date() is the following: - cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs() - -> update_ref() + cmd_rebase() + -> checkout_up_to_date() + -> reset_head() + -> update_refs() + -> update_ref() - When <branch> is not a valid branch but an oid, rebase sets the head_name - of rebase_options to NULL. This value gets passed down this call chain - through the branch member of reset_head_opts also getting set to NULL - all the way to update_refs(). + When B is not a valid branch but an oid, rebase sets the head_name + of rebase_options to NULL. This value gets passed down this call + chain through the branch member of reset_head_opts also getting set + to NULL all the way to update_refs(). Then update_refs() checks ropts.branch to decide whether or not to switch branches. If ropts.branch is NULL, it calls update_ref() to update HEAD. At this point however, from rebase's point of view, we want a detached HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH flag, the update_ref() call will deference HEAD and update the branch its - pointing to, which in the above example is main. - - The correct behavior is that git rebase should update HEAD to $(git - rev-parse topic) without dereferencing it. + pointing to. We want the HEAD detached at B instead. - Fix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date - if <branch> is not a valid branch. 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. + Fix this bug by adding the RESET_HEAD_DETACH flag in + checkout_up_to_date if B is not a valid branch, 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 the correct behavior. - 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/ + [1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/ + Reported-by: Michael McClimon <michael@mcclimon.org> Signed-off-by: John Cai <johncai86@gmail.com> + Signed-off-by: Junio C Hamano <gitster@pobox.com> ## builtin/rebase.c ## @@ builtin/rebase.c: static int checkout_up_to_date(struct rebase_options *options) @@ t/t3400-rebase.sh: test_expect_success 'switch to branch not checked out' ' git rebase main other ' --test_expect_success 'switch to non-branch changes branch HEAD points to' ' +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 main && -- test_cmp_rev main $(git rev-parse Second) && -- git symbolic-ref 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 && -- gitgitgadget ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v5 1/2] rebase: use test_commit helper in setup 2022-03-18 13:54 ` [PATCH v5 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget @ 2022-03-18 13:54 ` John Cai via GitGitGadget 2022-03-18 16:51 ` Junio C Hamano 2022-03-18 13:54 ` [PATCH v5 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget 2022-03-18 16:55 ` [PATCH v5 0/2] rebase: update HEAD when is an oid Junio C Hamano 2 siblings, 1 reply; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-18 13:54 UTC (permalink / raw) To: git Cc: Phillip Wood, Junio C Hamano, Ævar Arnfjörð Bjarmason, Michael McClimon, John Cai, John Cai From: John Cai <johncai86@gmail.com> To prepare for the next commit that will test rebase with oids instead of branch names, update the rebase setup test to add a couple of tags we can use. This uses the test_commit helper so we can replace some lines that add a commit manually. Setting logAllRefUpdates is not necessary because it's on by default for repositories with a working tree. Helped-by: Phillip Wood <phillip.wood123@gmail.com> Signed-off-by: John Cai <johncai86@gmail.com> --- t/t3400-rebase.sh | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 71b1735e1dd..0643d015255 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 && -- gitgitgadget ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v5 1/2] rebase: use test_commit helper in setup 2022-03-18 13:54 ` [PATCH v5 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget @ 2022-03-18 16:51 ` Junio C Hamano 0 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2022-03-18 16:51 UTC (permalink / raw) To: John Cai via GitGitGadget Cc: git, Phillip Wood, Ævar Arnfjörð Bjarmason, Michael McClimon, John Cai "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: John Cai <johncai86@gmail.com> > > To prepare for the next commit that will test rebase with oids instead > of branch names, update the rebase setup test to add a couple of tags we > can use. This uses the test_commit helper so we can replace some lines > that add a commit manually. > > Setting logAllRefUpdates is not necessary because it's on by default for > repositories with a working tree. > > Helped-by: Phillip Wood <phillip.wood123@gmail.com> > Signed-off-by: John Cai <johncai86@gmail.com> > --- > t/t3400-rebase.sh | 9 ++------- > 1 file changed, 2 insertions(+), 7 deletions(-) Looks much simpler and obviously correct ;-) > diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh > index 71b1735e1dd..0643d015255 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 && ^ permalink raw reply [flat|nested] 41+ messages in thread
* [PATCH v5 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() 2022-03-18 13:54 ` [PATCH v5 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 2022-03-18 13:54 ` [PATCH v5 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget @ 2022-03-18 13:54 ` John Cai via GitGitGadget 2022-03-18 16:55 ` [PATCH v5 0/2] rebase: update HEAD when is an oid Junio C Hamano 2 siblings, 0 replies; 41+ messages in thread From: John Cai via GitGitGadget @ 2022-03-18 13:54 UTC (permalink / raw) To: git Cc: Phillip Wood, Junio C Hamano, Ævar Arnfjörð Bjarmason, Michael McClimon, John Cai, John Cai From: John Cai <johncai86@gmail.com> "git rebase A B" where B is not a commit should behave as if the HEAD got detached at B and then the detached HEAD got rebased on top of A. A bug however overwrites the current branch to point at B, when B is a descendant of A (i.e. the rebase ends up being a fast-forward). See [1] for the original bug report. The callstack from checkout_up_to_date() is the following: cmd_rebase() -> checkout_up_to_date() -> reset_head() -> update_refs() -> update_ref() When B is not a valid branch but an oid, rebase sets the head_name of rebase_options to NULL. This value gets passed down this call chain through the branch member of reset_head_opts also getting set to NULL all the way to update_refs(). Then update_refs() checks ropts.branch to decide whether or not to switch branches. If ropts.branch is NULL, it calls update_ref() to update HEAD. At this point however, from rebase's point of view, we want a detached HEAD. But, since checkout_up_to_date() does not set the RESET_HEAD_DETACH flag, the update_ref() call will deference HEAD and update the branch its pointing to. We want the HEAD detached at B instead. Fix this bug by adding the RESET_HEAD_DETACH flag in checkout_up_to_date if B is not a valid branch, 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 the correct behavior. [1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/ Reported-by: Michael McClimon <michael@mcclimon.org> Signed-off-by: John Cai <johncai86@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com> --- builtin/rebase.c | 2 ++ t/t3400-rebase.sh | 9 +++++++++ 2 files changed, 11 insertions(+) diff --git a/builtin/rebase.c b/builtin/rebase.c index b29ad2b65e7..27fde7bf281 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options) ropts.oid = &options->orig_head; ropts.branch = options->head_name; ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK; + 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); diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh index 0643d015255..d5a8ee39fc4 100755 --- a/t/t3400-rebase.sh +++ b/t/t3400-rebase.sh @@ -394,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 && -- gitgitgadget ^ permalink raw reply related [flat|nested] 41+ messages in thread
* Re: [PATCH v5 0/2] rebase: update HEAD when is an oid 2022-03-18 13:54 ` [PATCH v5 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 2022-03-18 13:54 ` [PATCH v5 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget 2022-03-18 13:54 ` [PATCH v5 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget @ 2022-03-18 16:55 ` Junio C Hamano 2 siblings, 0 replies; 41+ messages in thread From: Junio C Hamano @ 2022-03-18 16:55 UTC (permalink / raw) To: John Cai via GitGitGadget Cc: git, Phillip Wood, Ævar Arnfjörð Bjarmason, Michael McClimon, John Cai "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes: > Fixes a bug [1] reported by Michael McClimon where rebase with oids will > erroneously update the branch HEAD points to. Looking good. I am tempted to say that we should declare victory. Thanks for working on this fix. ^ permalink raw reply [flat|nested] 41+ messages in thread
end of thread, other threads:[~2022-03-18 16:55 UTC | newest] Thread overview: 41+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-11 5:05 [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget 2022-03-11 5:33 ` Junio C Hamano 2022-03-11 5:55 ` Junio C Hamano 2022-03-11 14:14 ` John Cai 2022-03-11 15:05 ` Phillip Wood 2022-03-11 15:28 ` John Cai 2022-03-11 19:42 ` John Cai 2022-03-11 21:31 ` Phillip Wood 2022-03-11 17:24 ` [PATCH v2 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 2022-03-11 17:24 ` [PATCH v2 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget 2022-03-13 7:50 ` Junio C Hamano 2022-03-14 10:52 ` Phillip Wood 2022-03-14 21:47 ` Junio C Hamano 2022-03-11 17:24 ` [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget 2022-03-13 7:58 ` Junio C Hamano 2022-03-14 10:54 ` Phillip Wood 2022-03-14 14:05 ` Phillip Wood 2022-03-14 14:11 ` John Cai 2022-03-14 14:25 ` Phillip Wood 2022-03-17 3:16 ` [PATCH v3 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 2022-03-17 3:16 ` [PATCH v3 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget 2022-03-17 13:37 ` Ævar Arnfjörð Bjarmason 2022-03-17 3:16 ` [PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget 2022-03-17 13:42 ` Ævar Arnfjörð Bjarmason 2022-03-17 15:34 ` Junio C Hamano 2022-03-17 19:53 ` [PATCH v4 0/3] rebase: update HEAD when is an oid John Cai via GitGitGadget 2022-03-17 19:53 ` [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch John Cai via GitGitGadget 2022-03-17 21:10 ` Junio C Hamano 2022-03-17 21:37 ` Junio C Hamano 2022-03-17 22:44 ` John Cai 2022-03-17 19:53 ` [PATCH v4 2/3] rebase: use test_commit helper in setup John Cai via GitGitGadget 2022-03-18 11:14 ` Phillip Wood 2022-03-18 13:06 ` John Cai 2022-03-17 19:53 ` [PATCH v4 3/3] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget 2022-03-17 21:36 ` Junio C Hamano 2022-03-18 0:30 ` John Cai 2022-03-18 13:54 ` [PATCH v5 0/2] rebase: update HEAD when is an oid John Cai via GitGitGadget 2022-03-18 13:54 ` [PATCH v5 1/2] rebase: use test_commit helper in setup John Cai via GitGitGadget 2022-03-18 16:51 ` Junio C Hamano 2022-03-18 13:54 ` [PATCH v5 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date() John Cai via GitGitGadget 2022-03-18 16:55 ` [PATCH v5 0/2] rebase: update HEAD when is an oid Junio C Hamano
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).