Error message not surfaced for git bisect with script

[verhovsky]

(I'm using Magit through Doom Emacs)

I typed B s to start git bisecting with a script, chose the revisions, typed in my script (./configure && make) then it just said "Not bisecting". A completely useless error message. When I tried to git bisect manually and typed B B, chose my revisions it immediately said "Cannot bisect with uncommited changes", which was much more useful and I fixed the issue and was able to bisect with script after that.

1. when there's an error when I try to bisect with script it should tell me the error instead of just saying it refuses to do it.
2. it should tell me that it "Can't bisect with uncommited changes" before I type in the script to run. It should tell me that before I even type in any commits. Similarly for when I try to bisect but get the order of the commit hashes wrong.
3. it seems that it just naively puts the given bisect command into a command line that starts with git bisect, so that putting an && in the bisect command probably doesn't work? Or maybe it works with B B but doesn't work when the bisect breaks and you do B r again? it would be nice if it was escaped correctly so that it worked, or if it warned me that my command won't work.

If you want to reproduce my issue (I'm trying to figure out the oldest version of Bash that I can compile on my machine. I can compile the latest one):

cd /tmp
git checkout https://git.savannah.gnu.org/git/bash.git
cd bash
git checkout 4b0f8ba2
./configure && make

Then git bisect between 4b0f8ba2 and ca6a2ba

[verhovsky]

Seems similar to #3604

[kyleam]

Seems similar to #3604

Right, this is essentially the same issue. I agree that magit-bisect-run should have the same guards as magit-bisect-start:

• ca76db7 (magit-bisect-start: Error out on flipped revisions, 2018-09-28)
• 7c74b66 (magit-bisect-start: Error out when there are uncommitted changes, 2018-09-28)

it seems that it just naively puts the given bisect command into a command line that starts with git bisect, so that putting an && in the bisect command probably doesn't work?

I don't understand what you're referring to here. magit-bisect-run has used shell-file-name to run the command since 1093e5b (magit-bisect-run: Use shell to run shell commands, 2017-02-28). Here's the current code:

magit/lisp/magit-bisect.el

Lines 208 to 210 in c7ab693

	(magit--with-connection-local-variables
	(magit-git-bisect "run" (list shell-file-name
	shell-command-switch cmdline))))

When I enter a dummy true && true, I see running '/bin/zsh' '-c' 'true && true' as expected.

[kyleam]

I said:

I agree that magit-bisect-run should have the same guards as magit-bisect-start:

• ca76db7 (magit-bisect-start: Error out on flipped revisions, 2018-09-28)

• 7c74b66 (magit-bisect-start: Error out when there are uncommitted changes, 2018-09-28)

At the time of those commits, magit-bisect-run went through magit-bisect-start, so it shared the same guards. In f592e36 (magit-bisect-run: Don't start bisect asynchronously, 2021-06-20), I switched magit-bisect-run away from using magit-bisect-start to fix an unrelated issue, losing the guards.

[kyleam]

faa1907 restores the checks for magit-bisect-run.

It should tell me that before I even type in any commits [that there are uncommitted changes]

The above commit doesn't change this behavior. magit-bisect-run, like magit-bisect-start, reads the revs before aborting due to uncommitted changes. We want the "uncommitted changes?" check in the main body for non-interactive calls, but we could of course repeat the check in the interactive forms to avoid reading the revs. However, I think it's probably good enough as is and either way is separate from the main issue fixed above (-run not having the same checks as -start).