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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 15, 2016, 21:07 UTC
Message-ID: <xmqqpotqa8de.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1460294354-7031-6-git-send-email-s-beyer@gmx.net>

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

Show 12 quoted lines

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

Show 8 quoted lines

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

Show 6 quoted lines

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

Show 5 quoted lines

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

Show 13 quoted lines

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