Junio,
Thank you for your feedback! Several of your concerns were already
raised by Jonathan Nieder; I'm going to focus on the specifics you
brought up that Jonathan didn't, to avoid repeating myself too much.
On Fri, Jan 3, 2020 at 10:27 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Hmm.
>
> No off-the-shelf tool I know of exits 125 to signal "I dunno", so if
> you have to care about the special status 125, the "command" you are
> driving with "git bisect run" must be something that was written
> specifically to match what "git bisect" expects by knowing that 125
> is a special code to declare that the commit's goodness cannot be
> determined. Now, what's the reason why this "command" written
> specifically to be used with "git bisect", which even knows the
> special 125 convention, yields "this is good" in the wrong polarity?
>
> The only realistic reason I can think of is when the user is hunting
> for a commit that fixed what has long been broken. In such a use
> case, commits in the older part of the history would not pass the
> test (i.e. the exit status of the script would be non-zero) while
> commits in the newer part of the history would.
That's nearly exactly what happened: after bisecting the problem to
find when it was introduced, I found that an updated version did not
fail the test. I wanted to bisect the "fix" in order to review the
change and verify that it did, in fact, fix the problem -- as opposed
to change things so my test didn't fail (as it turned out, it was the
latter: rather than fix the issue, the "fix" commit simply turned the
bugged feature off by default.)
> I'd suggest dropping "-r", which has little connection to "--invert".
I was simply trying to be thorough, so if it doesn't need a short
name, that's fine by me. (And it's probably going to be
--invert-status.)
> Let's not make the style violations even worse.
Jonathan pointed that one out; you can see my corrected version in my
(at his suggestion) split-out version that simply adds support for
'run -- cmd'
> > + # how to localize part 2?
>
> Using things like "%2$s", you mean?
Hah, yeah, that was a leftover comment from before I came up with the
'$(printf $(gettext))' trick. (Which is now eval_gettext.)
> As I alluded earlier, it is unclear how this new feature should
> interact with the "we use 'xyzzy' to mean 'good', and 'frotz' to
> mean 'bad'" feature. One part of me would want to say that when
> running bisect with good and bad swapped, we should reverse the
> polarity of "bisect run" script automatically, but the rest of me
> of course would say that it would not necessarily be a good idea,
> and it is of course a huge backward incompatible change, so anything
> automatic is a no go.
This new feature works just fine with that (in fact, the last round of
tests in the test script explicitly covers this interaction.)
With "--invert-status" it doesn't matter what your names are, and with
"--success=<term>" it honors whatever terms you specified.
> > + echo "exit code $res means this commit is $state"
>
> Is this a leftover debugging output?
Yep, missed that one during my cursory review before uploading (since
test scripts hide their output by default, I didn't realize I'd left
it in there.)
> In any case, I wonder why something along the line of the attached
> patch is not sufficient and it needs this much code.
>
> git-bisect.sh | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index efee12b8b1..7fc4f9bd8f 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -247,6 +247,15 @@ bisect_run () {
> "$@"
> res=$?
>
> + if test -n "$invert_run_status"
> + then
> + case "$res" in
> + 0) res=1 ;;
> + 125) ;;
> + *) res=0 ;;
> + esac
> + fi
> +
Unfortunately, that doesn't behave properly.
As far as 'git bisect run' is concerned, there are four distinct sets
of status codes:
1. Test passed (translated to 'git bisect good') -- status 0
2. Test failed (translated to 'git bisect bad') -- status 1-124 or 126-127
3. Untestable (translated to 'git bisect skip') -- status 125
4. Malfunction (causes 'git bisect run' to halt immediately, leaving
the bisection incomplete) -- anything else
What needs to happen is that status code lists for "test passed" (#1)
and "test failed" (#2) are swapped; even when bisecting a fix, #3
(untestable) and #4 (malfunction) remain unchanged. Your patch remaps
case #4 to case #1.
(I'm actually going to put together a patch to allow the user to pare
down the exit code list for #2 to a specific list, to make 'run' less
dicey in the face of complex test requirements.)
--
-- Stevie-O
Real programmers use COPY CON PROGRAM.EXE
