Stephan Beyer <s-be...@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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to