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

["Rubén Justo" <rjusto@gmail.com>]

On 22-ene-2023 18:01:46, Junio C Hamano wrote:

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

Maybe we make it fail for no reason.  Thank you for thinking about this with
depth.

I know you are aware, but for other readers;  In another thread, a bug-fix is
being reviewed that, if accepted, will affect "bisect".  With that fix, the
phrase "checking out may fail" will become "checking out will fail".  But does
it need to fail?

As you said, we want to reject normal check outs of a branch when that
branch is already checked out in other worktrees.  Because of this, sounds
reasonable to leave the implicit 'checkout' in 'bisect reset' to fail in those
cases, and just add some tests to notice if this changes in the future.

But, and this is what makes me think that "checking out will fail" is the wrong
choice for "bisect", while bisecting, with the worktree in a detached HEAD
state, the branch to which "bisect reset" will check out back (BISECT_START),
is still considered checked out in the working tree:

	$ git checkout -b work
	$ git bisect start HEAD HEAD~3
	... bisect detaches the current worktree ...
	$ git worktree add work
	Preparing worktree (checkout out 'work')
	fatal: 'work' is already checked out at ...

So, checking out back to the implicitly checked out branch sounds like it
should not fail.  And should be pretty secure (under the risk accepted by the
user) because we do not allow to normally check out the branch in another
worktree.  We even reject the checkout in the current worktree while bisecting,
but let's leave that for another time.

Note that due to the bug, what we are changing here is the reset on "second
worktrees".  That means, "bisect reset" on the main worktree has been allowed
for some time, we are just making it official.

And this is the less disrupting change in the usage.  Which does not justify
the change but does support it.

This is the reasoning I had and what makes me think that "checking out may
fail" is an inconvenience for the user, without any gain.

Now, if these arguments are reasonable, the next issue is what and how to
allow.  I chose at least strict, but maybe we can do something more
elaborate... just NULL sounds good.