(+cc: Christian Couder, bisect expert)
Hi!
Stephen Oberholtzer wrote:
> NOTE: This is obviously not ready for merging; I just wanted to
> get feedback.
Thanks for writing. I like where you're going.
> In particular, I expect some bikeshedding on the specific option
> names (-r, --invert, --expect). I'm probably going to change
> `--expect` to `--success`, in fact.
>
> If we can come to a consensus on the names (and, of course, on the
> feature itself), I'll clean up the tests, remove the debug output,
> update the documentation, then resubmit.
>
> >8------------------------------------------------------8<
>
> If your automated test naturally yields zero for _old_/_good_,
> 1-124 or 126-127 for _new_/_bad_, then you're set.
>
> If that logic is reversed, however, it's a bit more of a pain: you
> can't just stick a `sh -c !` in front of your command, because that
> doesn't account for exit codes 125 or 129-255.
>
> This commit enhances `git bisect run` as follows:
>
> * '--' can be used as an option list terminator,
> just as everywhere else.
Could this part go in a separate commit? That way, it can go in
more quickly while we figure out the rest. :)
> * The treatment of the exit code can be selected via an option:
>
> - No option, of course, treats 0 as _old_/_good_
> - `-r` (for reverse) treats 0 as _new_/_bad_
> - `--invert` is the long form for `-r`
> - `--expect=<term>` treats 0 as <term>
Initial thoughts:
- it's not immediately clear to me that this is common enough
to need the short-and-sweet name "-r". Could it have a long
form only?
- I think I agree with you that "git bisect run --expect=5" might
be more clearly written as "git bisect run --success=5". Or even
something explicitly referring to exit status, like
--success-status=5.
- This has an interesting relationship to the "alternate terms"
feature (--term-old / --term-new). I don't know if there's a
good way to make that more explicit --- maybe just some notes
with examples in the relevant parts of the manpage?
- the name --invert doesn't make it obvious to me what it is
inverting: good versus bad, ones complement of the status
code, revision range passed to "git bisect start"?
I'm even tempted to call it something like '-!', to make the
allusion to ! in shells more explicit. (But that's probably not a
great idea, since quoting ! correctly in interactive shells can be
difficult.)
Are there other commands we can try to make this consistent with?
"find" supports arbitrary expressions involving '!' and '-not'.
"git grep" has --invert-match: perhaps a name --invert-status
would be clear enough?
> You're not allowed to specify more than one expectation.
Usual convention would be "last specified option wins".
> Note that this lets one specify `--expect=good` as an explicit
> selection of the default behavior. This is intentional.
>
> Signed-off-by: Stephen Oberholtzer <stevie@xxxxxxxxx>
> ---
> git-bisect.sh | 33 +++++++++-
> t/t6071-bisect-run.sh | 142 ++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 172 insertions(+), 3 deletions(-)
> create mode 100755 t/t6071-bisect-run.sh
As you said, this needs docs. Writing docs often helps make the UI a
bit better since it forces one to think about the various ways a tool
would be used in practice.
> diff --git a/git-bisect.sh b/git-bisect.sh
> index efee12b8b1..dbeb213846 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -26,7 +26,7 @@ git bisect replay <logfile>
> replay bisection log.
> git bisect log
> show bisect log.
> -git bisect run <cmd>...
> +git bisect run [--expect=<term> | -r | --invert] [--] <cmd>...
> use <cmd>... to automatically bisect.
>
> Please use "git help bisect" to get the full man page.'
> @@ -238,6 +238,31 @@ bisect_replay () {
> bisect_run () {
> git bisect--helper --bisect-next-check $TERM_GOOD $TERM_BAD fail || exit
>
> + SUCCESS_TERM=$TERM_GOOD
> + FAIL_TERM=$TERM_BAD
> + INVERT_SET=
> + while [ "$1" != "${1#-}" ]; do
Might be simpler to do 'while true', and in the *) case to break.
> + case "$1" in
> + --)
> + shift
> + break ;;
> + --expect=$TERM_GOOD)
> + [ -z "$INVERT_SET" ] || die "$(gettext "bisect run: multiple expect options specified")"
> + INVERT_SET=1 ;;
> + -r|--invert|--expect=$TERM_BAD)
> + [ -z "$INVERT_SET" ] || die "$(gettext "bisect run: multiple expect options specified")"
> + SUCCESS_TERM=$TERM_BAD
> + FAIL_TERM=$TERM_GOOD
> + INVERT_SET=1 ;;
> + --expect=*)
> + # how to localize part 2?
> + die "$(printf "$(gettext "bisect run: invalid --expect value, use --expect=%s or --expect=%s")" "$TERM_GOOD" "$TERM_BAD")" ;;
It's more idiomatic to use eval_gettext here. See
"git grep -e die -- '*.sh'" for some examples.
> + *)
> + die "$(printf "$(gettext "bisect run: invalid option: %s")" "$1")" ;;
> + esac
> + shift
> + done
> +
> test -n "$*" || die "$(gettext "bisect run failed: no command provided.")"
>
> while true
> @@ -262,11 +287,13 @@ exit code \$res from '\$command' is < 0 or >= 128" >&2
> state='skip'
> elif [ $res -gt 0 ]
> then
> - state="$TERM_BAD"
> + state="$FAIL_TERM"
> else
> - state="$TERM_GOOD"
> + state="$SUCCESS_TERM"
> fi
>
> + echo "exit code $res means this commit is $state"
> +
> # We have to use a subshell because "bisect_state" can exit.
> ( bisect_state $state >"$GIT_DIR/BISECT_RUN" )
> res=$?
> diff --git a/t/t6071-bisect-run.sh b/t/t6071-bisect-run.sh
> new file mode 100755
> index 0000000000..2708e0f854
> --- /dev/null
> +++ b/t/t6071-bisect-run.sh
> @@ -0,0 +1,142 @@
> +# verify that unrecognized options are rejected by 'git bisect run'
> +#!/bin/sh
> +
> +# the linter's not smart enough to handle set -e
> +GIT_TEST_CHAIN_LINT=0
> +
> +test_description='Tests git bisect run'
> +
> +exec </dev/null
> +
> +. ./test-lib.sh
> +
> +{ test_expect_success 'Setting up repo for "git bisect run" tests.' "$(cat)" ; } <<'SETUP'
Tests put the commands to be passed to test_expect_success in a
single-quoted argument, with explicit &&-chaining. ("set -e" has
enough exceptions that it hasn't been worth using for this. The test
harness is able to detect a missing "&&" so this is less error-prone
than it sounds.) See t/README and any recently added test scripts in
t/ (try "git log -p --diff-filter=A -- t") for more details.
Thanks for a clear and pleasant description of the problem being
solved. I would like to say "here's a simple shell construct to use
instead of ! that makes this all redundant" but lacking that, for what
it's worth this set of new options seems like a good idea to me. :)
Sincerely,
Jonathan
(the rest left unsnipped for reference)
> +(
> +# I don't know how they managed it, but the git test engine
> +# somehow ignores the effect of 'set -e'.
> +set -eu || exit 1
> +# set -e canary
> +false
> +# hopefully, next year, we get -o pipefail!
> +echo '.DEFAULT: dummy
> +.PHONY: dummy
> +dummy:
> + true
> +' > Makefile
> +make
> +echo '0' >path0
> +git update-index --add -- Makefile path0
> +git commit -q -m 'initial commit'
> +git tag working0
> +# make some commits that don't cause problems
> +for x in `test_seq 1 20`; do
> + echo "$x" >path0
> + git update-index --replace -- path0
> + git commit -q -m "working commit $x"
> + git tag "working$x"
> +done
> +# break the makefile
> +sed -i.bak -e 's/true/false/' Makefile
> +rm -f Makefile.bak
> +! make
> +git update-index --replace -- Makefile
> +git commit -q -m "First broken commit"
> +git tag broken0
> +# make some more commits that still FTBFS
> +echo "exit code was $?; flags are $-"
> +for x in `test_seq 1 20`; do
> + echo "$x" >path0
> + git update-index --replace -- path0
> + git commit -q -m "broken build $x"
> + git tag "broken$x"
> +done
> +# repair it
> +git checkout working0 -- Makefile
> +make
> +git update-index --replace -- Makefile
> +git commit -q -m "First repaired commit"
> +git tag fixed0
> +# make some more commits with the bugfix
> +for x in `test_seq 1 20`; do
> + echo "$x" >path0
> + git update-index --replace -- path0
> + git commit -q -m "fixed build $x"
> + git tag "fixed$x"
> +done
> +#sh -c 'bash -i <> /dev/tty >&0 2>&1'
> +)
> +
> +SETUP
> +
> +test_expect_success 'setup first bisect' 'git bisect start && git bisect good working0 && git bisect bad broken9'
> +
> +test_expect_failure() {
> + shift
> + #echo arguments are "$*"
> + test_must_fail "$@"
> +}
> +
> +# okay, let's do some negative testing
> +
> +OLDPATH="$PATH"
> +
> +PATH="$PATH:."
> +
> +test_expect_success 'setup this-is-not-a-valid-option' '
> + echo "#/bin/sh" > --this-is-not-a-valid-option &&
> + chmod a+x -- --this-is-not-a-valid-option &&
> + --this-is-not-a-valid-option'
> +
> +test_expect_failure 'git bisect run: reject unrecognized options' git bisect run --this-is-not-a-valid-option
> +
> +PATH="$OLDPATH"
> +
> +test_expect_failure 'git bisect run: reject invalid values for --expect' git bisect run --expect=invalid make
> +
> +# okay, all of these settings are mutually exclusive (for sanity's sake, even with themselves)
> +for a in --expect=bad --expect=good -r --invert; do
> + for b in --expect=bad --expect=good -r --invert; do
> + test_expect_failure 'git bisect run: reject multiple --expect options' git bisect run $a $b make
> + done
> +done
> +
> +# finally, verify that '--' is honored (note that will mess things up and require a bisect reset)
> +PATH="$PATH:."
> +
> +test_expect_success 'git bisect run: honor --' 'git bisect run -- --this-is-not-a-valid-option'
> +
> +PATH="$OLDPATH"
> +
> +for expect_syntax in '' --expect=good; do
> +
> + # now we have to undo the bisect run
> + test_expect_success 'restarting bisection' 'git bisect reset && git bisect start && git bisect good working0 && git bisect bad broken9'
> +
> + test_expect_success "running bisection ($expect_syntax)" "
> +git bisect run $expect_syntax make &&
> +git log --oneline &&
> + # we should have determined that broken0 is the first bad version
> + test_cmp_rev broken0 refs/bisect/bad &&
> + # and that version should be the one checked out
> + test_cmp_rev broken0 HEAD
> +"
> +done
> +
> +
> +# NOW, test the reverse: find when we fixed it again
> +
> +for expect_syntax in -r --invert --expect=fixed; do
> +
> + test_expect_success 'restarting bisection' 'git bisect reset && git bisect start --term-old=broken --term-new=fixed && git bisect broken broken20 && git bisect fixed fixed20'
> + test_expect_success "running bisection ($expect_syntax)" "
> + git bisect run $expect_syntax make &&
> + git log --oneline &&
> + test_cmp_rev fixed0 refs/bisect/fixed &&
> + test_cmp_rev fixed0 HEAD
> + "
> +done
> +
> +test_expect_failure 'sanity check error message with custom terms' git bisect run --expect=invalid make
> +
> +
> +test_done
> --
> 2.20.1
>
