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

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

[Rubén Justo]

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.

Let's teach bisect to use the "--ignore-other-worktrees" flag.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
 builtin/bisect.c            |  3 ++-
 t/t6030-bisect-porcelain.sh | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index cc9483e851..56da34648b 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -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);
 		if (run_command(&cmd)) {
 			error(_("could not check out original"
 				" HEAD '%s'. Try 'git bisect"
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 98a72ff78a..cc8acbab18 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -122,6 +122,29 @@ test_expect_success 'bisect start without -- takes unknown arg as pathspec' '
 	grep bar ".git/BISECT_NAMES"
 '
 
+test_expect_success 'bisect reset: back in a branch checked out also elsewhere' '
+	echo "shared" > branch.expect &&
+	test_bisect_reset() {
+		git -C $1 bisect start &&
+		git -C $1 bisect good $HASH1 &&
+		git -C $1 bisect bad $HASH3 &&
+		git -C $1 bisect reset &&
+		git -C $1 branch --show-current > branch.output &&
+		cmp branch.expect branch.output
+	} &&
+	test_when_finished "
+		git worktree remove wt1 &&
+		git worktree remove wt2 &&
+		git branch -d shared
+	" &&
+	git worktree add wt1 -b shared &&
+	git worktree add wt2 -f shared &&
+	# we test in both worktrees to ensure that works
+	# as expected with "first" and "next" worktrees
+	test_bisect_reset wt1 &&
+	test_bisect_reset wt2
+'
+
 test_expect_success 'bisect reset: back in the main branch' '
 	git bisect reset &&
 	echo "* main" > branch.expect &&
-- 
2.39.0

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

[Junio C Hamano]

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?

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

[Rubén Justo]

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.

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

[Junio C Hamano]

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

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

If that is what you are aiming at, I suspect that the posted patch
is doing it in a wrong way.  Instead, we should just declare that
the branch being bisected does not mean the branch cannot be checked
out elsewhere, so that

	$ git worktree add --detach ../another HEAD^0
	$ git checkout -b work
	$ git bisect start work work~3
        ... detaches ...
	$ git -C ../another checkout work

should just work, no?

I admit I haven't thought things through, but I tend to be
sympathetic to such a declaration.  After all, "bisect" is a
read-only operation as far as the branch you happened to be on when
you started a bisect session is concerned.  Jumping around and
materializing tree states recorded in various commits leading to the
tip of the branch and inspecting them would not change anything on
the branch itself.  And more importantly, the branch being checked
out in another worktree and modified there should not break the
bisection, EXCEPT that the final "git bisect reset" (without
arguments) would fail if the other worktree removed the branch.

So, how about removing the is_worktree_being_bisected() check from
find_shared_symref(), so that not just "worktree add" and "bisect
reset", but "checkout" and "switch" are allowed to make the branch
current even it is being bisected elsewhere?

That would affect the other topic, I suspect, as well.  It may be a
positive change.  Or are there cases I missed, where the branch
being bisected should not be touched from elsewhere, and we cannot
remove the check from find_shared_symref()?

Thanks.

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> the branch itself.  And more importantly, the branch being checked
> out in another worktree and modified there should not break the
> bisection, EXCEPT that the final "git bisect reset" (without
> arguments) would fail if the other worktree removed the branch.

And "bisect reset [<branch>]" (with or without arguments) should not
ignore other worktrees when it runs "checkout" internally.  You
might have done

    * checkout 'work' in worktree A
    * start bisection of it there
    * checkout 'work' in worktree B
    * finish bisection of 'work' in worktree A
    * "git bisect reset"

and the third step should allow you work on 'work' in the other
worktree, but then the last step should not allow 'work' to be
checked out in two places (it is OK for the user to use "git bisect
reset main" and then "git checkout --ignore-other work" to work it
around, of course, but the default should be safe).

Hmm?

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

[Rubén Justo]

On 26-ene-2023 09:06:28, Junio C Hamano wrote:

> > 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.
> 
> If that is what you are aiming at, I suspect that the posted patch
> is doing it in a wrong way.  Instead, we should just declare that
> the branch being bisected does not mean the branch cannot be checked
> out elsewhere, so that
> 
> 	$ git worktree add --detach ../another HEAD^0
> 	$ git checkout -b work
> 	$ git bisect start work work~3
>         ... detaches ...
> 	$ git -C ../another checkout work
> 
> should just work, no?

Sorry, I should have been more explicit, I meant: "checking out back to the
implicitly checked out branch sounds like it should not fail in the worktree
where the user is bisecting".

So, to your question: no, in another worktree should not work without
the --ignore-other-worktrees.  But

	$ git checkout -b work
	$ git worktree add -f ../another work
	$ git -C ../another bisect start work work~3
	  ... detaches ...
	$ git -C ../another bisect reset

should work.

> So, how about removing the is_worktree_being_bisected() check from
> find_shared_symref(), so that not just "worktree add" and "bisect
> reset", but "checkout" and "switch" are allowed to make the branch
> current even it is being bisected elsewhere?

The devil is in the details: "git branch -m", "git branch -d".

We're not ready to have BISECT_START pointing to a deleted branch, or
renaming a branch pointed by it.

Also the inconvenience that, if we allow the user to checkout normally
the same branch in another worktree, we are not providing any facility
in "git bisect reset" to override the "already checked out".  We are
forcing the user to "git bisect reset HEAD~0 && git checkout
--ignore-other-worktrees ...".  Which, to me, sound as an
inconvenience.

> 
> That would affect the other topic, I suspect, as well.

I'm not sure.  The other topic is somewhat independent of what we decide
here.

This series started because the other topic is going to affect "git
bisect", not the other way around.  But this series can be
considered even if the other topic is discarded.

Now, "git bisect reset" with a branch checked out multiple times, works
in the first worktree that has the branch checkedout (the main tree is
always the first), and fails in the next ones.  This is due to a bug
the other topic is fixing.

This series aims to make "git bisect reset" to the original branch, work
in all worktrees.  Independently of the other topic.

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

[Junio C Hamano]

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

> The devil is in the details: "git branch -m", "git branch -d".
>
> We're not ready to have BISECT_START pointing to a deleted branch, or
> renaming a branch pointed by it.

It indicates that the callers of find_shared_symref() to see if "is
this branch being actively used by checked out, bisected, or
rebased, and I shouldn't touch it?" need to know more than what
find_shared_symref() interface gives them---namely, "can I repoint
it to a different commit?" and "can I make it disappear?" are
different conditions they need to be able to learn.

Until that distinction becomes expressible, I am actually OK with
forbidding both operations, i.e. while a branch is being bisected
elsewhere, we should be able to update its tip to point at a
different commit, but it is OK to forbid that because we cannot
allow the branch be renamed away or removed.

Thanks.

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

[Rubén Justo]

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.

Let's teach bisect to use the "--ignore-other-worktrees" flag.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---
Range-diff against v1:
1:  f902db6bfb ! 1:  72e1526313 bisect: fix "reset" when branch is checked out elsewhere
    @@ builtin/bisect.c: static int bisect_reset(const char *commit)
      
      		cmd.git_cmd = 1;
     -		strvec_pushl(&cmd.args, "checkout", branch.buf, "--", NULL);
    -+		strvec_pushl(&cmd.args, "checkout", "--ignore-other-worktrees",
    -+				branch.buf, "--", NULL);
    ++		strvec_pushl(&cmd.args, "checkout", NULL);
    ++		if (!commit)
    ++			strvec_pushl(&cmd.args, "--ignore-other-worktrees", NULL);
    ++		strvec_pushl(&cmd.args, branch.buf, "--", NULL);
      		if (run_command(&cmd)) {
      			error(_("could not check out original"
      				" HEAD '%s'. Try 'git bisect"

 builtin/bisect.c            |  5 ++++-
 t/t6030-bisect-porcelain.sh | 23 +++++++++++++++++++++++
 2 files changed, 27 insertions(+), 1 deletion(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index 7301740267..46fba8db50 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -244,7 +244,10 @@ 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", NULL);
+		if (!commit)
+			strvec_pushl(&cmd.args, "--ignore-other-worktrees", NULL);
+		strvec_pushl(&cmd.args, branch.buf, "--", NULL);
 		if (run_command(&cmd)) {
 			error(_("could not check out original"
 				" HEAD '%s'. Try 'git bisect"
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 3ba4fdf615..fb01bd6abc 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -122,6 +122,29 @@ test_expect_success 'bisect start without -- takes unknown arg as pathspec' '
 	grep bar ".git/BISECT_NAMES"
 '
 
+test_expect_success 'bisect reset: back in a branch checked out also elsewhere' '
+	echo "shared" > branch.expect &&
+	test_bisect_reset() {
+		git -C $1 bisect start &&
+		git -C $1 bisect good $HASH1 &&
+		git -C $1 bisect bad $HASH3 &&
+		git -C $1 bisect reset &&
+		git -C $1 branch --show-current > branch.output &&
+		cmp branch.expect branch.output
+	} &&
+	test_when_finished "
+		git worktree remove wt1 &&
+		git worktree remove wt2 &&
+		git branch -d shared
+	" &&
+	git worktree add wt1 -b shared &&
+	git worktree add wt2 -f shared &&
+	# we test in both worktrees to ensure that works
+	# as expected with "first" and "next" worktrees
+	test_bisect_reset wt1 &&
+	test_bisect_reset wt2
+'
+
 test_expect_success 'bisect reset: back in the main branch' '
 	git bisect reset &&
 	echo "* main" > branch.expect &&
-- 
2.39.0

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

[Junio C Hamano]

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.

Sorry for asking possibly the same question again (which may mean
that the phrasing of this paragraph is misleading), but isn't it a
good thing if in this sequence:

 - I checkout 'main' and start bisecting (BISECT_HEAD says 'main');

 - I then checkout 'main' in another worktree; I may even make a
   commit or two, or even rename 'main' to 'master'.

 - I finish bisection and "bisect reset" tries to take me back to
   'main', which may notice that 'main' is checked out in the other
   worktree, and fail.

the last one failed?  After the above sequence, I now have two
worktrees, both checking out 'main', and it is exactly the situation
the safety valve tries to prevent from occuring, no?

Or is the new behaviour considered better because the third step
would try to check out 'main' that is checked out elsewhere only if
the second step was forced, so the person who decided to touch
'main' in another worktree should already be aware of the risk and
we should disable the safety valve in the third step automatically?

I am not sure if that is a sensible argument, but if that is the
case, let's spell it out in the proposed log message.

Thanks.

>  builtin/bisect.c            |  5 ++++-
>  t/t6030-bisect-porcelain.sh | 23 +++++++++++++++++++++++
>  2 files changed, 27 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/bisect.c b/builtin/bisect.c
> index 7301740267..46fba8db50 100644
> --- a/builtin/bisect.c
> +++ b/builtin/bisect.c
> @@ -244,7 +244,10 @@ 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", NULL);
> +		if (!commit)
> +			strvec_pushl(&cmd.args, "--ignore-other-worktrees", NULL);
> +		strvec_pushl(&cmd.args, branch.buf, "--", NULL);

OK, so this time around "git bisect reset" to go back to the
original branch gets --ignore-other-worktrees but "git bisect reset
HEAD" or other forms that names a branch still gets the safety.
That makes the blast radius smaller, but I am not 100% sure if
loosening the safety is a good thing.

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

[Rubén Justo]

On 06-feb-2023 14:29:03, 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.
> 
> Sorry for asking possibly the same question again (which may mean

No problem.  I am sorry because I don't understand what's worrying you.

> that the phrasing of this paragraph is misleading), but isn't it a
> good thing if in this sequence:
> 
>  - I checkout 'main' and start bisecting (BISECT_HEAD says 'main');
> 
>  - I then checkout 'main' in another worktree; I may even make a
>    commit or two, or even rename 'main' to 'master'.
> 
>  - I finish bisection and "bisect reset" tries to take me back to
>    'main', which may notice that 'main' is checked out in the other
>    worktree, and fail.
> 
> the last one failed?  After the above sequence, I now have two
> worktrees, both checking out 'main', and it is exactly the situation
> the safety valve tries to prevent from occuring, no?

We are considering the initial branch (BISECT_START) as a branch checked
out _implicitly_ in the worktree that is bisecting.  Doesn't that
provide us and the user enough safety?  And is this not enough safety
for us to allow "git bisect reset"?  Just "git bisect reset", without
any other argument.

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

[Junio C Hamano]

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

> No problem.  I am sorry because I don't understand what's worrying you.
>
>> that the phrasing of this paragraph is misleading), but isn't it a
>> good thing if in this sequence:
>> 
>>  - I checkout 'main' and start bisecting (BISECT_HEAD says 'main');
>> 
>>  - I then checkout 'main' in another worktree; I may even make a
>>    commit or two, or even rename 'main' to 'master'.
>> 
>>  - I finish bisection and "bisect reset" tries to take me back to
>>    'main', which may notice that 'main' is checked out in the other
>>    worktree, and fail.
>> 
>> the last one failed?  After the above sequence, I now have two
>> worktrees, both checking out 'main', and it is exactly the situation
>> the safety valve tries to prevent from occuring, no?
>
> We are considering the initial branch (BISECT_START) as a branch checked
> out _implicitly_ in the worktree that is bisecting.  Doesn't that
> provide us and the user enough safety?

If that is a question, then the answer is no.  If that is
rhetorical, then I just do not see how it gives us any safety.

In the end, if you allow "bisect reset" to check out 'main' in the
worktree you used to run bisection, the 'main' branch is checked out
twice, once there, and another checkout in the other worktree.  That
is exactly what "git checkout 'main'" in one worktree while 'main'
is already checked out in another would prevent from happening, no?

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

[Rubén Justo]

On 07-feb-2023 21:16:59, Junio C Hamano wrote:
> Rubén Justo <rjusto@gmail.com> writes:
> 
> > No problem.  I am sorry because I don't understand what's worrying you.
> >
> >> that the phrasing of this paragraph is misleading), but isn't it a
> >> good thing if in this sequence:
> >> 
> >>  - I checkout 'main' and start bisecting (BISECT_HEAD says 'main');
> >> 
> >>  - I then checkout 'main' in another worktree; I may even make a
> >>    commit or two, or even rename 'main' to 'master'.
> >> 
> >>  - I finish bisection and "bisect reset" tries to take me back to
> >>    'main', which may notice that 'main' is checked out in the other
> >>    worktree, and fail.
> >> 
> >> the last one failed?  After the above sequence, I now have two
> >> worktrees, both checking out 'main', and it is exactly the situation
> >> the safety valve tries to prevent from occuring, no?
> >
> > We are considering the initial branch (BISECT_START) as a branch checked
> > out _implicitly_ in the worktree that is bisecting.  Doesn't that
> > provide us and the user enough safety?
> 
> If that is a question, then the answer is no.  If that is
> rhetorical, then I just do not see how it gives us any safety.
> 
> In the end, if you allow "bisect reset" to check out 'main' in the
> worktree you used to run bisection, the 'main' branch is checked out
> twice, once there, and another checkout in the other worktree.  That
> is exactly what "git checkout 'main'" in one worktree while 'main'
> is already checked out in another would prevent from happening, no?

Yes, but I still don't understand what you are worried about.

If we compare with "rebase": "git rebase --abort" does checkout back to
the original branch too.  But as "git rebase" is in a more evolved
"builtin transition", and uses reset_head() instead of spawing a new Git
with "checkout", it avoids the "--ignore-other-worktrees".

The safety I'm considering with "git bisect reset" is that while a
branch is being bisected in a worktree, that branch cannot be (without
forcing): checked out in another worktree, deleted or renamed.

And this safety is enough, to me, to alleviate the user from having to
"git bisect reset" to a "safe" place and then "git checkout
--ignore-other-worktrees" to have the BISECT_START branch checked out
again.

Also, note that the aim of this patch is not to introduce a new behavior, but
fix how "git bisect reset" works with multiple worktrees.

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

[Eric Sunshine]

On Sat, Feb 4, 2023 at 6:02 PM Rubén Justo <rjusto@gmail.com> wrote:
> 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.
>
> Let's teach bisect to use the "--ignore-other-worktrees" flag.
>
> Signed-off-by: Rubén Justo <rjusto@gmail.com>
> ---
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> @@ -122,6 +122,29 @@ test_expect_success 'bisect start without -- takes unknown arg as pathspec' '
> +test_expect_success 'bisect reset: back in a branch checked out also elsewhere' '
> +       echo "shared" > branch.expect &&
> +       test_bisect_reset() {
> +               git -C $1 bisect start &&
> +               git -C $1 bisect good $HASH1 &&
> +               git -C $1 bisect bad $HASH3 &&
> +               git -C $1 bisect reset &&
> +               git -C $1 branch --show-current > branch.output &&
> +               cmp branch.expect branch.output
> +       } &&
> +       test_when_finished "
> +               git worktree remove wt1 &&
> +               git worktree remove wt2 &&
> +               git branch -d shared
> +       " &&

As mentioned in my review[1] of one of your other patches, &&-chaining
within the argument to test_when_finished() is probably undesirable in
this case since failure of any cleanup command would cause
test_when_finish() to fail, which would cause the test to fail
overall.

[1]: https://lore.kernel.org/git/CAPig+cQpizjmhmDKb=HPrcYqqRq7JpvC-NZvY7B9eBbG+NrfKw@mail.gmail.com/

> +       git worktree add wt1 -b shared &&
> +       git worktree add wt2 -f shared &&
> +       # we test in both worktrees to ensure that works
> +       # as expected with "first" and "next" worktrees
> +       test_bisect_reset wt1 &&
> +       test_bisect_reset wt2
> +'

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

[Rubén Justo]

On 14-feb-2023 23:52:08, Eric Sunshine wrote:
> On Sat, Feb 4, 2023 at 6:02 PM Rubén Justo <rjusto@gmail.com> wrote:
> > 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.
> >
> > Let's teach bisect to use the "--ignore-other-worktrees" flag.
> >
> > Signed-off-by: Rubén Justo <rjusto@gmail.com>
> > ---
> > diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> > @@ -122,6 +122,29 @@ test_expect_success 'bisect start without -- takes unknown arg as pathspec' '
> > +test_expect_success 'bisect reset: back in a branch checked out also elsewhere' '
> > +       echo "shared" > branch.expect &&
> > +       test_bisect_reset() {
> > +               git -C $1 bisect start &&
> > +               git -C $1 bisect good $HASH1 &&
> > +               git -C $1 bisect bad $HASH3 &&
> > +               git -C $1 bisect reset &&
> > +               git -C $1 branch --show-current > branch.output &&
> > +               cmp branch.expect branch.output
> > +       } &&
> > +       test_when_finished "
> > +               git worktree remove wt1 &&
> > +               git worktree remove wt2 &&
> > +               git branch -d shared
> > +       " &&
> 
> As mentioned in my review[1] of one of your other patches, &&-chaining
> within the argument to test_when_finished() is probably undesirable in
> this case since failure of any cleanup command would cause
> test_when_finish() to fail, which would cause the test to fail
> overall.

As I said in my other response, if there is no argument against this
change, I'll reroll with it.

Thanks.

> 
> [1]: https://lore.kernel.org/git/CAPig+cQpizjmhmDKb=HPrcYqqRq7JpvC-NZvY7B9eBbG+NrfKw@mail.gmail.com/
> 
> > +       git worktree add wt1 -b shared &&
> > +       git worktree add wt2 -f shared &&
> > +       # we test in both worktrees to ensure that works
> > +       # as expected with "first" and "next" worktrees
> > +       test_bisect_reset wt1 &&
> > +       test_bisect_reset wt2
> > +'

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

[Rubén Justo]

When the user starts a bisection in a worktree, we save the branch
currently checked out in that worktree (BISECT_START) to allow the user
to go back to that branch later on.

From that moment and until the bisection ends, the branch is no longer
checked out in that worktree, but we give it the same consideration as
if it was checked out, which means that we prevent:
 - deleting that branch and
 - checking out that branch in another worktree, unless the user force
   the ckeck out.

"bisect" was introduced as a helper script in 8cc6a08 ([PATCH] Making it
easier to find which change introduced a bug, 2005-07-30), and
originally based its functioning in Git builtin commands.  Although
"bisect" is now itself a builtin command, it still spawns Git
sub-processes to use other builtin commands, like "checkout".

Since 1d0fa89 (checkout: add --ignore-other-worktrees, 2015-01-03) we
have a safety valve in "checkout" and "switch" commands to prevent the
user from normally checking out simultaneously the same branch in
multiple worktrees.

If the user, using "--ignore-other-worktrees", checks out a branch
before or while bisecting that same branch in another worktree, we may
fail when the user ends the bisection using "git bisect reset":

   $ git worktree add work ../work/
   $ git checkout --ignore-other-worktrees work
   $ git -C ../work bisect start HEAD HEAD~3
   ... the user inspects some commits ...
   $ git -C ../work bisect reset
   fatal: 'work' is already checked out at ...
   error: could not check out original HEAD 'work'. Try 'git bisect reset <commit>'.

Thanks to the special consideration we give to the branch while being
bisected, and the safety valve introduced in 1d0fa89, we can assume the
user is aware and responsible of the "multiple checked out branch"
situation.  This makes sensible to allow the user to go back to the
original branch using "git bisect reset" and avoid him a somewhat less
intuitive sequence like: "git bisect reset work~0 && git checkout
--ignore-other-worktree work".

Let's teach "bisect" to use the "--ignore-other-worktrees" when the user
wants to end the bisection.

Signed-off-by: Rubén Justo <rjusto@gmail.com>
---

Changes since v2

 - Reworded the message to be more descriptive.
 - Using "||:" to chain commands in the test_when_finished block,
   suggested by Eric.

This series started because of a bug, being fixed in another series,
allowed "git bisect reset" to work in certain cases where multiple
branches were checked out.  Once the bug is fixed, "git bisect reset"
will no longer work in those scenarios.  Which is not bad, but taking
into account the different scenarios and the fact that some users may
rely on this bug as a feature, I think that the current patch proposes a
sensible and convenient change for the user.

Range-diff against v2:
1:  0fe0bc70dd ! 1:  3b2ac1aa8f bisect: fix "reset" when branch is checked out elsewhere
    @@ Metadata
      ## Commit message ##
         bisect: fix "reset" when branch is checked out elsewhere
     
    -    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.
    +    When the user starts a bisection in a worktree, we save the branch
    +    currently checked out in that worktree (BISECT_START) to allow the user
    +    to go back to that branch later on.
     
    -    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.
    +    From that moment and until the bisection ends, the branch is no longer
    +    checked out in that worktree, but we give it the same consideration as
    +    if it was checked out, which means that we prevent:
    +     - deleting that branch and
    +     - checking out that branch in another worktree, unless the user force
    +       the ckeck out.
     
    -    Let's teach bisect to use the "--ignore-other-worktrees" flag.
    +    "bisect" was introduced as a helper script in 8cc6a08 ([PATCH] Making it
    +    easier to find which change introduced a bug, 2005-07-30), and
    +    originally based its functioning in Git builtin commands.  Although
    +    "bisect" is now itself a builtin command, it still spawns Git
    +    sub-processes to use other builtin commands, like "checkout".
    +
    +    Since 1d0fa89 (checkout: add --ignore-other-worktrees, 2015-01-03) we
    +    have a safety valve in "checkout" and "switch" commands to prevent the
    +    user from normally checking out simultaneously the same branch in
    +    multiple worktrees.
    +
    +    If the user, using "--ignore-other-worktrees", checks out a branch
    +    before or while bisecting that same branch in another worktree, we may
    +    fail when the user ends the bisection using "git bisect reset":
    +
    +       $ git worktree add work ../work/
    +       $ git checkout --ignore-other-worktrees work
    +       $ git -C ../work bisect start HEAD HEAD~3
    +       ... the user inspects some commits ...
    +       $ git -C ../work bisect reset
    +       fatal: 'work' is already checked out at ...
    +       error: could not check out original HEAD 'work'. Try 'git bisect reset <commit>'.
    +
    +    Thanks to the special consideration we give to the branch while being
    +    bisected, and the safety valve introduced in 1d0fa89, we can assume the
    +    user is aware and responsible of the "multiple checked out branch"
    +    situation.  This makes sensible to allow the user to go back to the
    +    original branch using "git bisect reset" and avoid him a somewhat less
    +    intuitive sequence like: "git bisect reset work~0 && git checkout
    +    --ignore-other-worktree work".
    +
    +    Let's teach "bisect" to use the "--ignore-other-worktrees" when the user
    +    wants to end the bisection.
     
         Signed-off-by: Rubén Justo <rjusto@gmail.com>
     
    @@ builtin/bisect.c: static int bisect_reset(const char *commit)
     -		strvec_pushl(&cmd.args, "checkout", branch.buf, "--", NULL);
     +		strvec_pushl(&cmd.args, "checkout", NULL);
     +		if (!commit)
    -+			strvec_pushl(&cmd.args, "--ignore-other-worktrees", NULL);
    ++			strvec_pushl(&cmd.args, "--ignore-other-worktrees",
    ++				     NULL);
     +		strvec_pushl(&cmd.args, branch.buf, "--", NULL);
      		if (run_command(&cmd)) {
      			error(_("could not check out original"
    @@ t/t6030-bisect-porcelain.sh: test_expect_success 'bisect start without -- takes
     +		cmp branch.expect branch.output
     +	} &&
     +	test_when_finished "
    -+		git worktree remove wt1 &&
    -+		git worktree remove wt2 &&
    -+		git branch -d shared
    ++		git worktree remove wt1 ||:
    ++		git worktree remove wt2 ||:
    ++		git branch -d shared ||:
     +	" &&
     +	git worktree add wt1 -b shared &&
     +	git worktree add wt2 -f shared &&

 builtin/bisect.c            |  6 +++++-
 t/t6030-bisect-porcelain.sh | 23 +++++++++++++++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index 7301740267..011f674b08 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -244,7 +244,11 @@ 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", NULL);
+		if (!commit)
+			strvec_pushl(&cmd.args, "--ignore-other-worktrees",
+				     NULL);
+		strvec_pushl(&cmd.args, branch.buf, "--", NULL);
 		if (run_command(&cmd)) {
 			error(_("could not check out original"
 				" HEAD '%s'. Try 'git bisect"
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 3ba4fdf615..1d047f1c1a 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -122,6 +122,29 @@ test_expect_success 'bisect start without -- takes unknown arg as pathspec' '
 	grep bar ".git/BISECT_NAMES"
 '
 
+test_expect_success 'bisect reset: back in a branch checked out also elsewhere' '
+	echo "shared" > branch.expect &&
+	test_bisect_reset() {
+		git -C $1 bisect start &&
+		git -C $1 bisect good $HASH1 &&
+		git -C $1 bisect bad $HASH3 &&
+		git -C $1 bisect reset &&
+		git -C $1 branch --show-current > branch.output &&
+		cmp branch.expect branch.output
+	} &&
+	test_when_finished "
+		git worktree remove wt1 ||:
+		git worktree remove wt2 ||:
+		git branch -d shared ||:
+	" &&
+	git worktree add wt1 -b shared &&
+	git worktree add wt2 -f shared &&
+	# we test in both worktrees to ensure that works
+	# as expected with "first" and "next" worktrees
+	test_bisect_reset wt1 &&
+	test_bisect_reset wt2
+'
+
 test_expect_success 'bisect reset: back in the main branch' '
 	git bisect reset &&
 	echo "* main" > branch.expect &&
-- 
2.34.1