rebase: set REF_HEAD_DETACH in checkout_up_to_date()

t/t3400-rebase.sh +1 -0

index 71b1735e1dd..0b92e78976c 100755
@@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' '
 	)
 '
 
+test_expect_success 'rebase with oids' '

t/t3400-rebase.sh +4 -0

+		test_cmp pre post
+	)
+'
+
 test_done

base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b

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.

Quoted 10 lines · John Cai via GitGitGadget

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.

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.

From: John Cai <[email protected]>

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/[email protected]/

Signed-off-by John Cai <[email protected]>

---
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/[email protected]/

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

builtin/rebase.c +0 -0

index b29ad2b65e7..52afeffcc2e 100644

builtin/rebase.c +0 -0

  	ropts.head_msg = buf.buf;
  	if (reset_head(the_repository, &ropts) < 0)
  		ret = error(_("could not switch to %s"), options->switch_to);

t/t3400-rebase.sh +10 -0

index 71b1735e1dd..0b92e78976c 100755
@@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' '
  	)
  '
 
+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 &&

Hi John

Thanks for working on this

Quoted 7 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

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,

Quoted 5 lines

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

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)

Quoted 9 lines

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

Quoted 4 lines

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

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.

Quoted 7 lines

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/[email protected]/

Maybe

Reported-by Michael McClimon <[email protected]>

?

Quoted 20 lines

Signed-off-by John Cai <[email protected]>

---
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/[email protected]/

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

builtin/rebase.c +0 -0

index b29ad2b65e7..52afeffcc2e 100644

builtin/rebase.c static int checkout_up_to_date(struct rebase_options *options) · line 831

Quoted 5 lines

builtin/rebase.c +1 -1

@@ -828,7 +828,7 @@ static int checkout_up_to_date(struct rebase_options *options)
  		    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;

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.

Quoted 25 lines

builtin/rebase.c +0 -0

  	ropts.head_msg = buf.buf;
  	if (reset_head(the_repository, &ropts) < 0)
  		ret = error(_("could not switch to %s"), options->switch_to);

t/t3400-rebase.sh +13 -0

index 71b1735e1dd..0b92e78976c 100755
@@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' '
  	)
  '
 +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 &&

t/t3400-rebase.sh test_expect_success 'rebase when inside worktree subdirectory' ' · line 458

Quoted 6 lines

t/t3400-rebase.sh +6 -0

+		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
+	)
+'

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

t/t3400-rebase.sh +11 -7

index 71b1735e1d..d5a8ee39fc 100755
@@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address
 export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
 
 test_expect_success 'prepare repository with topic branches' '
-       git config core.logAllRefUpdates true &&
-       echo First >A &&
-       git update-index --add A &&
-       git commit -m "Add A." &&
+       test_commit "Add A." A First First &&
        git checkout -b force-3way &&
        echo Dummy >Y &&
        git update-index --add Y &&

@@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '
        git mv A D/A &&
        git commit -m "Move A." &&
        git checkout -b my-topic-branch main &&
-       echo Second >B &&
-       git update-index --add B &&
-       git commit -m "Add B." &&
+       test_commit "Add B." B Second Second &&
        git checkout -f main &&
        echo Third >>A &&
        git update-index A &&

@@ -399,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '
        git rebase main other
 '
 
+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_expect_success 'refuse to switch to branch checked out elsewhere' '
        git checkout main &&
        git worktree add wt &&

Hi John

Thanks for working on this

Quoted 5 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

Fixes a bug whereby rebase updates the deferenced reference HEAD points to instead of HEAD directly.

Quoted 7 lines

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

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)

Quoted 8 lines

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

Quoted 4 lines

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

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.

Quoted 7 lines

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/[email protected]/

Maybe

Reported-by Michael McClimon <[email protected]>

?

Quoted 15 lines

Signed-off-by John Cai <[email protected]>

---
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/[email protected]/

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

builtin/rebase.c static int checkout_up_to_date(struct rebase_options *options) · line 831

Quoted 10 lines

builtin/rebase.c +1 -1

index b29ad2b65e7..52afeffcc2e 100644
@@ -828,7 +828,7 @@ static int checkout_up_to_date(struct rebase_options *options)
  		    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;

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.

Quoted 20 lines

builtin/rebase.c +0 -0

  	ropts.head_msg = buf.buf;
  	if (reset_head(the_repository, &ropts) < 0)
  		ret = error(_("could not switch to %s"), options->switch_to);

t/t3400-rebase.sh +9 -0

index 71b1735e1dd..0b92e78976c 100755
@@ -437,4 +437,25 @@ test_expect_success 'rebase when inside worktree subdirectory' '
  	)
  '
 +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 &&

t/t3400-rebase.sh test_expect_success 'rebase when inside worktree subdirectory' ' · line 458

Quoted 10 lines

t/t3400-rebase.sh +10 -0

+		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
+	)
+'

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

t/t3400-rebase.sh +11 -7

index 71b1735e1d..d5a8ee39fc 100755
@@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address
 export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
 
 test_expect_success 'prepare repository with topic branches' '
-       git config core.logAllRefUpdates true &&
-       echo First >A &&
-       git update-index --add A &&
-       git commit -m "Add A." &&
+       test_commit "Add A." A First First &&
        git checkout -b force-3way &&
        echo Dummy >Y &&
        git update-index --add Y &&

@@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '
        git mv A D/A &&
        git commit -m "Move A." &&
        git checkout -b my-topic-branch main &&
-       echo Second >B &&
-       git update-index --add B &&
-       git commit -m "Add B." &&
+       test_commit "Add B." B Second Second &&
        git checkout -f main &&
        echo Third >>A &&
        git update-index A &&

@@ -399,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '
        git rebase main other
 '
 
+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_expect_success 'refuse to switch to branch checked out elsewhere' '
        git checkout main &&
        git worktree add wt &&

[...]

Quoted 2 lines

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

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

From: John Cai <[email protected]>

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.

t/t3400-rebase.sh +2 -6

-	echo First >A &&
-	git update-index --add A &&
-	git commit -m "Add A." &&
+	test_commit "Add A." A First First &&
 	git checkout -b force-3way &&
 	echo Dummy >Y &&
 	git update-index --add Y &&

@@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '
 	git mv A D/A &&
 	git commit -m "Move A." &&
 	git checkout -b my-topic-branch main &&
-	echo Second >B &&
-	git update-index --add B &&
-	git commit -m "Add B." &&
+	test_commit "Add B." B Second Second &&
 	git checkout -f main &&
 	echo Third >>A &&
 	git update-index A &&

reset.c +0 -0

 	if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
 		ret = -1;
 		goto leave_reset_head;

t/t3400-rebase.sh +0 -0

index 0643d015255..d5a8ee39fc4 100755

t/t3400-rebase.sh +2 -0

+'
+
 test_expect_success 'refuse to switch to branch checked out elsewhere' '
 	git checkout main &&
 	git worktree add wt &&

Quoted 6 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

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.

OK.

t/t3400-rebase.sh GIT_AUTHOR_EMAIL=bogus@email@address · line 21

Quoted 2 lines

test_expect_success 'prepare repository with topic branches' '

t/t3400-rebase.sh +0 -1

-	git config core.logAllRefUpdates true &&

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

reset.c int reset_head(struct repository *r, const struct reset_head_opts *opts) · line 104

Quoted 9 lines · John Cai via GitGitGadget

reset.c +1 -0

index e3383a93343..f8e32fcc240 100644
@@ -101,6 +101,9 @@ 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)

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

reset.c int reset_head(struct repository *r, const struct reset_head_opts *opts) · line 105

Quoted 1 line

reset.c +1 -0

+		BUG("attempting to detach HEAD when branch is given");

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.

Quoted 15 lines · Junio C Hamano

reset.c int reset_head(struct repository *r, const struct · line 104

Quoted 9 lines · John Cai via GitGitGadget

reset.c +1 -0

index e3383a93343..f8e32fcc240 100644
@@ -101,6 +101,9 @@ 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)

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

Quoted 6 lines

reset.c int reset_head(struct repository *r, const struct reset_head_opts *opts) · line 105

Quoted 1 line

reset.c +1 -0

+        BUG("attempting to detach HEAD when branch is given");

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.

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

Best Wishes

Phillip

reset.c int reset_head(struct repository *r, const struct reset_head_opts *opts) · line 104

Quoted 9 lines · John Cai via GitGitGadget

reset.c +1 -0

index e3383a93343..f8e32fcc240 100644
@@ -101,6 +101,9 @@ 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)

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

reset.c int reset_head(struct repository *r, const struct reset_head_opts *opts) · line 105

Quoted 1 line

reset.c +1 -0

+		BUG("attempting to detach HEAD when branch is given");

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.

Quoted 9 lines

reset.c +0 -0

 	if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
 		ret = -1;
 		goto leave_reset_head;

t/t3400-rebase.sh +0 -0

index 0643d015255..d5a8ee39fc4 100755

t/t3400-rebase.sh test_expect_success 'switch to branch not checked out' ' · line 400

Quoted 7 lines

t/t3400-rebase.sh +4 -0

@@ -394,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '
 	git rebase main other
 '
 
+test_expect_success 'switch to non-branch detaches HEAD' '
+	git checkout main &&
+	old_main=$(git rev-parse HEAD) &&
+	git rebase First Second^0 &&

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.

t/t3400-rebase.sh test_expect_success 'switch to branch not checked out' ' · line 403

Quoted 3 lines

t/t3400-rebase.sh +3 -0

+	test_cmp_rev HEAD Second &&
+	test_cmp_rev main $old_main &&
+	test_must_fail git symbolic-ref HEAD

All correct and to the point. Good.

Will queue. Thanks.

Quoted 6 lines

t/t3400-rebase.sh +2 -0

+'
+
 test_expect_success 'refuse to switch to branch checked out elsewhere' '
 	git checkout main &&
 	git worktree add wt &&

Hi Junio,

Quoted 22 lines · Junio C Hamano

reset.c int reset_head(struct repository *r, const struct reset_head_opts *opts) · line 104

Quoted 9 lines · John Cai via GitGitGadget

reset.c +1 -0

index e3383a93343..f8e32fcc240 100644
@@ -101,6 +101,9 @@ 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)

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

reset.c int reset_head(struct repository *r, const struct reset_head_opts *opts) · line 105

Quoted 1 line

reset.c +1 -0

+		BUG("attempting to detach HEAD when branch is given");

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.

Quoted 3 lines

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.

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

Quoted 10 lines · Junio C Hamano

Quoted 6 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

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.

OK.

Quoted 4 lines

t/t3400-rebase.sh GIT_AUTHOR_EMAIL=bogus@email@address · line 21

Quoted 2 lines

test_expect_success 'prepare repository with topic branches' '

t/t3400-rebase.sh +0 -1

-	git config core.logAllRefUpdates true &&

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

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

From: John Cai <[email protected]>

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 <[email protected]>

Signed-off-by John Cai <[email protected]>

---
 t/t3400-rebase.sh | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

t/t3400-rebase.sh +1 -4

index 71b1735e1dd..0643d015255 100755
@@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address
 export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
 
 test_expect_success 'prepare repository with topic branches' '
-	git config core.logAllRefUpdates true &&
-	echo First >A &&
-	git update-index --add A &&
-	git commit -m "Add A." &&
+	test_commit "Add A." A First First &&
 	git checkout -b force-3way &&
 	echo Dummy >Y &&
 	git update-index --add Y &&

From: John Cai <[email protected]>

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/[email protected]/

Reported-by Michael McClimon <[email protected]>

Signed-off-by John Cai <[email protected]>

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

builtin/rebase.c +0 -0

index b29ad2b65e7..5ae7fa2a169 100644
@@ -827,8 +827,11 @@ static int checkout_up_to_date(struct rebase_options *options)
 		    getenv(GIT_REFLOG_ACTION_ENVIRONMENT),
 		    options->switch_to);

t/t3400-rebase.sh +2 -0

index 0643d015255..d5a8ee39fc4 100755
@@ -394,6 +394,15 @@ test_expect_success 'switch to branch not checked out' '
 	git rebase main other
 '
 
+test_expect_success 'switch to non-branch detaches HEAD' '
+	git checkout main &&

builtin/rebase.c static int checkout_up_to_date(struct rebase_options *options) · line 837

Quoted 13 lines

builtin/rebase.c +4 -1

@@ -827,8 +827,11 @@ 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;
+	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);

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

From: John Cai <[email protected]>

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.

+'

From: John Cai <[email protected]>

Fixes a bug whereby rebase updates the deferenced reference HEAD points to instead of HEAD directly.

... See [1] for
the original bug report.

...
Also add a test to ensure the correct behavior.

1. https://lore.kernel.org/git/[email protected]/

t/t3400-rebase.sh test_expect_success 'switch to branch not checked out' ' · line 408

Quoted 3 lines

t/t3400-rebase.sh +3 -0

+	test_cmp_rev HEAD main &&
+	test_cmp_rev main $(git rev-parse Second) &&
+	git symbolic-ref HEAD

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"

Quoted 5 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

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

Quoted 8 lines

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.

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.

t/t3400-rebase.sh test_expect_success 'switch to branch not checked out' ' · line 408

Quoted 8 lines

t/t3400-rebase.sh +7 -0

+test_expect_success 'switch to non-branch changes branch HEAD points to' '
+	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

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.

Quoted 2 lines

+'

Thanks.

Quoted 4 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

Fixes a bug whereby rebase updates the deferenced reference HEAD points to instead of HEAD directly.

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

Quoted 2 lines

... See [1] for
the original bug report.

OK (URL is wrong; see below).

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

Quoted 2 lines

...
Also add a test to ensure the correct behavior.

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

Quoted 1 line

1. https://lore.kernel.org/git/[email protected]/

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.

builtin/rebase.c static int checkout_up_to_date(struct rebase_options *options) · line 834

Quoted 6 lines

builtin/rebase.c +2 -0

 	ropts.oid = &options->orig_head;
 	ropts.branch = options->head_name;
 	ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+	if (!ropts.branch)
+		ropts.flags |=  RESET_HEAD_DETACH;
 	ropts.head_msg = buf.buf;

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.

Review

Quoted 10 lines

(quoted diff) +4 -3

+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
+	test_cmp_rev HEAD Second &&
+	test_cmp_rev main $old_main &&
+	test_must_fail git symbolic-ref HEAD

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 <[email protected]>
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 <[email protected]>

Signed-off-by John Cai <[email protected]>

Signed-off-by Junio C Hamano <[email protected]>

---
 builtin/rebase.c  | 2 ++
 t/t3400-rebase.sh | 9 +++++++++
 2 files changed, 11 insertions(+)

builtin/rebase.c +2 -0

index b29ad2b65e..27fde7bf28 100644
@@ -829,6 +829,8 @@ static int checkout_up_to_date(struct rebase_options *options)
 	ropts.oid = &options->orig_head;
 	ropts.branch = options->head_name;
 	ropts.flags = RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
+	if (!ropts.branch)
+		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);

t/t3400-rebase.sh +9 -1

index 6dc8df8be7..cf55b017ff 100755
@@ -389,6 +389,15 @@ test_expect_success 'switch to branch not checked out' '
 	git rebase main other
 '
 
+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_expect_success 'refuse to switch to branch checked out elsewhere' '
 	git checkout main &&
 	git worktree add wt &&
--

2.35.1-757-g4266a5c05c

From: John Cai <[email protected]>

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 <[email protected]>

Signed-off-by John Cai <[email protected]>

---
  t/t3400-rebase.sh | 18 ++++--------------
  1 file changed, 4 insertions(+), 14 deletions(-)

t/t3400-rebase.sh +1 -4

index 5c4073f06d6..2fb3fabe60e 100755
@@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address
  export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
 
  test_expect_success 'prepare repository with topic branches' '
-	git config core.logAllRefUpdates true &&
-	echo First >A &&
-	git update-index --add A &&
-	git commit -m "Add A." &&
+	test_commit "Add A." A First First &&
  	git checkout -b force-3way &&
  	echo Dummy >Y &&
  	git update-index --add Y &&

@@ -32,17 +29,13 @@ test_expect_success 'prepare repository with topic branches' '
  	git mv A D/A &&
  	git commit -m "Move A." &&

t/t3400-rebase.sh +2 -7

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

@@ -119,10 +112,7 @@ 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 &&

Hi John

Quoted 38 lines · John Cai via GitGitGadget

From: John Cai <[email protected]>

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 <[email protected]>

Signed-off-by John Cai <[email protected]>

---
  t/t3400-rebase.sh | 18 ++++--------------
  1 file changed, 4 insertions(+), 14 deletions(-)

t/t3400-rebase.sh +1 -7

index 5c4073f06d6..2fb3fabe60e 100755
@@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address
  export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
   test_expect_success 'prepare repository with topic branches' '
-	git config core.logAllRefUpdates true &&
-	echo First >A &&
-	git update-index --add A &&
-	git commit -m "Add A." &&
+	test_commit "Add A." A First First &&
  	git checkout -b force-3way &&
  	echo Dummy >Y &&
  	git update-index --add Y &&

@@ -32,17 +29,13 @@ test_expect_success 'prepare repository with topic branches' '
  	git mv A D/A &&
  	git commit -m "Move A." &&
  	git checkout -b my-topic-branch main &&
-	echo Second >B &&
-	git update-index --add B &&
-	git commit -m "Add B." &&

t/t3400-rebase.sh test_expect_success 'prepare repository with topic branches' ' · line 37

Quoted 6 lines

t/t3400-rebase.sh +1 -0

+	test_commit "Add B." B Second Second &&
  	git checkout -f main &&
  	echo Third >>A &&
  	git update-index A &&
  	git commit -m "Modify A." &&
  	git checkout -b side my-topic-branch &&

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

Quoted 20 lines

t/t3400-rebase.sh +2 -7

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

@@ -119,10 +112,7 @@ 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 &&

From: John Cai <[email protected]>

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 <[email protected]>

Signed-off-by John Cai <[email protected]>

---
 t/t3400-rebase.sh | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

t/t3400-rebase.sh +2 -7

index 71b1735e1dd..0643d015255 100755
@@ -18,10 +18,7 @@ GIT_AUTHOR_EMAIL=bogus@email@address
 export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL
 
 test_expect_success 'prepare repository with topic branches' '
-	git config core.logAllRefUpdates true &&
-	echo First >A &&
-	git update-index --add A &&
-	git commit -m "Add A." &&
+	test_commit "Add A." A First First &&
 	git checkout -b force-3way &&
 	echo Dummy >Y &&
 	git update-index --add Y &&

@@ -32,9 +29,7 @@ test_expect_success 'prepare repository with topic branches' '
 	git mv A D/A &&
 	git commit -m "Move A." &&
 	git checkout -b my-topic-branch main &&
-	echo Second >B &&
-	git update-index --add B &&
-	git commit -m "Add B." &&
+	test_commit "Add B." B Second Second &&
 	git checkout -f main &&
 	echo Third >>A &&
 	git update-index A &&

Fixes a bug [1] reported by Michael McClimon where rebase with oids will erroneously update the branch HEAD points to.