[v4,0/3] rebase: update HEAD when is an oid

Message

Christoph Sommer via GitGitGadget March 17, 2022, 7:53 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 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 <[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.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 &&