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@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.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
