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@xxxxxxxxxxxx> Signed-off-by: John Cai <johncai86@xxxxxxxxx> ## 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
