Re: [PATCH v2] t6030: add test for git bisect skip started with --term* arguments

From: Christian Couder <christian.couder@gmail.com>
Date: Apr 28, 2021, 08:11 UTC
Message-ID: <CAP8UFD0k9fwwW94BKtMPLmA=Qx1ce3Qh3UfF0KiC5+AOj-=MiA@mail.gmail.com>
In-Reply-To: <20210425080508.154159-1-bagasdotme@gmail.com>

On Sun, Apr 25, 2021 at 10:06 AM Bagas Sanjaya <bagasdotme@gmail.com> wrote:

Show 10 quoted lines

>
> Trygve Aaberge reported [1] git bisect breakage when the bisection
> is started with --term* arguments (--term-new and --term-old). In that
> case, skipping with `git bisect skip` should cause HEAD to be changed,
> but actually the HEAD stayed same after skipping.
>
> Let's add the test to catch the breakage. Because there isn't any fixes
> yet, mark the test as test_expect_failure.
>
> [1]: https://lore.kernel.org/git/20210418151459.GC10839@aaberge.net/

We prefer when people can give a proper explanation of what happens in the commit message, instead of providing a link to such an explanation. Here it's not so important if this patch (which only adds tests) is merged along with (or if it's squashed into) the fix which hopefully contains an explanation. Maybe if both patches are sent in the same patch series the commit message might want to only say something like "A previous commit fixed a `git bisect skip` breakage when the bisection is started with --term* arguments, let's add a test for that breakage."

A commit message trailer might be the right way to credit Trygve Aaberge (also please don't forget to put the reporter in Cc like I am doing), for example like:

Reported-by: Trygve Aaberge <trygveaa@gmail.com>

Show 10 quoted lines

> Signed-off-by: Bagas Sanjaya <bagasdotme@gmail.com>
> ---
>  Changes from v1 [1]:
>
>    * Move the test to the the end of test script (test 74).
>      v1 placed the test as test 26, and when I run the script, there
>      are 9 more failed tests rather than just one (because such tests
>      depended on previous ones). Now the test is placed together with
>      similar tests (git bisect with --terms*).
>    * Begin the test with git bisect reset, as with other nearby tests.

Nice!

Show 18 quoted lines

>  [1]: https://lore.kernel.org/git/20210423070308.85275-1-bagasdotme@gmail.com/
>
>  t/t6030-bisect-porcelain.sh | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
>
> diff --git a/t/t6030-bisect-porcelain.sh b/t/t6030-bisect-porcelain.sh
> index 32bb66e1ed..b1b847ebbc 100755
> --- a/t/t6030-bisect-porcelain.sh
> +++ b/t/t6030-bisect-porcelain.sh
> @@ -922,6 +922,19 @@ test_expect_success 'bisect start takes options and revs in any order' '
>         test_cmp expected actual
>  '
>
> +# Bisect is started with --term-new and --term-old arguments,
> +# then skip. The HEAD should be changed.
> +# FIXME: Mark this test as test_expect_failure. Remove the FIXME and
> +# mark as test_expect_success when this test successes (fixed bug).
> +test_expect_failure '"bisect skip: bisection is started with --term*"' '

I am not sure why there are double quotes inside simple quotes around the name of the test. Aren't simple quotes enough?

Also what about a simpler name like:

test_expect_failure 'bisect skip works with --term*' '

Show 6 quoted lines

> +       git bisect reset &&
> +       git bisect start --term-new=fixed --term-old=unfixed HEAD $HASH1 &&
> +       HASH_SKIPPED_FROM=$(git rev-parse --verify HEAD) &&
> +       git bisect skip &&
> +       HASH_SKIPPED_TO=$(git rev-parse --verify HEAD) &&
> +       test $HASH_SKIPPED_FROM != $HASH_SKIPPED_TO

It might be a bit safer and more consistent with the rest of this test script to use double quotes around $HASH_SKIPPED_FROM and $HASH_SKIPPED_TO, like:

       test "$HASH_SKIPPED_FROM" != "$HASH_SKIPPED_TO"

Thanks, Christian.