Re: [PATCH v2 05/21] t6030: generalize test to not rely on current implementation

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

Stephan Beyer <s-beyer@gmx.net> writes:

> The bisect algorithm allows different outcomes if, for example,
> the number of commits between a good and a bad commit is even.
> The current test relies on a specific behavior (for example,
> the behavior of the halfway() implementation). By disabling
> halfway(), some skip tests fail although the algorithm works.
>
> This commit generalizes the test t6030 such that it works
> even if the bisect algorithm uses its degree of freedom to
> choose another commit.
>
> While at it, fix some indentation issues: use tabs instead of
> 4 spaces.

While style fixes are very much welcome, it makes the patch
unnecessary noisy.  We typically do so as a preparatory clean-up.

And if you do style fixes, please fix other style issues, such as

 - use of "if [ ... ]; then", which should be spelled as

	if test ...
        then

 - unnecessasry space between redirection operator and the filename,
   and lack of double-quoting around such a filename in a variable
   to work around certain vintage of bash that gives unnecessary
   warnings, e.g. 'echo foo > $file' must be spelled as

	echo foo >"$file"

etc.

> @@ -84,9 +82,8 @@ test_expect_success 'bisect fails if given any junk instead of revs' '
>  
>  test_expect_success 'bisect reset: back in the master branch' '
>  	git bisect reset &&
> -	echo "* master" > branch.expect &&
>  	git branch > branch.output &&
> -	cmp branch.expect branch.output
> +	grep "^* master" branch.output

This is not a style fix, and it is not a "possibly multiple valid
outcomes", either.

If the purpose of change is "to do the right thing", checking the
output from "git symbolic-ref HEAD" against "refs/heads/master" is
the kosher way to check what test is trying to do.

> @@ -180,14 +175,15 @@ test_expect_success 'bisect start: no ".git/BISECT_START" if checkout error' '
>  	git checkout HEAD hello
>  '
>  
> -# $HASH1 is good, $HASH4 is bad, we skip $HASH3
> +# $HASH1 is good, monday is bad, we skip $HASH3

I am not sure this s/$HASH4/monday/ is adding value.  Certainly it
breaks consistency, which you could keep by defining SIDE_HASH5 or
something when you added the "Ok Monday, let's do it" commit.  On
the other hand, you could choose to consistently use branch-relative
names by turning $HASH3 to master~1, etc.

>  # but $HASH2 is bad,
>  # so we should find $HASH2 as the first bad commit
> ...

> +test_expect_success '"git bisect run" simple case' '
> +	echo "#"\!"/bin/sh" > test_script.sh &&
> +	echo "grep Another hello > /dev/null" >> test_script.sh &&
> +	echo "test \$? -ne 0" >> test_script.sh &&
> +	chmod +x test_script.sh &&

Use write_script in the "style fix" preparatory clean-up patch?

> +	git bisect start &&
> +	git bisect good $HASH1 &&
> +	git bisect bad $HASH4 &&
> +	git bisect run ./test_script.sh > my_bisect_log.txt &&
> +	grep "$HASH3 is the first bad commit" my_bisect_log.txt &&
> +	git bisect reset
> +'
> ...
> +test_expect_success '"git bisect run" with more complex "git bisect start"' '
> +	echo "#"\!"/bin/sh" > test_script.sh &&
> +	echo "grep Ciao hello > /dev/null" >> test_script.sh &&
> +	echo "test \$? -ne 0" >> test_script.sh &&
> +	chmod +x test_script.sh &&

Likewise.