git bisect code 125 - "WFT?"

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 <piotr.krukowiecki@gmail.com> writes:

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?

On Wed, Mar 16, 2011 at 10:36 PM, Junio C Hamano <gitster@pobox.com> wrote:

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.

Am 3/16/2011 23:06, schrieb Piotr Krukowiecki:

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

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

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

Jeff King <peff@peff.net> writes:

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

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:

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 ;) )

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(-)