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

Commit Message

John Cai March 11, 2022, 5:24 p.m. UTC

From: John Cai <[e-mail]>

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 <[e-mail]>
Signed-off-by: John Cai <[e-mail]>
---
 builtin/rebase.c  | 5 ++++-
 reset.c           | 3 +++
 t/t3400-rebase.sh | 9 +++++++++
 3 files changed, 16 insertions(+), 1 deletion(-)

Comments

Junio C Hamano March 13, 2022, 7:58 a.m. UTC | #1

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

Phillip Wood March 14, 2022, 10:54 a.m. UTC | #2

On 13/03/2022 07:58, Junio C Hamano wrote:
> "John Cai via GitGitGadget" <[e-mail]> 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

Phillip Wood March 14, 2022, 2:05 p.m. UTC | #3

On 14/03/2022 10:54, Phillip Wood wrote:
> On 13/03/2022 07:58, Junio C Hamano wrote:
>> "John Cai via GitGitGadget" <[e-mail]> 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

John Cai March 14, 2022, 2:11 p.m. UTC | #4

Hi Junio,

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

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

Phillip Wood March 14, 2022, 2:25 p.m. UTC | #5

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" <[e-mail]> 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

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