git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Stephen Oberholtzer <stevie@qrpff.net>
To: git@vger.kernel.org
Cc: Christian Couder <chriscool@tuxfamily.org>
Subject: [RFC] Proposal: New options for git bisect run to control the interpretation of exit codes
Date: Fri, 10 Jan 2020 20:42:35 -0500	[thread overview]
Message-ID: <CAD_xR9f7jHnCByOaOVJvxdW2c5dPHM8OUDwZhcPL1iTVR3NzmQ@mail.gmail.com> (raw)

After considering the responses I got for my --invert-status proposal, I
went back to the drawing board and considered how it might interact with
another feature I was going to propose.  This is the result.

To avoid repeating myself, I'm going to start with an example of what I
imagine putting into Documentation/git-bisect.txt, to document this
feature.  After the second snip line, you can find my justification for
this feature, as well as some preemptive answers to questions I expect
people to have.

>8----------------------------------------------------------------------8<

{at top}

git bisect run [--(old|good|<term-old>)-status=<list>]
[--(new|bad|<term-new>)-status=<list>]
[--skip-status=<list>] [--] <cmd>...

{below the paragraph beginning with "The special exit code 125")

Custom exit status mappings
^^^^^^^^^^^^^^^^^^^^^^^^^^^

The previous paragraphs describe how the exit status of <cmd> is mapped
to one of four actions:

 * `old`/`good`, which defaults to exit status 0.

 * `new`/`bad`, which defaults to exit status 1-127 except 125.

 * `skip`, which defaults to exit status 125.

 * Any status not mapped to one of the first three is treated as a fatal
   error, causing `git bisect run` to immediately terminate with a
   nonzero exit status.

For more precise control over bisect run's interpretation of the command's
exit status, you can use one or more `--<term>-status=<list>` options:

<term>::
  One of the following:
  * `skip` (`--skip-status`)
  * `old` or the corresponding term specified in the bisect start call
     (`--old-status` or `--good-status`)
  * `new` or the corresponding term specified in the bisect start call
     (`--new-status` or `--bad-status`)
<list>::
A comma-separated list of values (e.g. 1) or ranges (e.g. 1-10).

(It should be noted that specifying --old-status is unlikely to be useful
without a corresponding --new-status option.)

This feature can make a few things much easier:

 * If you want to bisect a "fix", you can use (as an example)

---------------
$ git bisect run --old-status=1-127 --new-status=0 my_script arguments
---------------

 * If the test involves some fundamentally-unreliable aspects such as I/O
  (for example, a network connection could be disrupted, or a file write
   could fail due to disk space issues), you can specify something like
   --new-status=99, and then any exit status other than 0 or 99 will be
   treated as a fatal error:

   -----------------
   $ git bisect run --new-status=99 sh -c 'setup-test && (run-test || exit 99)'
   -----------------

   If setup-test exits with a nonzero status code (except 99), then the
   run will abort with an error, rather than assume that the current commit
   contains the issue being bisected.


The default status code lists are as follows:
  --skip-status=125
  --old-status=0
  --new-status=1-127

The priority of each classification is as follows:

 * If the status code is on the --skip-status list, the action is "skip".
   Note that this takes precedence over the --old-status and --new-status
   lists; doing so simplifies the specification of --new-status.

 * If the status is on the --old-status list, *and* not on the --new-status
   list, the action is "old" (or "good").

 * If the status is on the --new-status list, *and* not on the --old-status
   list, the action is "new" (or "bad").

 * Otherwise, the command is treated as having experienced a fatal error,
   and run will terminate with a nonzero exit status.

In the last case, the bisection will be left where it was; you may correct
the error and perform another bisect run, or you may finish the bisection
with manual good/bad commands.

>8----------------------------------------------------------------------8<

The motivation
==============

First, an excerpt from the current documentation for 'git bisect':

> 126 and 127 are used by POSIX shells to signal specific error status
> (127 is for command not found, 126 is for command found but not executable
> - these details do not matter, as they are normal errors in the script,
> as far as bisect run is concerned

This shows a fundamental disconnect between bisect run's view of the
world and reality.  I submit that, in reality, status codes 126 and 127 are
overwhelmingly likely indicators that the script did not work correctly,
in which case the run should be halted so the user may correct the issue.
However, 126 and 127 are mapped to "git bisect bad" -- as in, "this commit
definitely contains the bug that I am searching for".


Let's consider the consequences of an inappropriate status code:

- 'good':  The bisect will incorrectly select the wrong commit (specifically,
  a later commit than the one that actually introduced the issue.)

  This will also indirectly result in more trials than would otherwise be
  necessary to determine the result, because the bisection will have to be
  restarted at the point where the mistake occurred.  (In the worst possible
  case, where a mistake occurs on the first step, the bisection will take
  at least twice as long.)

- 'bad': The bisect will incorrectly select the wrong commit (specifically,
  an earlier commit than the one that actually introduced the issue.)

  Like 'good', this will also indirectly result in extra trials.

- 'skip': The bisect will unnecessarily test at least one extra commit for
  each false 'skip' result.  In the worst case, it may not be able to narrow
  down the issue to a single commit.

- Abort: The bisection stops until the user restarts it.  No extra commits
  are tested, though if the user isn't paying attention, the wallclock
  time will take longer than usual.

In particular, the behavior of a false match on 'good' or 'bad' is *at best*
extra time needed to do the bisection.  At worst -- if the user is not
familiar with the code in question -- a great deal of confusion and time-
wasting can result as the user investigates the wrong commit.  As such, it
is critical that you have no false 'good' or 'bad' results.


If you're using a shell script to run your test, a false 'good' result can
easily be prevented by putting 'set -e' at the top of the script.

Avoiding a false 'bad' result is far more difficult, especially if the test
is complex and you're not familiar with shell scripting in general.  (The
man page for bash, on my machine, is 3725 lines long, and does not lend
itself well to searching.)  For the task that set me down this path, it
took me about six iterations to create a robust script that would return
the exit code that git wanted,  and _it still didn't work right in all
cases_.


What would have been a great deal simpler is if I could have just picked
an exit code that none of the other commands in my script would ever
return (such as 99), and told git to treat any code other than 0 or 99 as
a fatal error.  Which is essentially what I ended up doing (or trying to),
but unfamiliar with shell scripting as I was, I had several issues.

If I can make it easy for someone to use bisect run without them having
to learn any otherwise-unnecessary shell scripting constructs, I would
consider that a win.


Pre-emptive Q&A
===============

Q: Why allow ranges?  Why not restrict it to single status codes?
A: It allows for a simpler implementation, because the default 'bad' status
   list is a already a range (1-124,126-127).  By supporting ranges,
   we need only one single code path:

   exit_bad=1-124,126-127
   # override exit_bad if --bad-status is specified
   # check exit code against exit_bad

Q: Why treat any exit code that's on both the 'old' and 'new' lists as a
   fatal error?
A: Because it's obviously a mistake, and we don't know the correct way
   to interpret it.  By halting the run and returning an error, we can
   return control to the user, who can fix it by fixing the overlapping
   lists and re-running the command.

Q: Why not an error when a code is on both 'old' and 'skip', or 'new' and
   'skip'?
A: Two reasons:
   1. As I detailed earlier, a false 'skip' is less problematic than a
      false 'new'/'bad'.
   2. By prioritizing 'skip' over 'new', the default behavior for 'new'
      can be written as '1-127' instead of '1-124,126-127'.


-- 
-- Stevie-O
Real programmers use COPY CON PROGRAM.EXE

             reply	other threads:[~2020-01-11  1:51 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-01-11  1:42 Stephen Oberholtzer [this message]
2020-01-16  0:39 ` [RFC] Proposal: New options for git bisect run to control the interpretation of exit codes Christian Couder
2020-01-16  2:40   ` Stephen Oberholtzer
2020-03-28 19:14     ` Philip Oakley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAD_xR9f7jHnCByOaOVJvxdW2c5dPHM8OUDwZhcPL1iTVR3NzmQ@mail.gmail.com \
    --to=stevie@qrpff.net \
    --cc=chriscool@tuxfamily.org \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).