Re: [RFC PATCH v5] Implement --first-parent for git rev-list --bisect

From: Christian Couder <christian.couder@gmail.com>
Date: Jun 26, 2018, 11:59 UTC
Message-ID: <CAP8UFD3oEjW75qsk4d_wqo2V8PmzMvZLshutw20CD7AU4b4ocg@mail.gmail.com>
In-Reply-To: <xmqq4lhqpy80.fsf@gitster-ct.c.googlers.com>

On Mon, Jun 25, 2018 at 7:33 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 29 quoted lines

> Tiago Botelho <tiagonbotelho@gmail.com> writes:
>
>> +test_expect_success "--bisect-all --first-parent" '
>> +cat >expect1 <<EOF &&
>> +$(git rev-parse CC) (dist=2)
>> +$(git rev-parse EX) (dist=1)
>> +$(git rev-parse D) (dist=1)
>> +$(git rev-parse FX) (dist=0)
>> +EOF
>> +
>> +cat >expect2 <<EOF &&
>> +$(git rev-parse CC) (dist=2)
>> +$(git rev-parse D) (dist=1)
>> +$(git rev-parse EX) (dist=1)
>> +$(git rev-parse FX) (dist=0)
>> +EOF
>> +
>> +git rev-list --bisect-all --first-parent FX ^A >actual &&
>> +  ( test_cmp expect1 actual || test_cmp expect2 actual )
>> +'
>
> I hate to say this, but the above looks like a typical
> unmaintainable mess.
>
> What happens when you or somebody else later needs to update the
> graph to be tested to add one more commit (or even more)?  Would it
> be enough to add another "rev-parse" plus "dist=X" line in both
> expects?  Or do we see a trap for combinatorial explosion that
> requires us to add new expect$N?

What about the following then:

test_dist_order () {
    file="$1"
    n="$2"
    while read -r hash dist
    do
        d=$(echo "$dist" | sed -e "s/(dist=\(.*\))/\1/")
        case "$d" in
            ''|*[!0-9]*) return 1 ;;
            *) ;;
        esac
        test "$d" -le "$n" || return 1
        n="$d"
    done <"$file"
}

test_expect_success "--bisect-all --first-parent" '
    cat >expect <<EOF &&
$(git rev-parse CC) (dist=2)
$(git rev-parse EX) (dist=1)
$(git rev-parse D) (dist=1)
$(git rev-parse FX) (dist=0)
EOF
    sort expect >expect_sorted &&
    git rev-list --bisect-all --first-parent FX ^A >actual &&
    sort actual >actual_sorted &&
    test_cmp expect_sorted actual_sorted &&
    test_dist_order actual 2
'

This feels overkill to me, but it should scale if we ever make more complex tests.