"Nico Weber via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: Nico Weber <thakis@xxxxxxxxxxxx>
>
> If it's passed, then `git bisect run --verify script` will first
> check out the good revision and verify that the script passes there,
> then check out the bad revision and verify that it fails there,
> and only then start the actual `git bisect run script`.
>
> We use `git bisect run` heavily for bisecting bugs in LLVM when using
> clang to build Chromium. We sometimes end up with run scripts that are
> broken in some way, either by missing the +x bit, or in more subtle
> ways, and this adds a simple, low conceptual overhead way to smoke check
> the run script before starting a bisect that could run for a day or two.
In our log message, we tend NOT to say "This commit does X" or "X is
done", because such a statement is often insufficient to illustrate
if the commit indeed does X, and explain why it is a good thing to
do X in the first place.
Instead, we
- first explain that the current system does not do X (in present
tense, so we do NOT say "previously we did not do X"), then
- explain why doing X would be a good thing, and finally
- give an order to the codebase to start doing X.
For this change, it might look like this:
The script given to the `git bisect run script` may broken in some
way. [Explain why it is bad that 'script' is not run on the known
bad or good revisions here in the current code. The script is run
to test on the midway commit anyway and the lack of +x bit would
be noticed just as quickly as you retest a known good or bad
revision, so this 'feature' appears as a pure overhead to cause
extra checkouts and builds for no great additional benefit, at
least to me, and I tried but cannot make a good case for you
here].
As we already know at least one bad revision and good revision(s)
when the command is run, we can sanity check the script by running
on these revisions and checking if it produces expected results.
Introduce `--verify` option to the `git bisect run` command for
this purpose. It would cost checking out extra revisions and
retesting them but gives a basic sanity check for the script, to
save a potentially long "bisect" session that may span days.
> @@ -93,7 +93,6 @@ Eventually there will be no more revisions left to inspect, and the
> command will print out a description of the first bad commit. The
> reference `refs/bisect/bad` will be left pointing at that commit.
>
> -
> Bisect reset
> ~~~~~~~~~~~~
Why this removal? Just checking to make sure that the document does
not use double-blank lines between sections, in which case this
removal is a bad idea. In any case, this change does not have
anything to do with the `--verify` option, so leave it out---we can
review a documentation clean-up patch separately if you want.
> @@ -317,7 +316,7 @@ If you have a script that can tell if the current source code is good
> or bad, you can bisect by issuing the command:
>
> ------------
> -$ git bisect run my_script arguments
> +$ git bisect run [--verify] my_script arguments
> ------------
>
> Note that the script (`my_script` in the above example) should exit
> @@ -376,6 +375,13 @@ ignored.
> This option is particularly useful in avoiding false positives when a merged
> branch contained broken or non-buildable commits, but the merge itself was OK.
>
> +--verify::
> ++
> +Before the actual bisect run, check out the current bad revision and
> +verify that the script exits with a code between 1 and 127 (inclusive),
> +except 125, then check out the current good revision and verify that
> +the script exits with code 0. If not, abort the bisect run.
> +
"current bad revision" is OK, there is no concenpt of "current good
revision", but "the last revision marked as good" is a better way to
phrase what the above paragraph may be trying to address.
> +get_current_bisect_bounds () {
> + test -s "$GIT_DIR/BISECT_LOG" || die "$(gettext "We are not bisecting.")"
> + oIFS="$IFS" IFS="$IFS$(printf '\015')"
What is this CR about? Is anybody manually futzing with DOS line endings?
> + while read git bisect command rev tail
> + do
There are bisect/bad ref and bisect/good-* refs that lets you figure
out exactly where the unknown territories are, but because you are
only trying to find one bad and one good rev to use as a sample, you
do not have to enumerate all of them.
But I doubt that reading the bisect log a good way to do this. If
you only want one good and one bad rev to test, why not peek into
the implementation of bisect_next and find
git show-ref --hash --verify refs/bisect/$TERM_BAD
git for-each-ref --format="%(objectname" "refs/bisect/$TERM_GOOD-*"
that can easily be mimicked?
> + start)
> + CURRENT_BISECT_BAD="$rev"
> + CURRENT_BISECT_GOOD="$tail"
> + ;;
> + "$TERM_GOOD")
> + CURRENT_BISECT_GOOD="$rev" ;;
> + "$TERM_BAD")
> + CURRENT_BISECT_BAD="$rev" ;;
These variables are better called LAST_BISECT_(GOOD/BAD), as current
set of good revs have multiple commits in it. CURRENT_BISECT_BAD is
OK as-is when taken by itself, but as a contrasting pair to
LAST_BISECT_GOOD, renaming it to LAST_BISECT_BAD would make more
sense.
In any case, I am not sure why we need to read thru the log, futzing
with $IFS and all that.
> bisect_run () {
> + verify=
> + while test $# -ne 0
> + do
> + case "$1" in
> + --verify)
> + verify=t
> + ;;
> + --no-verify)
> + verify=
> + ;;
> + --)
Funny indentation.
> + shift
> + break
> + ;;
> + -*)
> + usage
> + ;;
> + *)
> + break
> + ;;
> + esac
> + shift
> + done
> +
> + if [ -n "$verify" ]; then
Style (Documentation/CodingGuidelines):
- We prefer "test" over "[ ... ]".
- Do not write control structures on a single line with semicolon.
"then" should be on the next line for if statements, and "do"
should be on the next line for "while" and "for".
> + git rev-parse --verify -q BISECT_HEAD > /dev/null && die "$(gettext "bisect run --verify is incompatible with --no-checkout")"
- Overlong line (cut after && _without_ adding trailing backslash)
- Redirection operators should be written with space before, but no
space after them. In other words, write 'echo test >"$file"'
instead of 'echo test> $file' or 'echo test > $file'.
> +
> + get_current_bisect_bounds
> + test -n "$CURRENT_BISECT_BAD" || die "$(gettext "bisect run --verify: no current bad revision")"
> + test -n "$CURRENT_BISECT_GOOD" || die "$(gettext "bisect run --verify: no current good revision")"
Dotto.
> + bisected_head=$(bisect_head)
> + rev=$(git rev-parse --verify "$bisected_head") ||
> + die "$(eval_gettext "Bad rev input: \$bisected_head")"
> +
> + trap "git checkout -q $rev" 0
> +
> + # Check script passes for good rev.
> + command="$@"
What effect do we expect out of this assignment to $command?
> + eval_gettextln "verifying script passes at \$TERM_GOOD rev"
> + eval git checkout -q "$CURRENT_BISECT_GOOD" || die "$(eval_gettext "failed to check out \$TERM_GOOD rev")"
> + "$@"
> + res=$?
> + if [ $res -ne 0 ]
> + then
> + die_with_status $res "$(eval_gettext "aborting: run script fails for \$TERM_GOOD rev")"
> + fi
> +
> + # Check script fails orderly for bad rev.
> + command="$@"
Ditto.
> + eval_gettextln "verifying script fails at \$TERM_BAD rev"
> + eval git checkout -q "$CURRENT_BISECT_BAD" || die "$(eval_gettext "failed to check out \$TERM_BAD rev")"
> + "$@"
> + res=$?
> + if [ $res -lt 0 -o $res -ge 128 ]
- We do not write our "test" command with "-a" and "-o" and use "&&"
or "||" to concatenate multiple "test" commands instead,...
