[v2,0/2] rebase: update HEAD when is an oid

Message

Philippe Blain via GitGitGadget March 11, 2022, 5:24 p.m. UTC

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 <[e-mail]>
          Signed-off-by: John Cai <[e-mail]>
      
       ## 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 &&