git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] bisect: fix "reset" when branch is checked out elsewhere
@ 2023-01-22  1:38 Rubén Justo
  2023-01-23  2:01 ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Rubén Justo @ 2023-01-22  1:38 UTC (permalink / raw)
  To: Git List; +Cc: Junio C Hamano, Phillip Wood

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

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] bisect: fix "reset" when branch is checked out elsewhere
  2023-01-22  1:38 [PATCH] bisect: fix "reset" when branch is checked out elsewhere Rubén Justo
@ 2023-01-23  2:01 ` Junio C Hamano
  2023-01-26  2:18   ` Rubén Justo
  0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2023-01-23  2:01 UTC (permalink / raw)
  To: Rubén Justo; +Cc: Git List, Phillip Wood

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?

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] bisect: fix "reset" when branch is checked out elsewhere
  2023-01-23  2:01 ` Junio C Hamano
@ 2023-01-26  2:18   ` Rubén Justo
  2023-01-26 17:06     ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Rubén Justo @ 2023-01-26  2:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Git List, Phillip Wood

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.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] bisect: fix "reset" when branch is checked out elsewhere
  2023-01-26  2:18   ` Rubén Justo
@ 2023-01-26 17:06     ` Junio C Hamano
  2023-01-26 17:13       ` Junio C Hamano
  0 siblings, 1 reply; 5+ messages in thread
From: Junio C Hamano @ 2023-01-26 17:06 UTC (permalink / raw)
  To: Rubén Justo; +Cc: Git List, Phillip Wood

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.




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] bisect: fix "reset" when branch is checked out elsewhere
  2023-01-26 17:06     ` Junio C Hamano
@ 2023-01-26 17:13       ` Junio C Hamano
  0 siblings, 0 replies; 5+ messages in thread
From: Junio C Hamano @ 2023-01-26 17:13 UTC (permalink / raw)
  To: Rubén Justo; +Cc: Git List, Phillip Wood

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?

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2023-01-26 17:13 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-01-22  1:38 [PATCH] bisect: fix "reset" when branch is checked out elsewhere Rubén Justo
2023-01-23  2:01 ` Junio C Hamano
2023-01-26  2:18   ` Rubén Justo
2023-01-26 17:06     ` Junio C Hamano
2023-01-26 17:13       ` Junio C Hamano

Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).