Re: [PATCH 02/16] bisect: add test for the bisect algorithm

From: Christian Couder <christian.couder@gmail.com>
Date: Feb 27, 2016, 11:40 UTC
Message-ID: <CAP8UFD27f3zmrLrvyCuMfs6ijt7MtLB8rX0Ykvfar3kidpm6LQ@mail.gmail.com>
In-Reply-To: <56D0C5E0.2020703@gmx.net>

Hi Stephan,

On Fri, Feb 26, 2016 at 10:38 PM, Stephan Beyer <s-beyer@gmx.net> wrote:

Show 13 quoted lines

> Hi Christian,
>
> On 02/26/2016 07:53 AM, Christian Couder wrote:
>>> +test_expect_success 'bisect algorithm works in linear history with an odd number of commits' '
>>> +       git bisect start A7 &&
>>> +       git bisect next &&
>>> +       test "$(git rev-parse HEAD)" = "$(git rev-parse A3)" \
>>> +         -o "$(git rev-parse HEAD)" = "$(git rev-parse A4)"
>>
>> I thought that we should not use "-o" and "-a" but instead "|| test"
>> and "&& test".
>
> Why is this?

I think it is because it might not be very portable, but I am not sure I remember well the previous discussions about this.

Show 10 quoted lines

> I understand the && instead of -a thing (test atomicity),
> however, for || this results in an ugly
>
> +       git bisect next &&
> +       ( test "$(git rev-parse HEAD)" = "$(git rev-parse A3)" ||
> +         test "$(git rev-parse HEAD)" = "$(git rev-parse A4)" )
>
> Right? (Otherwise a failure of e.g. "git bisect start A7" would run
> the command after || (which may still be fine in some cases but is wrong
> in most of the other cases).

Yeah.

By the way maybe t6030-bisect-porcelain.sh is paranoid, but it does some of those checks like:

    HASH4=$(git rev-parse --verify HEAD)

and later:

    rev_hash4=$(git rev-parse --verify HEAD) &&
    test "$rev_hash4" = "$HASH4" &&

So it uses "--verify" and also often puts the "git rev-parse" on a separate line...

Show 12 quoted lines

> However, what do you think about this?
>
> diff --git a/t/t8010-bisect-algorithm.sh b/t/t8010-bisect-algorithm.sh
> index bda59da..ae50e7c 100755
> --- a/t/t8010-bisect-algorithm.sh
> +++ b/t/t8010-bisect-algorithm.sh
> @@ -8,6 +8,16 @@ exec </dev/null
>
>  . ./test-lib.sh
>
> +test_compare_rev () {
> +       arg="$(git rev-parse "$1")"

...hence I would suggest:

    arg="$(git rev-parse --verify "$1")" || return

> +       shift
> +       for rev
> +       do
> +               test "$arg" = "$(git rev-parse "$rev")" && return 0

...and:

        hash="$(git rev-parse --verify "$rev")" || return
        test "$arg" = "$hash" && return 0

> +       done
> +       return 1
> +}

Otherwise I like test_compare_rev().

Thanks, Christian.