Re: [RFC PATCH] bisect run: allow inverting meaning of exit code

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jan 4, 2020, 01:22 UTC
Message-ID: <20200104012230.GD130883@google.com>
In-Reply-To: <20200103043027.4537-1-stevie@qrpff.net>

(+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.

Show 21 quoted lines

> 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. :)

Show 6 quoted lines

> * 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".

Show 9 quoted lines

> 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@qrpff.net>
> ---
>  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.

Show 21 quoted lines

> 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.

Show 15 quoted lines

> +		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.

Show 44 quoted lines

> +		*)
> +			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)

Show 132 quoted lines

> +(
> +# 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
>