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

Message

Philippe Blain via GitGitGadget March 18, 2022, 1:54 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 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 <[e-mail]>
          Signed-off-by: John Cai <[e-mail]>
     +    Signed-off-by: Junio C Hamano <[e-mail]>
      
       ## 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 &&

Comments

Junio C Hamano March 18, 2022, 4:55 p.m. UTC | #1

"John Cai via GitGitGadget" <[e-mail]> 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.