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

[Junio C Hamano <gitster@pobox.com>]

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