patchrebase: set REF_HEAD_DETACH in checkout_up_to_date()

From: John Cai <johncai86@gmail.com>

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). This bug was reported by Michael McClimon. See [1].

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

Also add a test to ensure this behavior.

1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/

Signed-off-by: John Cai <johncai86@gmail.com>
---
    rebase: update HEAD when is an oid
    
    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/

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v1
Pull-Request: https://github.com/git/git/pull/1226

 builtin/rebase.c  |  2 +-
 t/t3400-rebase.sh | 21 +++++++++++++++++++++
 2 files changed, 22 insertions(+), 1 deletion(-)

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

The above is sufficient set-up.

These two are unnecessary. It was there in my bisect "runme" script only because I originally used an out-of-line repository that is prepared beforehand, so that "move main back to hold and rerun the rebase" can be done regardless of the bug in the previous version checked during "bisect run".

You may want to prepare for segfaulting "git rev-parse" in the above three lines.

Never "cat .git/HEAD".  use "git rev-parse".

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

It is unintuitive that unconditionally setting the RESET_HEAD_DETACH bit is the right solution.

If the command weren't "rebase master side^0" but "rebase master side", i.e. "please rebase the side branch itself, not an unnamed branch created out of the side branch, on master", according to <reset.h>, we ought to end up being on a detached HEAD, as reset_head() with the bit

    /* Request a detached checkout */
    #define RESET_HEAD_DETACH (1<<0)

requests a detached checkout. But that apparently is not what would happen with your patch applied.

Puzzled. The solution to the puzzle probably deserves to be in the proposed log message.

Thanks.

Hi Junio,

On 11 Mar 2022, at 0:55, Junio C Hamano wrote:

Good point. Thinking aloud, here is the callstack.

checkout_up_to_date() -> reset_head() -> update_refs() -> update_ref()

if <branch> is not a valid ref, rebase_options head_name is set to NULL. This eventually leads update_refs() to decide that it doesn't need to switch to a branch via its switch_to_branch variable.

reset.c:

if (!switch_to_branch)
	ret = update_ref(reflog_head, "HEAD", oid, head,
			 detach_head ? REF_NO_DEREF : 0,
			 UPDATE_REFS_MSG_ON_ERR);
 else {
	ret = update_ref(reflog_branch ? reflog_branch : reflog_head,
			 switch_to_branch, oid, NULL, 0,
			 UPDATE_REFS_MSG_ON_ERR);
	if (!ret)
		ret = create_symref("HEAD", switch_to_branch,
				    reflog_head);
}

since the flags do not include RESET_HEAD_DETACH, detach_head is set to false and we get a deferenced HEAD update.

The solution I came up with works because when <branch> __is__ a valid branch, udpate_refs() takes a different code path that calls create_symref() with a branch, which is why we don't end up with a detached HEAD.

I see why this is confusing though. From checkout_up_to_date's perspective it looks like we are unconditionally detaching HEAD. So what we could do is only set the flag in checkout_up_to_date() when, from checkout_up_to_date's perspective, we will end up with a detached head. something like this:

Hi John

Thanks for working on this

On 11/03/2022 05:05, John Cai via GitGitGadget wrote:

Often we just add a Reported-by: trailer unless the liked email has some useful extra info (which arguably should not be the case with a well written commit message)

As Junio points out it is confusing that it is always ok to pass that flag, I think we should only set it if we are not checking out a branch, see below.

Maybe
Reported-by: Michael McClimon <michael@mcclimon.org>
?

I think it would be clearer if the post image ended up as

	ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK
	if (options->head_name)
		ropts.branch = option->head_name
	else
		ropts.flags |= RESET_HEAD_DETACH

and we changed reset_head() to BUG() if both branch and RESET_HEAD_DETACH are given.

Using a stand alone test for bisecting makes sense but I think we should try and use the existing test setup for the regression test (it certainly does not need to run in its own worktree). The diff below shows how this could be done. Ideally there would be a preparatory commit that modernized the whole of the setup test rather than just the two commits we're using in the new test but that's not essential.

Best Wishes

Phillip

---- >8 ----

Hi Phillip,

On 11 Mar 2022, at 10:05, Phillip Wood wrote:

Thanks, will adjust.

Yes, this is what I had in mind as well :).

I didn't consider this though, thanks for the suggestion.

sounds good to me, might as well clean things up while we're at it.

On 11 Mar 2022, at 10:05, Phillip Wood wrote:

This is entirely for my own sake and revealing my ignorance, but what exactly does "pre" and "post" image refer to?

Hi John

On 11/03/2022 19:42, John Cai wrote:

The old version and the new version

Best Wishes

Phillip

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@mcclimon.org>
          Signed-off-by: John Cai <johncai86@gmail.com>
      
       ## 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 &&

From: John Cai <johncai86@gmail.com>

To prepare for the next commit that will test rebase with oids instead of branch names, update the rebase setup test to add a couple of tags we can use. This uses the test_commit helper so we can replace some lines that add a commit manually.

Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t3400-rebase.sh | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

OK.

This lossage is not explained. I do not know if we actually make use of the reflog in the tests, though.

On 13/03/2022 07:50, Junio C Hamano wrote:

It is the default these days so we don't need to waste a process setting it here.

Best Wishes

Phillip

Phillip Wood <phillip.wood123@gmail.com> writes:

I said "not explained" in the log message. I didn't ask anybody to explain it to me or to readers on the list.

Thanks.

From: John Cai <johncai86@gmail.com>

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.

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 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@mcclimon.org>
Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/rebase.c  | 5 ++++-
 reset.c           | 3 +++
 t/t3400-rebase.sh | 9 +++++++++
 3 files changed, 16 insertions(+), 1 deletion(-)

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

It's just style thing but it probably is easier to read to have an extra () around the bitwise-&.

I wonder if there is a valid use case NOT to use RESET_HEAD_DETACH when switch_to_branch == NULL. If there isn't, it could be that we can get rid of RESET_HEAD_DETACH bit and base this decision solely on switch_to_branch'es NULLness.

But that is totally outside the scope of this fix. I'd prefer to see a narrowly and sharply focused fix, and to be quite honest, I would be happier if this assertion weren't in this topic.

Good. For reproduction, the fork-point "First" does not have to be a raw object name. "Second^0" not being a branch name does matter.

All correct and to the point.  Good.

Will queue.  Thanks.

On 13/03/2022 07:58, Junio C Hamano wrote:

"rebase --skip" and "rebase --autostash" are two such uses I think

Best Wishes

Phillip

On 14/03/2022 10:54, Phillip Wood wrote:

Those don't update any refs though so are not helpful examples. Possibly when we fast-forward because HEAD is an ancestor of 'onto' is a potential case but at the moment I think we detach HEAD when checking out 'onto' and then update and checkout the branch if there is one (I've been thinking about fixing that so we only detach HEAD if we're going to rebase).

Best Wishes

Phillip

Hi Junio,

On 13 Mar 2022, at 3:58, Junio C Hamano wrote:

I'm okay with taking this out, but would be good to get an ack from Phillip.

Thanks to Phillip for his help on this!

More of a question about protocol. If there are some minor adjustments that are not really worth more disussion on the ML (like removing the BUG assertion), is it still protocol to send another series or just re-roll on the branch without sending out another patch series?

Thanks! John

Hi John

On 14/03/2022 14:11, John Cai wrote:

That's fine with me. The rest of the patch looks good as far as I'm concerned.

Best Wishes

Phillip

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 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  |  5 ++++-
 t/t3400-rebase.sh | 18 +++++++++++-------
 2 files changed, 15 insertions(+), 8 deletions(-)

base-commit: b896f729e240d250cf56899e6a0073f6aa469f5d
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1226%2Fjohn-cai%2Fjc%2Ffix-rebase-oids-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1226/john-cai/jc/fix-rebase-oids-v3
Pull-Request: https://github.com/git/git/pull/1226

Range-diff vs v2:

 1:  f3f084adfa6 ! 1:  f658eb00bcd rebase: use test_commit helper in setup
     @@ Commit message
          can use. This uses the test_commit helper so we can replace some lines
          that add a commit manually.
      
     +    Setting logAllRefUpdates is not necessary because it's on by default for
     +    repositories with a working tree.
     +
          Helped-by: Phillip Wood <phillip.wood123@gmail.com>
          Signed-off-by: John Cai <johncai86@gmail.com>
      
 2:  0e3c73375c1 ! 2:  bd1c9537ffc rebase: set REF_HEAD_DETACH in checkout_up_to_date()
     @@ builtin/rebase.c: static int checkout_up_to_date(struct rebase_options *options)
       	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 'switch to branch not checked out' '
       	git rebase main other

From: John Cai <johncai86@gmail.com>

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.

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 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@mcclimon.org>
Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/rebase.c  | 5 ++++-
 t/t3400-rebase.sh | 9 +++++++++
 2 files changed, 13 insertions(+), 1 deletion(-)

On Thu, Mar 17 2022, John Cai via GitGitGadget wrote:

..but an OID...

brancheds -> branches.

And maybe a new paragraph before "update_refs()"? I.e. "\n\nThen update_refs() checks..." ?

But on the "tell" v.s. show... (more below)...

In this case a smaller change of:

    if (!ropts.branch)
		ropts.flags |=  RESET_HEAD_DETACH;

will do the same.

I wonder if just converting it to a designated initializer while we're at it (or a pre-cleanup commit) would be better, i.e.:

	
	diff --git a/builtin/rebase.c b/builtin/rebase.c
	index 5ae7fa2a169..bf4fd4d2920 100644
	--- a/builtin/rebase.c
	+++ b/builtin/rebase.c
	@@ -820,18 +820,18 @@ static int rebase_config(const char *var, const char *value, void *data)
	 static int checkout_up_to_date(struct rebase_options *options)
	 {
	 	struct strbuf buf = STRBUF_INIT;
	-	struct reset_head_opts ropts = { 0 };
	+	struct reset_head_opts ropts = {
	+		.oid = &options->orig_head,
	+		.branch = options->head_name,
	+		.flags = (RESET_HEAD_RUN_POST_CHECKOUT_HOOK |
	+			  (options->head_name ? 0 : RESET_HEAD_DETACH)),
	+	};
	 	int ret = 0;
	 
	 	strbuf_addf(&buf, "%s: checkout %s",
	 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
	 		    options->switch_to);
	-	ropts.oid = &options->orig_head;
	-	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);

But in any case the functionality will be the same, so this is just
bikeshedding.
	

I think it's *much* easier to review these sorts of changes where there's a preceding commit that positively asserts what we do now, and we'll then in the "fix" commit change the behavior.

So more "show" v.s. "tell".

I.e. in this case do the "test_cmp_rev" to the "wrong" tip with a TODO comment or whatever, then in the fix just adjust it, then it's clear what we had before/after.

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

Thanks.  That is much easier to read and simpler to follow.

I do not think it easier to follow than even the original or the improvement above, especially the part that computes .flags member.

From: John Cai <johncai86@gmail.com>

To prepare for the next commit that will test rebase with oids instead of branch names, update the rebase setup test to add a couple of tags we can use. This uses the test_commit helper so we can replace some lines that add a commit manually.

Setting logAllRefUpdates is not necessary because it's on by default for repositories with a working tree.

Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t3400-rebase.sh | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

On Thu, Mar 17 2022, John Cai via GitGitGadget wrote:

At least these could also be done, and skimming there's a few more "git commit" that could also be converted:

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@mcclimon.org>
          Signed-off-by: John Cai <johncai86@gmail.com>
      
       ## 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 &&

From: John Cai <johncai86@gmail.com>

Currently when rebase is used with a non branch, and <oid> is up to date with base:

git rebase base <oid>

It will update the ref that HEAD is pointing at to <oid>, and leave HEAD unmodified.

This is a bug. The expected behavior is that the branch HEAD points at remains unmodified while HEAD is updated to point to <oid> in detached HEAD mode.

Signed-off-by: John Cai <johncai86@gmail.com>
Reported-by: Michael McClimon <michael@mcclimon.org>
---
 t/t3400-rebase.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

Never do tests this way.

The primary reason why we do not want to write our tests the way this patch does is because we do not _care_ how it is broken in the behaviour of the original code. 'main' moving out of $old_main is the bug we care about. It is still buggy if it did not move to Second, but some other commit. Yet this patch insists that 'main' to move to Second and nothing else. What we want is 'main' to stay at $old_main in the end anyway, and we should directly test that condition.

If you insist to have two commits (which I strongly recommend against in this case), you write a test that makes sure that 'main' stays at $old_main, but mark the test with test_expect_failure. And then later in the step that fixes the code, flip "expect_failure" to "expect_success".

But it is not ideal, either. Imagine what you see in "git show" output of the commit that fixed the problem. Most of the test that shows the behaviour that the commit _cares_ about will be outside post-context of the hunk that flips test_expect_failure to test_expect_success.

The best and the simplest way, for a simple case like this, to write test is to add the test to expect what we want to see in the end, and do so in the same commit as the one that corrects the behaviour of the code. If somebody wants to see what the breakage looks like, it is easy to

 (1) checkout the commit that fixes the code and adds such a test,

 (2) tentatively revert everything outside t/, and

 (3) run the test with "-i -v" options.

Then test_expect_success that wants to see 'main' to stay at $old_main will show that 'main' moved by a test failure. Working from a patch is the same way, i.e. you can apply only the parts inside t/ and run the current code to see the breakage, and then apply the rest to see the fix.

I already said that the second one should expect main to be at $old_main, but the "HEAD and main are the same" and "HEAD is a symolic-ref" test can be replaced with a single test that is "HEAD is a symbolic-ref to 'main'", which would be more strict. I.e.

	test "$(git symbolic-ref HEAD)" = refs/heads/main &&
	test_cmp_rev main "$old_main"

And such a test that expects the correct behaviour we want to have in the end should be added in [PATCH 3/3] when the code is fixed, not here in a separate commit.

Thanks.

Junio C Hamano <gitster@pobox.com> writes:

Scratch that. No, we do not want HEAD to be pointing at 'main' after rebase, so the above is totally wrong. What you have at the end of the series is the right version, as I said in my review of the [PATCH 3/3].

Thanks for working on this topic.

Hi Junio,

On 17 Mar 2022, at 17:10, Junio C Hamano wrote:

I was attemping to follow the advice to "show" vs "tell" in [1]. All this make sense to me however.

1. https://lore.kernel.org/git/220317.86r170d6zs.gmgdl@evledraar.gmail.com/

From: John Cai <johncai86@gmail.com>

To prepare for the next commit that will test rebase with oids instead of branch names, update the rebase setup test to add a couple of tags we can use. This uses the test_commit helper so we can replace some lines that add a commit manually.

Setting logAllRefUpdates is not necessary because it's on by default for repositories with a working tree.

Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t3400-rebase.sh | 18 ++++--------------
 1 file changed, 4 insertions(+), 14 deletions(-)

Hi John

On 17/03/2022 19:53, John Cai via GitGitGadget wrote:

This version has added some more conversions that are not mentioned it the commit message. If you want to covert the whole setup to use test_commit then that's great but I think you need to do the whole thing and say in the commit message that you're modernizing everything. As it stands a reader is left wondering why some of the setup that is not used in the next commit has been converted but other bits such as the "Modify A." above are left as is. I think the two possibilities that make sense are (a) convert only what we need for the next commit, or (b) modernize the test and convert everything.

Best Wishes

Phillip

Hi Phillip,

On 18 Mar 2022, at 7:14, Phillip Wood wrote:

I see your point. Then for the sake of a smaller series for the patch, I'll opt to only update the parts we need. Then maybe we can have a separate series to modernize this suite.

I'll re-roll this series with the clarifications to the commit message that Junio made.

Thanks! John

From: John Cai <johncai86@gmail.com>

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.

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

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.

Also add a test to ensure the correct behavior.

1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/

Signed-off-by: John Cai <johncai86@gmail.com>
---
 builtin/rebase.c  | 2 ++
 t/t3400-rebase.sh | 8 ++++----
 2 files changed, 6 insertions(+), 4 deletions(-)

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

Perhaps

    "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).

OK (URL is wrong; see below).

The explanation of how the bug occurs (elided) in the patch looked all reasonable. It read well.

Yup. _Add_ a test to ensure that. Not replace a misleading test that expected to see a wrong behaviour.

This is not the original bug report. It was an early hint for diagnosis.

[1] https://lore.kernel.org/git/YiokTm3GxIZQQUow@newk/

would be a more appropriate pointer.

OK. If head_name is not set, we do not want to touch the branch the HEAD happens to be pointing at, so we want to detach.

As we want (1) HEAD (detached) is pointing at Second, (2) 'main' stayed at $old_main, and (3) HEAD is detched, these three conditions look sane.

Thanks.

For reference, I discarded [1/3], queued [2/3] and replaced this [3/3] with the following for now.

---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ---- ---- >8 ----
From: John Cai <johncai86@gmail.com>
Subject: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

"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()

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. We want the HEAD detached at B instead.

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/YiokTm3GxIZQQUow@newk/

Reported-by: Michael McClimon <michael@mcclimon.org>
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/rebase.c  | 2 ++
 t/t3400-rebase.sh | 9 +++++++++
 2 files changed, 11 insertions(+)

Hi Junio,

On 17 Mar 2022, at 17:36, Junio C Hamano wrote:

Sounds good--this is what I was about to do anyways!

Thanks John

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 <michael@mcclimon.org>
          Signed-off-by: John Cai <johncai86@gmail.com>
     +    Signed-off-by: Junio C Hamano <gitster@pobox.com>
      
       ## 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 &&

From: John Cai <johncai86@gmail.com>

To prepare for the next commit that will test rebase with oids instead of branch names, update the rebase setup test to add a couple of tags we can use. This uses the test_commit helper so we can replace some lines that add a commit manually.

Setting logAllRefUpdates is not necessary because it's on by default for repositories with a working tree.

Helped-by: Phillip Wood <phillip.wood123@gmail.com>
Signed-off-by: John Cai <johncai86@gmail.com>
---
 t/t3400-rebase.sh | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

Looks much simpler and obviously correct ;-)

From: John Cai <johncai86@gmail.com>

"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()

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. We want the HEAD detached at B instead.

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/YiokTm3GxIZQQUow@newk/

Reported-by: Michael McClimon <michael@mcclimon.org>
Signed-off-by: John Cai <johncai86@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/rebase.c  | 2 ++
 t/t3400-rebase.sh | 9 +++++++++
 2 files changed, 11 insertions(+)

"John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:

Looking good.  I am tempted to say that we should declare victory.

Thanks for working on this fix.