Re: [PATCH 02/27] bisect: Fixup bisect-porcelain/17

From: Taylor Blau <me@ttaylorr.com>
Date: Nov 18, 2021, 22:05 UTC
Message-ID: <YZbOKgoYmeM/yLAs@nand.local>
In-Reply-To: <20211118164940.8818-3-jack@suse.cz>

On Thu, Nov 18, 2021 at 05:49:15PM +0100, Jan Kara wrote:

> Test 17 from t6030-bisect-porcelain.sh assumes that bisection algorithm
> suggests first HASH3 where HASH2 and HASH3 are equivalent choices. Make
> sure test correctly handles both choices, add test variant to properly
> test commit skipping in the second case.

OK, makes sense-ish: at least in the context of preparing for the bisection algorithm to change. The subject line leaves a bit to be desired, though. Perhaps:

  t6030: handle equivalent bisection points gracefully

Show 10 quoted lines

> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index 1be85d064e76..f8cfdd3c36d2 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -197,11 +197,27 @@ test_expect_success 'bisect skip: successful result' '
>  	test_when_finished git bisect reset &&
>  	git bisect reset &&
>  	git bisect start $HASH4 $HASH1 &&
> -	git bisect skip &&
> +	if [ $(git rev-parse HEAD) == $HASH3 ]; then

This is somewhat uncommon style for Git's test suite. It might be more appropriate to write instead:

    if test "$HASH3" = "$(git rev-parse HEAD)"
    then
      git bisect skip
    fi &&
    # ...

> +# $HASH1 is good, $HASH4 is bad, we skip $HASH2
> +# but $HASH3 is good,

It looks like this comment should have gone above the start of the test in the previous hunk.

But it looks like you accidentally duplicated this test in its entirety (with the addition of the misplaced comment) below instead.

Thanks, Taylor