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

Commit Message

Rubén Justo Feb. 20, 2023, 10:53 p.m. UTC

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 <[e-mail]>
---

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 <[e-mail]>
     
    @@ 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(-)

Patch

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