Re: [PATCH] bisect: fix "reset" when branch is checked out elsewhere

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 23, 2023, 02:01 UTC
Message-ID: <xmqqo7qqovp1.fsf@gitster.g>
In-Reply-To: <1c36c334-9f10-3859-c92f-3d889e226769@gmail.com>

Rubén Justo <rjusto@gmail.com> writes:

Show 7 quoted lines

> Since 1d0fa89 (checkout: add --ignore-other-wortrees, 2015-01-03) we
> have a safety valve in checkout/switch to prevent the same branch from
> being checked out simultaneously in multiple worktrees.
>
> If a branch is bisected in a worktree while also being checked out in
> another worktree; when the bisection is finished, checking out the
> branch back in the current worktree may fail.

True. But we should explain why failing is a bad thing here. After all, "git checkout foo" to check out a branch 'foo' that is being used in a different worktree linked to the same repository fails, and that is a GOOD thing. Is the logic behind your "may fail and that is a bad thing" something like this?

    When "git bisect reset" goes back to the branch, it used to error
    out if the same branch is checked out in a different worktree.
    Since this can happen only after the end-user deliberately checked
    out the branch twice, erroring out does not contribute to any
    safety.

Having said that...

Show 7 quoted lines

> @@ -245,7 +245,8 @@ static int bisect_reset(const char *commit)
>  		struct child_process cmd = CHILD_PROCESS_INIT;
>  
>  		cmd.git_cmd = 1;
> -		strvec_pushl(&cmd.args, "checkout", branch.buf, "--", NULL);
> +		strvec_pushl(&cmd.args, "checkout", "--ignore-other-worktrees",
> +				branch.buf, "--", NULL);

"git bisect reset" does take an arbitrary commit or branch name, which may not be the original branch the user was on. If the user did not have any branch checked out twice, can they do something like

    $ git checkout foo
    $ git bisect start HEAD HEAD~20
    ... bisect session finds the first bad commit ...
    $ git bisect reset bar

where 'foo' is checked out only in this worktree? What happens if 'bar' has been checked out in a different worktree linked to the same repository while this bisect was going on? The current code may fail due to the safety "checkout" has, but isn't that exactly what we want? I.e. prevent 'bar' from being checked out twice by mistake? Giving 'bar' on the command line of "bisect reset" is likely because the user wants to start working on that branch, without necessarily knowing that they already have a worktree that checked out the branch elsewhere---in other words, isn't that a lazy folks' shorthand for "git bisect reset && git checkout bar"?

If we loosen the safety only when bisect_reset() receives NULL to its commit parameter, i.e. we are going back to the original branch, the damage might be limited to narrower use cases, but I still am not sure if the loosening is worth it.

IIUC, the scenario that may be helped would go like this:

    ... another worktree has 'foo' checked out ...
    $ git checkout --ignore-other-worktrees foo
    $ git bisect start HEAD HEAD~20
    ... bisect session finds the first bad commit ...
    $ git bisect reset

The last step wants to go back to 'foo', and it may be more convenient if it did not fail to go back to the risky state the user originally created. But doesn't the error message already tell us how to go back after this step refuses to recreate such a risky state? It sugests "git bisect reset <commit>" to switch to a detached HEAD, so presumably, after seeing the above fail and reading the error message, the user could do

    $ git bisect reset foo^{commit}

to finish the bisect session and detach the head at 'foo', and then the "usual" mantra to recreate the risky state that 'foo' is checked out twice can be done, i.e.

    $ git checkout --ignore-other-worktrees foo

So, I am not sure if this is a good idea in general.

Or do I misunderstand why you think "checking out may fail" is a bad thing?