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

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

[Stephen Oberholtzer]

NOTE: This is obviously not ready for merging; I just wanted to
get feedback.

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.

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

You're not allowed to specify more than one expectation.

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

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
+		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")" ;;
+		*)
+			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'
+(
+# 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

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

[Jonathan Nieder]

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

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

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

[Junio C Hamano]

Stephen Oberholtzer <stevie@qrpff.net> writes:

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

Hmm.

No off-the-shelf tool I know of exits 125 to signal "I dunno", so if
you have to care about the special status 125, the "command" you are
driving with "git bisect run" must be something that was written
specifically to match what "git bisect" expects by knowing that 125
is a special code to declare that the commit's goodness cannot be
determined.  Now, what's the reason why this "command" written
specifically to be used with "git bisect", which even knows the
special 125 convention, yields "this is good" in the wrong polarity?

The only realistic reason I can think of is when the user is hunting
for a commit that fixed what has long been broken.  In such a use
case, commits in the older part of the history would not pass the
test (i.e. the exit status of the script would be non-zero) while
commits in the newer part of the history would.  

> -git bisect run <cmd>...
> +git bisect run [--expect=<term> | -r | --invert] [--] <cmd>...
>  	use <cmd>... to automatically bisect.

I'd suggest dropping "-r", which has little connection to "--invert".

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

Let's not make the style violations even worse.  Ideally, a
preliminary clean-up patch to fix the existing ones before the main
patch would be a good idea (cf. Documentation/CodingGuidelines).

It is more efficient and conventional (hence easier to read) to
reuse the single "case" you would need to write anyway for the loop
control, i.e.

	while :
	do
		case "$1" in
                --) ... ;;
                --invert | ... ) ... ;;
                -*) die "unknown command ;;
		*) break ;;
		esac
	done

> +		--expect=*)
> +			# how to localize part 2?

Using things like "%2$s", you mean?

As I alluded earlier, it is unclear how this new feature should
interact with the "we use 'xyzzy' to mean 'good', and 'frotz' to
mean 'bad'" feature.  One part of me would want to say that when
running bisect with good and bad swapped, we should reverse the
polarity of "bisect run" script automatically, but the rest of me
of course would say that it would not necessarily be a good idea,
and it is of course a huge backward incompatible change, so anything
automatic is a no go.

> @@ -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"

Is this a leftover debugging output?

In any case, I wonder why something along the line of the attached
patch is not sufficient and it needs this much code.

 git-bisect.sh | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/git-bisect.sh b/git-bisect.sh
index efee12b8b1..7fc4f9bd8f 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -247,6 +247,15 @@ bisect_run () {
 		"$@"
 		res=$?
 
+		if test -n "$invert_run_status"
+		then
+			case "$res" in
+			0)	res=1 ;;
+			125)	;;
+			*)	res=0 ;;
+			esac
+		fi
+
 		# Check for really bad run error.
 		if [ $res -lt 0 -o $res -ge 128 ]
 		then

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

[Stephen Oberholtzer]

Jonathan,

Thanks for your feedback!

On Fri, Jan 3, 2020 at 8:22 PM Jonathan Nieder <jrnieder@gmail.com> wrote:
>
> (+cc: Christian Couder, bisect expert)
(noted)

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

Done!  That one was simple enough I didn't tag it RFC.

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

Of course; I was just trying to be thorough.

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

That's not quite what I had here (although it touches on another idea
I've been mulling over.)
My patch adds "--success=<term>", meaning

git bisect run --success=foo ./myscript.sh

does: run myscript.sh, and if the script succeeds -- that is, exits
with status 0 -- then do 'git bisect foo'; otherwise, do whatever the
other one is.

WIthout any specialized options, the behavior today is equivalent to
"--success=good" (where status 0 means 'git bisect good', otherwise
'git bisect bad'.)

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

I actually had this detail spelled out in the blurb I had been writing
in Documentation/git-bisect.txt:

+The "good/bad" interpretation can be reversed by specifying -r,
+--invert, or --success=bad (or, if the bisect was started with
+--term-bad specified, the appropriate string.) In such circumstances,
+an exit code of 0 is taken to mean 'bad', while an exit code of 1-124
+or 126-127 is treated as 'good'.  The meaning of all other exit codes
+is unaffected by this option.

That's why my initial version called it "expect": my thinking was that
I was "expecting" the script to say that the version was bad, rather
than that it was good.

> - 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"?
>
>   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?

"--invert-status" would work for me (or "--negate-status", perhaps?)

>
> > You're not allowed to specify more than one expectation.
>
> Usual convention would be "last specified option wins".

That's fine, I can make such a thing work.

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

I'd started writing them even before you replied; I didn't want to
write *too* much and have to do a lot of replacing on option names.

> > +     while [ "$1" != "${1#-}" ]; do
>
> Might be simpler to do 'while true', and in the *) case to break.

I went with `test -n "$1"` after Junio pointed me at the coding guidelines :)

> It's more idiomatic to use eval_gettext here.  See
> "git grep -e die -- '*.sh'" for some examples.

Yeah, I hadn't yet learned about about eval_gettext before sending the
patch.  Once I saw it referenced in other code, it was easy to make
happen.

> "set -e" has
> enough exceptions that it hasn't been worth using for this.

Exceptions? Like what? As my comment pointed out, I don't know how the
test suite managed to make 'set -e' _not_ work (although I can see
that it has, in fact, managed this.) In fact, I suspect the fact that
it managed it is actually a bug in dash.

> The test
> harness is able to detect a missing "&&" so this is less error-prone
> than it sounds.)

'set -e' should be functionally equivalent to using '&&' between all
pipelines, except it also handles loops.
(also, all of those &&s are awkward to write.)

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

Well, yeah, if there was a simple shell construct to use instead of !,
I wouldn't have bothered to code this up in the first place :)

Here's what got me started on this stuff in the first place:

I was trying to bisect an kernel issue I was having. However, I had
two problems: (a) the affected machine is not powerful enough to build
a kernel in less than a day, and (b) my most reliable way to detect
the issue had to be initiated from an external system.

I wrote a script that would: build the kernel, install it onto
affected machine, boot new kernel on affected machine, and run some
tests to see if the problem occurs.

There were four possible outcomes from each attempt:

1. Kernel fails to build or boot: correct action is 'git bisect skip'
(this was mapped to exit code 125)
    (note that, since there was no way to automatically determine if
the kernel booted correctly, that situation actually manifested as
case 4)
2. Problem is not present: correct action is 'git bisect good' (mapped
to exit code 0)
3. Problem is present: correct action is 'git bisect bad' (mapped to
exit code 99)
    (also: need to reboot the machine back to a working kernel.)
4. Something unexpected went wrong: correct action is to hit the
emergency stop and let me fix it manually (mapped to exit code 255)

Situation #4 probably happened about five or six times, in all. (One
memorable instance was during the 16th or 17th revision, when the
target machine ran out of disk space while copying the new kernel
over.  A couple of times, ssh had an error trying to connect.  And
several times, for reasons beyond my understanding, the 255 failsafe
kicked in on exit code 99.)

I had to map a _lot_ of failure cases to 255. In essence, I did this:
(That's why I used 99 to indicate failure; it was unlikely to produce
a false positive.)  I used a bunch of ultimately-not-very-reliable
techniques to try and map all exit codes other than 0, 99, or 125 to
255.  I'm thinking of adding a "--fail-status=" option, which would
let you specify the explicit status code or codes to treat as 'bad',
with all other nonzero codes being hard errors and causing 'run' to
bail immediately.  Such a thing would have made several aspects of my
script far less error-prone.

It took most of a day (much better than most of a month) but I found
the offending commit.

Later, I noticed that, in one of the recent -rc builds, the problem no
longer seemed to manifest.  But then the question became: was the
problem truly fixed, or was the problem being masked otherwise?
That's when I discovered that properly adjusting the exit status from
my script was... complicated: I needed to reverse the behavior for
cases 2 and 3 above, but leave the behavior for cases 1 and 4
unchanged. And I figured that, if I had this problem, someone else
surely did as well.

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

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

[Stephen Oberholtzer]

Junio,

Thank you for your feedback! Several of your concerns were already
raised by Jonathan Nieder; I'm going to focus on the specifics you
brought up that Jonathan didn't, to avoid repeating myself too much.

On Fri, Jan 3, 2020 at 10:27 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Hmm.
>
> No off-the-shelf tool I know of exits 125 to signal "I dunno", so if
> you have to care about the special status 125, the "command" you are
> driving with "git bisect run" must be something that was written
> specifically to match what "git bisect" expects by knowing that 125
> is a special code to declare that the commit's goodness cannot be
> determined.  Now, what's the reason why this "command" written
> specifically to be used with "git bisect", which even knows the
> special 125 convention, yields "this is good" in the wrong polarity?
>
> The only realistic reason I can think of is when the user is hunting
> for a commit that fixed what has long been broken.  In such a use
> case, commits in the older part of the history would not pass the
> test (i.e. the exit status of the script would be non-zero) while
> commits in the newer part of the history would.

That's nearly exactly what happened: after bisecting the problem to
find when it was introduced, I found that an updated version did not
fail the test.  I wanted to bisect the "fix" in order to review the
change and verify that it did, in fact, fix the problem -- as opposed
to change things so my test didn't fail (as it turned out, it was the
latter: rather than fix the issue, the "fix" commit simply turned the
bugged feature off by default.)

> I'd suggest dropping "-r", which has little connection to "--invert".

I was simply trying to be thorough, so if it doesn't need a short
name, that's fine by me.  (And it's probably going to be
--invert-status.)

> Let's not make the style violations even worse.

Jonathan pointed that one out; you can see my corrected version in my
(at his suggestion) split-out version that simply adds support for
'run -- cmd'

> > +                     # how to localize part 2?
>
> Using things like "%2$s", you mean?

Hah, yeah, that was a leftover comment from before I came up with the
'$(printf $(gettext))' trick.  (Which is now eval_gettext.)

> As I alluded earlier, it is unclear how this new feature should
> interact with the "we use 'xyzzy' to mean 'good', and 'frotz' to
> mean 'bad'" feature. One part of me would want to say that when
> running bisect with good and bad swapped, we should reverse the
> polarity of "bisect run" script automatically, but the rest of me
> of course would say that it would not necessarily be a good idea,
> and it is of course a huge backward incompatible change, so anything
> automatic is a no go.

This new feature works just fine with that (in fact, the last round of
tests in the test script explicitly covers this interaction.)
With "--invert-status" it doesn't matter what your names are, and with
"--success=<term>" it honors whatever terms you specified.

> > +             echo "exit code $res means this commit is $state"
>
> Is this a leftover debugging output?

Yep, missed that one during my cursory review before uploading (since
test scripts hide their output by default, I didn't realize I'd left
it in there.)

> In any case, I wonder why something along the line of the attached
> patch is not sufficient and it needs this much code.
>
>  git-bisect.sh | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index efee12b8b1..7fc4f9bd8f 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -247,6 +247,15 @@ bisect_run () {
>                 "$@"
>                 res=$?
>
> +               if test -n "$invert_run_status"
> +               then
> +                       case "$res" in
> +                       0)      res=1 ;;
> +                       125)    ;;
> +                       *)      res=0 ;;
> +                       esac
> +               fi
> +

Unfortunately, that doesn't behave properly.
As far as 'git bisect run' is concerned, there are four distinct sets
of status codes:

1. Test passed (translated to 'git bisect good') -- status 0
2. Test failed (translated to 'git bisect bad') -- status 1-124 or 126-127
3. Untestable (translated to 'git bisect skip') -- status 125
4. Malfunction (causes 'git bisect run' to halt immediately, leaving
the bisection incomplete) -- anything else

What needs to happen is that status code lists for "test passed" (#1)
and "test failed" (#2) are swapped; even when bisecting a fix, #3
(untestable) and #4 (malfunction) remain unchanged.  Your patch remaps
case #4 to case #1.

(I'm actually going to put together a patch to allow the user to pare
down the exit code list for #2 to a specific list, to make 'run' less
dicey in the face of complex test requirements.)

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

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

[Junio C Hamano]

Stephen Oberholtzer <stevie@qrpff.net> writes:

>> In any case, I wonder why something along the line of the attached
>> patch is not sufficient and it needs this much code.
>> ...
> Unfortunately, that doesn't behave properly.
> As far as 'git bisect run' is concerned, there are four distinct sets
> ...
> What needs to happen is that status code lists for "test passed" (#1)
> and "test failed" (#2) are swapped; even when bisecting a fix, #3
> (untestable) and #4 (malfunction) remain unchanged.  Your patch remaps
> case #4 to case #1.

Yeah, I know.  I didn't mean to give you a perfect solution and that
was why I said "along the line of...".  I know I ignored the 128 and
above, as I usually trust that our contributors are competent enough
to be able to fill in the missing details given an outline.

The key takeaway I wanted you to notice was that a single case
statement that maps the exit code external command would give us
would look sufficient, without any of the {SUCCESS,FAIL}_TERM magic
you had in your version, which indicates that there is more than the
simple "using a run script to find where a bug was fixed can be done
by swapping exit code" going on.  And it is quite unclear why that
is needed either from the patch or the text that accompanied the
patch.

Thanks.

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

[Stephen Oberholtzer]

Junio,

On Sun, Jan 5, 2020 at 8:16 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Yeah, I know.  I didn't mean to give you a perfect solution and that
> was why I said "along the line of...".  I know I ignored the 128 and
> above, as I usually trust that our contributors are competent enough
> to be able to fill in the missing details given an outline.
>
> The key takeaway I wanted you to notice was that a single case
> statement that maps the exit code external command would give us
> would look sufficient, without any of the {SUCCESS,FAIL}_TERM magic
> you had in your version, which indicates that there is more than the
> simple "using a run script to find where a bug was fixed can be done
> by swapping exit code" going on.  And it is quite unclear why that
> is needed either from the patch or the text that accompanied the
> patch.


In this particular instance at least, I'm not competent enough to come
up with a clean, portable way to create a single case statement that
handles the final condition.
The issue I'm having is that case-esac blocks do string matching, not
integer value matching, so I don't know how to replicate the current
behavior without a case pattern that looks like
[12]|1[3-9]|1[01][0-9]|12[0-467]|1[3-9]|2[0-9]|[3-9].



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

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

[Junio C Hamano]

Stephen Oberholtzer <stevie@qrpff.net> writes:

> ... a clean, portable way to create a single case statement that
> handles the final condition.

Hmm, don't we want to say (1) original 0 (success) maps to 1 (2)
original 125 maps to 125 (3) anything below 128 in the original maps
to 0 and (4) everything else is left intact?  Perhaps something as
simple like this, taking advantage of the fact that $? won't have
anything other than digits?

    ... do something ...
    status=$?

    case $status in
    0) status=1 ;;
    125) : as-is ;;
    ? | ?? | 1[01]? | 12[0-7]) status=0;;
    *) : as-is ;;
    esac

I am still unclear where the need for {SUCCESS_FAIL}_TERM magic
comes from, though.