Re: [RFC PATCH] bisect run: allow inverting meaning of exit code
- From
- Stephen Oberholtzer <stevie@qrpff.net>
- Date
- Jan 4, 2020, 06:22 UTC
- Message-ID
- <CAD_xR9fUxDTvwmAsfH-6=buRP+UmwBHhQJSV+T3paUOy-S1CGw@mail.gmail.com>
- In-Reply-To
- <xmqqftgvdhpz.fsf@gitster-ct.c.googlers.com>
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@pobox.com> wrote:
Show 17 quoted lines
> > 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.)
Show 8 quoted lines
> 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.)
Show 23 quoted lines
> 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