git-bisect reset not deleting .git/BISECT_LOG

git-bisect reset not deleting .git/BISECT_LOG

[Janik Haag]

Thank you for filling out a Git bug report!
Please answer the following questions to help us understand your issue.

What did you do before the bug happened? (Steps to reproduce your issue)
```bash
git init /tmp/reproduce-bisect-warning
cd /tmp/reproduce-bisect-warning
touch .git/BISECT_LOG
git bisect reset
git switch -c log
```

What did you expect to happen? (Expected behavior)

`git-bisect reset` should have deleted .git/BISECT_LOG

What happened instead? (Actual behavior)

.git/BISECT_LOG is still there

What's different between what you expected and what actually happened?

git switch and some external tools like shell prompts are relying on 
that file to know if we are doing a bisect so switch printed a warning 
that we are still bisecting. Funnily enough git checkout doesn't print 
that warning.

Anything else you want to add:

In case you are wondering why I created the .git/BISECT_LOG file with 
touch instead of using the git cli, I forgot how I ended up there in the 
first place that was like a year ago and the warning didn't really 
bother me, today someone told me to look for anything in .git namend 
BISECT* and after deleting the log by hand it worked. So I figured 
git-bisect reset should probably have done that after I ended up in that 
unkown state. the content I had in BISECT_LOG before deleting it was:
```
# good: [e24028141f2] qdmr: fixup
git bisect good e24028141f278590407dd5389330f23d01c89cff
```


Please review the rest of the bug report below.
You can delete any lines you don't wish to share.


[System Info]
git version:
git version 2.42.0
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: 
/nix/store/lf0wpjrj8yx4gsmw2s3xfl58ixmqk8qa-bash-5.2-p15/bin/bash
uname: Linux 6.1.61 #1-NixOS SMP PREEMPT_DYNAMIC Thu Nov  2 08:35:33 UTC 
2023 x86_64
compiler info: gnuc: 12.3
libc info: glibc: 2.38
$SHELL (typically, interactive shell): /run/current-system/sw/bin/zsh


[Enabled Hooks]

Re: git-bisect reset not deleting .git/BISECT_LOG

[Jeff King]

On Mon, Nov 13, 2023 at 09:42:09PM +0100, Janik Haag wrote:

> Thank you for filling out a Git bug report!
> Please answer the following questions to help us understand your issue.
> 
> What did you do before the bug happened? (Steps to reproduce your issue)
> ```bash
> git init /tmp/reproduce-bisect-warning
> cd /tmp/reproduce-bisect-warning
> touch .git/BISECT_LOG
> git bisect reset
> git switch -c log
> ```
> 
> What did you expect to happen? (Expected behavior)
> 
> `git-bisect reset` should have deleted .git/BISECT_LOG
> 
> What happened instead? (Actual behavior)
> 
> .git/BISECT_LOG is still there

I don't think this is really specific to BISECT_LOG. In "bisect reset",
we'll call bisect_clean_state(), which removes a bunch of files. The
problem in your example is that "bisect reset" sees that we are not
bisecting and bails early.

Which I can kind of see, as part of the "reset" process is to reset to
the original pre-bisect commit. If it's not given on the command line,
the default is to use BISECT_START, which of course does not exist.

  As a side note, "git bisect reset HEAD" will do what you want, because
  it skips the BISECT_START check. But obviously that's not something
  normal users should need to know about, and I think is even an
  accidental side-effect of the conversion in 5e82c3dd22
  (bisect--helper: `bisect_reset` shell function in C, 2019-01-02).

So really, you just want the "clean" part of "bisect reset", but not the
"reset". We could have a separate "bisect clean" that would do what you
want (clean state without trying to reset HEAD). But I don't think it
would be unreasonable to "reset" to just unconditionally clean. I think
it would probably just be a few lines in bisect_reset() to avoid the
early return, and skip the call to "checkout" when we have no branch to
go back to.

Maybe a good simple patch for somebody interested in getting into Git
development?

-Peff

[PATCH] bisect: always clean on reset

[Jeff King]

On Mon, Nov 13, 2023 at 04:08:20PM -0500, Jeff King wrote:

> So really, you just want the "clean" part of "bisect reset", but not the
> "reset". We could have a separate "bisect clean" that would do what you
> want (clean state without trying to reset HEAD). But I don't think it
> would be unreasonable to "reset" to just unconditionally clean. I think
> it would probably just be a few lines in bisect_reset() to avoid the
> early return, and skip the call to "checkout" when we have no branch to
> go back to.
> 
> Maybe a good simple patch for somebody interested in getting into Git
> development?

I didn't want to forget about this, so I rolled it up into a patch.

-- >8 --
Subject: [PATCH] bisect: always clean on reset

Usually "bisect reset" cleans up any refs/bisect/ refs, along with
meta-files like .git/BISECT_LOG. But it only does so after deciding that
a bisection is active, which it does by reading BISECT_START. This is
usually fine, but it's possible to get into a confusing state if the
BISECT_START file is gone, but other cruft is left (this might be due to
a bug, or a system crash, etc).

And since "bisect reset" refuses to do anything in this state, the user
has no easy way to clean up the leftover cruft. While another "bisect
start" would clear the state, in the interim it can be annoying, as
other tools (like our bash prompt code) think we are bisecting, and
for-each-ref output may be polluted with refs/bisect/ entries.

Further adding to the confusion is that running "bisect reset $some_ref"
skips the BISECT_START check. So it never realizes that there's no
bisection active and does the cleanup anyway!

So let's just make sure we always do the cleanup, whether we looked at
BISECT_START or not. If the user doesn't give us a commit to reset to,
we'll still say "We are not bisecting" and skip the call to "git
checkout".

Reported-by: Janik Haag <janik@aq0.de>
Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/bisect.c            | 9 ++++-----
 t/t6030-bisect-porcelain.sh | 6 ++++++
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/builtin/bisect.c b/builtin/bisect.c
index 35938b05fd..c5565686bf 100644
--- a/builtin/bisect.c
+++ b/builtin/bisect.c
@@ -233,11 +233,10 @@ static int bisect_reset(const char *commit)
 	struct strbuf branch = STRBUF_INIT;
 
 	if (!commit) {
-		if (strbuf_read_file(&branch, git_path_bisect_start(), 0) < 1) {
+		if (!strbuf_read_file(&branch, git_path_bisect_start(), 0))
 			printf(_("We are not bisecting.\n"));
-			return 0;
-		}
-		strbuf_rtrim(&branch);
+		else
+			strbuf_rtrim(&branch);
 	} else {
 		struct object_id oid;
 
@@ -246,7 +245,7 @@ static int bisect_reset(const char *commit)
 		strbuf_addstr(&branch, commit);
 	}
 
-	if (!ref_exists("BISECT_HEAD")) {
+	if (branch.len && !ref_exists("BISECT_HEAD")) {
 		struct child_process cmd = CHILD_PROCESS_INIT;
 
 		cmd.git_cmd = 1;
diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
index 2a5b7d8379..7b24d1684e 100755
--- a/t/t6030-bisect-porcelain.sh
+++ b/t/t6030-bisect-porcelain.sh
@@ -170,6 +170,12 @@ test_expect_success 'bisect reset when not bisecting' '
 	cmp branch.expect branch.output
 '
 
+test_expect_success 'bisect reset cleans up even when not bisecting' '
+	echo garbage >.git/BISECT_LOG &&
+	git bisect reset &&
+	test_path_is_missing .git/BISECT_LOG
+'
+
 test_expect_success 'bisect reset removes packed refs' '
 	git bisect reset &&
 	git bisect start &&
-- 
2.43.0.664.ga12c899002