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

[Junio C Hamano <gitster@pobox.com>]

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

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

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