git bisect code 125 - "WFT?"

git bisect code 125 - "WFT?"

[Piotr Krukowiecki]

Hi,

in git bisect exit code 1-124 and 126-127 means "source is bad"
(0 means "ok", 125 means "can't be tested" and anything other is "stop").

My first reaction was "WTF" - you have a special value in the middle
of a range??

It got a bit clearer after reading the original patch[1] and man bash. It seems
this value is last "free" value for the user to use, at least in bash - values
above 125 may be used by the shell.

Bash uses code 126 if command is not executable, and 127 if command is
not found.

I think it would be better to use 126 and 127 as either "can't be tested" or
"stop" (125 should be left as is):

   * It would not leave a WTF gap in the "bad" range

   * If you get "command not executable" or "command not found" it's more
     probable something is broken than the code is bad, and you should fix
     your script

Opinions? Would it be possible to change the meaning of the codes now
(in 1.8.0)?

[1] http://article.gmane.org/gmane.comp.version-control.git/62390

-- 
Piotr Krukowiecki

Re: git bisect code 125 - "WFT?"

[Junio C Hamano]

Piotr Krukowiecki <piotr.krukowiecki@gmail.com> writes:

> Opinions? Would it be possible to change the meaning of the codes now
> (in 1.8.0)?

How about just documenting why it is a bad idea to use 126 or 127 as you
found out somewhere, and stopping there, iow, without changing the code to
use 126/127 that we consider it is a bad idea to use and avoided using so
far?

Re: git bisect code 125 - "WFT?"

[Piotr Krukowiecki]

On Wed, Mar 16, 2011 at 10:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Piotr Krukowiecki <piotr.krukowiecki@gmail.com> writes:
>
>> Opinions? Would it be possible to change the meaning of the codes now
>> (in 1.8.0)?
>
> How about just documenting why it is a bad idea to use 126 or 127 as you
> found out somewhere, and stopping there, iow, without changing the code to
> use 126/127 that we consider it is a bad idea to use and avoided using so
> far?

Documenting it won't help. If you get 126 code, you won't know if user
returned it to mark the code as bad, or if bash returned it to say
that it can't
execute a command.

Of course if changing the meaning is out of option it's better to document
then not to document.

-- 
Piotr Krukowiecki

Re: git bisect code 125 - "WFT?"

[Johannes Sixt]

Am 3/16/2011 23:06, schrieb Piotr Krukowiecki:
> On Wed, Mar 16, 2011 at 10:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Piotr Krukowiecki <piotr.krukowiecki@gmail.com> writes:
>>
>>> Opinions? Would it be possible to change the meaning of the codes now
>>> (in 1.8.0)?
>>
>> How about just documenting why it is a bad idea to use 126 or 127 as you
>> found out somewhere, and stopping there, iow, without changing the code to
>> use 126/127 that we consider it is a bad idea to use and avoided using so
>> far?
> 
> Documenting it won't help. If you get 126 code, you won't know if user
> returned it to mark the code as bad, or if bash returned it to say
> that it can't
> execute a command.

Huh? Why should the user's script return 126 or 127, particularly if the
documentation says "don't do that"? Moreover, any decent (shell)
programmer will know that these two values are reserved by POSIX for
particular purposes (they are _not_ specific to bash):

http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01_01

-- Hannes

Re: git bisect code 125 - "WFT?"

[Jeff King]

On Thu, Mar 17, 2011 at 07:55:06AM +0100, Johannes Sixt wrote:

> Am 3/16/2011 23:06, schrieb Piotr Krukowiecki:
> > On Wed, Mar 16, 2011 at 10:36 PM, Junio C Hamano <gitster@pobox.com> wrote:
> >> Piotr Krukowiecki <piotr.krukowiecki@gmail.com> writes:
> >>
> >>> Opinions? Would it be possible to change the meaning of the codes now
> >>> (in 1.8.0)?
> >>
> >> How about just documenting why it is a bad idea to use 126 or 127 as you
> >> found out somewhere, and stopping there, iow, without changing the code to
> >> use 126/127 that we consider it is a bad idea to use and avoided using so
> >> far?
> > 
> > Documenting it won't help. If you get 126 code, you won't know if user
> > returned it to mark the code as bad, or if bash returned it to say
> > that it can't
> > execute a command.
> 
> Huh? Why should the user's script return 126 or 127, particularly if the
> documentation says "don't do that"? Moreover, any decent (shell)
> programmer will know that these two values are reserved by POSIX for
> particular purposes (they are _not_ specific to bash):
> 
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_09_01_01

I think the argument is not that the user would want to return those
codes, but that we can protect a poorly written test script from itself
by including those exit codes in the list of "indeterminate result"
codes.

IOW, currently this:

  git bisect run 'make && ./test-pogram'

will happily generate a bogus bisection. And obviously that's a trivial
example, but one can imagine a much larger script with a missing command
in it.

There are two problems with that argument that I see, though:

  1. It only protects some very specific cases, and they're not even
     interesting cases. Say your bisection runs a test script that looks
     like this:

       cmd1
       missing-cmd
       cmd2

     we _still_ won't see it as an indeterminate result, because the
     missing cmd's exit code is lost. So you would have to write:

       cmd1 &&
       missing-cmd &&
       cmd2

     but that doesn't really help much. If you are going to be that
     careful, what you really want is something like:

       cmd1 || exit 125
       missing-cmd || exit 125
       cmd2 || exit 125
       some-final-command-to-check-the-state

     So it can help, but I don't think it really helps in real-world
     cases.

  2. If we do detect such a mishap, I'm not sure that "indeterminate
     result" is necessarily the best result, as that will just keep
     trying more and more commits without success. It is more likely a
     sign of a poorly written test script, and the best thing we could
     do is die and say "your test script looks buggy".

-Peff

Re: git bisect code 125 - "WFT?"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   2. If we do detect such a mishap, I'm not sure that "indeterminate
>      result" is necessarily the best result, as that will just keep
>      trying more and more commits without success. It is more likely a
>      sign of a poorly written test script, and the best thing we could
>      do is die and say "your test script looks buggy".

Exactly. I agree "Aborting the bisect as run-script is a crap" is the
right thing to do here.

[PATCH 1/2] git bisect run: exit code 126 and 127 abort the run

[Piotr Krukowiecki]

Those codes are reserved by POSIX to indicate
"command not executable" and "command not found":
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_08_02

Bisect used to treat them as codes returned by user
to mark a "bad" code.

With that approach it was not possible to differentiate
between codes returned by the user and codes returned by
shell (which is likely a sign of a poorly written test
script).

Another minor problem was lack of consistency in exit codes.
A "bad" code was marked by any value in range 1-124,126-127
and the gap in the middle looked weird.

Change the meaning of exit codes 126 and 127 to
"abort the bisect process" to fixes the above problems.

Signed-off-by: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
---
 git-bisect.sh |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

W dniu 17.03.2011 18:56, Junio C Hamano pisze:
> Jeff King <peff@peff.net> writes:
> 
>>   2. If we do detect such a mishap, I'm not sure that "indeterminate
>>      result" is necessarily the best result, as that will just keep
>>      trying more and more commits without success. It is more likely a
>>      sign of a poorly written test script, and the best thing we could
>>      do is die and say "your test script looks buggy".
> 
> Exactly. I agree "Aborting the bisect as run-script is a crap" is the
> right thing to do here.

Here's a patch. Next one changes git-bisect.txt

There's also Documentation/git-bisect-lk2009.txt which talks about exit
codes and should be changed somehow. As I understand it's a quote from an
email so I don't know if it should be edited in place, or should a note
be added at beginning?


(And again my patch has commit message longer than the changes...
 Now I understand why in-code documentation is lacking - after writing
 lengthy commit message documenting the code is just too much ;) )

diff --git a/git-bisect.sh b/git-bisect.sh
index c21e33c..9ca4852 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -376,9 +376,9 @@ bisect_run () {
       res=$?
 
       # Check for really bad run error.
-      if [ $res -lt 0 -o $res -ge 128 ]; then
+      if [ $res -lt 0 -o $res -ge 126 ]; then
 	  echo >&2 "bisect run failed:"
-	  echo >&2 "exit code $res from '$@' is < 0 or >= 128"
+	  echo >&2 "exit code $res from '$@' is < 0 or >= 126"
 	  exit $res
       fi
 
-- 
1.7.1

-- 
Piotr Krukowiecki

[PATCH 2/2] bisect documentation: exit codes 126,127 abort bisect

[Piotr Krukowiecki]

Update documentation after meaning of those exit codes changed
from "mark as bad code" to "abort bisect run".

Signed-off-by: Piotr Krukowiecki <piotr.krukowiecki@gmail.com>
---
 Documentation/git-bisect.txt |   11 +++++------
 1 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-bisect.txt b/Documentation/git-bisect.txt
index a1e47d6..70d8807 100644
--- a/Documentation/git-bisect.txt
+++ b/Documentation/git-bisect.txt
@@ -232,17 +232,16 @@ $ git bisect run my_script arguments
 
 Note that the script (`my_script` in the above example) should
 exit with code 0 if the current source code is good, and exit with a
-code between 1 and 127 (inclusive), except 125, if the current
-source code is bad.
+code between 1 and 124 (inclusive) if the current source code is bad.
+
+Exit code 125 should be used when the current source code cannot be
+tested. If the script exits with this code, the current revision will
+be skipped (see `git bisect skip` above).
 
 Any other exit code will abort the bisect process. It should be noted
 that a program that terminates via "exit(-1)" leaves $? = 255, (see the
 exit(3) manual page), as the value is chopped with "& 0377".
 
-The special exit code 125 should be used when the current source code
-cannot be tested. If the script exits with this code, the current
-revision will be skipped (see `git bisect skip` above).
-
 You may often find that during a bisect session you want to have
 temporary modifications (e.g. s/#define DEBUG 0/#define DEBUG 1/ in a
 header file, or "revision that does not have this commit needs this
-- 
1.7.1

-- 
Piotr Krukowiecki