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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 13, 2022, 07:58 UTC
Message-ID: <xmqq7d8ywaf0.fsf@gitster.g>
In-Reply-To: <0e3c73375c18a470fd5357b09acefeaf5ca4017f.1647019492.git.gitgitgadget@gmail.com>

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

Show 9 quoted lines

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

Show 15 quoted lines

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

Show 5 quoted lines

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