[PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[John Cai via GitGitGadget]

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

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b29ad2b65e7..52afeffcc2e 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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;
 	ropts.head_msg = buf.buf;
 	if (reset_head(the_repository, &ropts) < 0)
 		ret = error(_("could not switch to %s"), options->switch_to);
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 71b1735e1dd..0b92e78976c 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -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 &&
+		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_done

base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b
-- 
gitgitgadget

Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[Junio C Hamano]

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

> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 71b1735e1dd..0b92e78976c 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -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 &&

The above is sufficient set-up.

> +		git tag hold &&
> +		git checkout -B main hold &&

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

> +		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)" &&

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

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

> +		test_cmp pre post
> +	)
> +'
> +
>  test_done
>
> base-commit: 1a4874565fa3b6668042216189551b98b4dc0b1b

Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[Junio C Hamano]

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

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

Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[John Cai]

Hi Junio,

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

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 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.

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:

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b29ad2b65e72..f0403fb12421 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -827,8 +827,10 @@ 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.branch = options->head_name;
+       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);

Otherwise, checkout_up_to_date() has to implicitly know the downstream logic in
update_refs(). I believe that's the main source of the confusion--is that right?

>
> Thanks.

Thanks
John

Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[Phillip Wood]

Hi John

Thanks for working on this

On 11/03/2022 05:05, John Cai via GitGitGadget wrote:
> 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].

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)

> 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

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.

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

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

> 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(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b29ad2b65e7..52afeffcc2e 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -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.

>   	ropts.head_msg = buf.buf;
>   	if (reset_head(the_repository, &ropts) < 0)
>   		ret = error(_("could not switch to %s"), options->switch_to);
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 71b1735e1dd..0b92e78976c 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -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 &&
> +		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 ----
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 71b1735e1d..d5a8ee39fc 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -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 &&

Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[John Cai]

Hi Phillip,

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

> Hi John
>
> Thanks for working on this
>
> On 11/03/2022 05:05, John Cai via GitGitGadget wrote:
>> 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].
>
> 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)

Thanks, will adjust.

>
>> 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
>
> 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.
>
>> 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/
>
> Maybe
> Reported-by: Michael McClimon <michael@mcclimon.org>
> ?
>
>> 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(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index b29ad2b65e7..52afeffcc2e 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -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

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

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

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

>
>>   	ropts.head_msg = buf.buf;
>>   	if (reset_head(the_repository, &ropts) < 0)
>>   		ret = error(_("could not switch to %s"), options->switch_to);
>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index 71b1735e1dd..0b92e78976c 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -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 &&
>> +		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.

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

>
> Best Wishes
>
> Phillip
>
> ---- >8 ----
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 71b1735e1d..d5a8ee39fc 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -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 &&

Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[John Cai]

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

> Hi John
>
> Thanks for working on this
>
> On 11/03/2022 05:05, John Cai via GitGitGadget wrote:
>> 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].
>
> 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)
>
>> 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
>
> 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.
>
>> 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/
>
> Maybe
> Reported-by: Michael McClimon <michael@mcclimon.org>
> ?
>
>> 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(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index b29ad2b65e7..52afeffcc2e 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -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

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

>
> 	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.
>
>>   	ropts.head_msg = buf.buf;
>>   	if (reset_head(the_repository, &ropts) < 0)
>>   		ret = error(_("could not switch to %s"), options->switch_to);
>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index 71b1735e1dd..0b92e78976c 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -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 &&
>> +		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 ----
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 71b1735e1d..d5a8ee39fc 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -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 &&

Re: [PATCH] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[Phillip Wood]

Hi John

On 11/03/2022 19:42, John Cai wrote:
> [...]
>>
>> 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?

The old version and the new version

Best Wishes

Phillip

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

[John Cai via GitGitGadget]

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

-- 
gitgitgadget

[PATCH v2 1/2] rebase: use test_commit helper in setup

[John Cai via GitGitGadget]

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

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 71b1735e1dd..0643d015255 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -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 &&
-- 
gitgitgadget

Re: [PATCH v2 1/2] rebase: use test_commit helper in setup

[Junio C Hamano]

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

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

OK.

>  test_expect_success 'prepare repository with topic branches' '
> -	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.

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

Re: [PATCH v2 1/2] rebase: use test_commit helper in setup

[Phillip Wood]

On 13/03/2022 07:50, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> 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.
> 
> OK.
> 
>>   test_expect_success 'prepare repository with topic branches' '
>> -	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.

Best Wishes

Phillip

Re: [PATCH v2 1/2] rebase: use test_commit helper in setup

[Junio C Hamano]

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

> On 13/03/2022 07:50, Junio C Hamano wrote:
>> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>> 
>>> 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.
>> OK.
>> 
>>>   test_expect_success 'prepare repository with topic branches' '
>>> -	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.

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.

[PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[John Cai via GitGitGadget]

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

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b29ad2b65e7..5ae7fa2a169 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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);
diff --git a/reset.c b/reset.c
index e3383a93343..f8e32fcc240 100644
--- a/reset.c
+++ b/reset.c
@@ -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)
+		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;
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 0643d015255..d5a8ee39fc4 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -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 &&
+	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 &&
-- 
gitgitgadget

Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[Junio C Hamano]

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

> diff --git a/reset.c b/reset.c
> index e3383a93343..f8e32fcc240 100644
> --- a/reset.c
> +++ b/reset.c
> @@ -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-&.

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

>  	if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
>  		ret = -1;
>  		goto leave_reset_head;
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 0643d015255..d5a8ee39fc4 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -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.

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

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

Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[Phillip Wood]

On 13/03/2022 07:58, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> diff --git a/reset.c b/reset.c
>> index e3383a93343..f8e32fcc240 100644
>> --- a/reset.c
>> +++ b/reset.c
>> @@ -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-&.
> 
>> +		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

Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[Phillip Wood]

On 14/03/2022 10:54, Phillip Wood wrote:
> On 13/03/2022 07:58, Junio C Hamano wrote:
>> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> diff --git a/reset.c b/reset.c
>>> index e3383a93343..f8e32fcc240 100644
>>> --- a/reset.c
>>> +++ b/reset.c
>>> @@ -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-&.
>>
>>> +        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

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

> Best Wishes
> 
> Phillip

Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[John Cai]

Hi Junio,

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

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> diff --git a/reset.c b/reset.c
>> index e3383a93343..f8e32fcc240 100644
>> --- a/reset.c
>> +++ b/reset.c
>> @@ -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-&.
>
>> +		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.

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

>
>>  	if (!refs_only && repo_hold_locked_index(r, &lock, LOCK_REPORT_ON_ERROR) < 0) {
>>  		ret = -1;
>>  		goto leave_reset_head;
>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index 0643d015255..d5a8ee39fc4 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -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.
>
>> +	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.

Thanks to Phillip for his help on this!

>
> Will queue.  Thanks.

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

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

Re: [PATCH v2 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[Phillip Wood]

Hi John

On 14/03/2022 14:11, John Cai wrote:
> Hi Junio,
> 
> On 13 Mar 2022, at 3:58, Junio C Hamano wrote:
> 
>> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>>
>>> diff --git a/reset.c b/reset.c
>>> index e3383a93343..f8e32fcc240 100644
>>> --- a/reset.c
>>> +++ b/reset.c
>>> @@ -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-&.
>>
>>> +		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.
> 
> I'm okay with taking this out, but would be good to get an ack from Phillip.

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

Best Wishes

Phillip

[PATCH v3 0/2] rebase: update HEAD when is an oid

[John Cai via GitGitGadget]

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

-- 
gitgitgadget

[PATCH v3 1/2] rebase: use test_commit helper in setup

[John Cai via GitGitGadget]

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

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 71b1735e1dd..0643d015255 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -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 &&
-- 
gitgitgadget

Re: [PATCH v3 1/2] rebase: use test_commit helper in setup

[Ævar Arnfjörð Bjarmason]

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

> 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(-)
>
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 71b1735e1dd..0643d015255 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -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 &&

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

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 0643d015255..6dc8df8be7e 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -35,9 +35,7 @@ test_expect_success 'prepare repository with topic branches' '
 	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
 '
@@ -114,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 &&

Maybe worthwhile squashing in & doing the rest?

Note that --no-tag and --append allow for converting more than could be
in the past before those (relatively recently added) options.

[PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[John Cai via GitGitGadget]

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

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b29ad2b65e7..5ae7fa2a169 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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);
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 0643d015255..d5a8ee39fc4 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -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 &&
+	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 &&
-- 
gitgitgadget

Re: [PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[Ævar Arnfjörð Bjarmason]

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

> 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

..but an OID...

> 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

brancheds -> branches.

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

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

But on the "tell" v.s. show... (more below)...
>
> 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(-)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b29ad2b65e7..5ae7fa2a169 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -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.:

	
	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.
	
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 0643d015255..d5a8ee39fc4 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -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 &&
> +	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 &&

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.

Re: [PATCH v3 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[Junio C Hamano]

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

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

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

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

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

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

[John Cai via GitGitGadget]

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

-- 
gitgitgadget

[PATCH v4 1/3] rebase: test showing bug in rebase with non-branch

[John Cai via GitGitGadget]

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

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 71b1735e1dd..5c4073f06d6 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -399,6 +399,15 @@ 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' '
+	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_expect_success 'refuse to switch to branch checked out elsewhere' '
 	git checkout main &&
 	git worktree add wt &&
-- 
gitgitgadget

Re: [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch

[Junio C Hamano]

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

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

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.

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

> +'

Thanks.

Re: [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch

[Junio C Hamano]

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

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

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.

Re: [PATCH v4 1/3] rebase: test showing bug in rebase with non-branch

[John Cai]

Hi Junio,

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

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> 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.
>
> 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.

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

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

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

>
>> +'
>
> Thanks.

[PATCH v4 2/3] rebase: use test_commit helper in setup

[John Cai via GitGitGadget]

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

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 5c4073f06d6..2fb3fabe60e 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -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." &&
+	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 &&
-	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 &&
-- 
gitgitgadget

Re: [PATCH v4 2/3] rebase: use test_commit helper in setup

[Phillip Wood]

Hi John

On 17/03/2022 19:53, John Cai via GitGitGadget wrote:
> 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(-)
> 
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 5c4073f06d6..2fb3fabe60e 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -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." &&
> +	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

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

Re: [PATCH v4 2/3] rebase: use test_commit helper in setup

[John Cai]

Hi Phillip,

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

> Hi John
>
> On 17/03/2022 19:53, John Cai via GitGitGadget wrote:
>> 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(-)
>>
>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> index 5c4073f06d6..2fb3fabe60e 100755
>> --- a/t/t3400-rebase.sh
>> +++ b/t/t3400-rebase.sh
>> @@ -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." &&
>> +	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.

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.

>
> Best Wishes
>
> Phillip

Thanks!
John

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

[PATCH v4 3/3] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[John Cai via GitGitGadget]

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

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b29ad2b65e7..27fde7bf281 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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);
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 2fb3fabe60e..cf55b017ffc 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -389,13 +389,13 @@ 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
+	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' '
-- 
gitgitgadget

Re: [PATCH v4 3/3] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[Junio C Hamano]

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

> From: John Cai <johncai86@gmail.com>
>
> 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).

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

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

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

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.

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

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

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b29ad2b65e..27fde7bf28 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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);
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 6dc8df8be7..cf55b017ff 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -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

Re: [PATCH v4 3/3] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[John Cai]

Hi Junio,

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

> "John Cai via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
>> From: John Cai <johncai86@gmail.com>
>>
>> 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).
>
>> ... 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.
>
>> ...
>> 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.
>
>> 1. https://lore.kernel.org/git/xmqqsfrpbepd.fsf@gitster.g/
>
> 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.
>
>>  	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.
>
>> +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.

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

>
> ---- >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(+)
>
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index b29ad2b65e..27fde7bf28 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -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);
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 6dc8df8be7..cf55b017ff 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -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

Thanks
John

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

[John Cai via GitGitGadget]

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

-- 
gitgitgadget

[PATCH v5 1/2] rebase: use test_commit helper in setup

[John Cai via GitGitGadget]

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

diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 71b1735e1dd..0643d015255 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -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 &&
-- 
gitgitgadget

Re: [PATCH v5 1/2] rebase: use test_commit helper in setup

[Junio C Hamano]

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

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

Looks much simpler and obviously correct ;-)

> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> index 71b1735e1dd..0643d015255 100755
> --- a/t/t3400-rebase.sh
> +++ b/t/t3400-rebase.sh
> @@ -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 &&

[PATCH v5 2/2] rebase: set REF_HEAD_DETACH in checkout_up_to_date()

[John Cai via GitGitGadget]

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

diff --git a/builtin/rebase.c b/builtin/rebase.c
index b29ad2b65e7..27fde7bf281 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -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);
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index 0643d015255..d5a8ee39fc4 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -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 &&
+	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 &&
-- 
gitgitgadget

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

[Junio C Hamano]

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

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

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

Thanks for working on this fix.