Re: [PATCH v2 01/11] t6301: for-each-ref tests for ref-filter APIs

From: karthik nayak <karthik.188@gmail.com>
Date: Jun 14, 2015, 08:32 UTC
Message-ID: <CAOLa=ZRC+mZmXKJnrDFdM9oy4+WrWC0rg7kj_bpNCf=iRSVG2g@mail.gmail.com>
In-Reply-To: <vpqvbeqd7y5.fsf@anie.imag.fr>

On Sun, Jun 14, 2015 at 1:42 PM, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:

Show 14 quoted lines

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> +test_expect_success 'setup some history and refs' '
>> +     test_commit one &&
>> +     test_commit two &&
>> +     test_commit three &&
>> +     git checkout -b side &&
>> +     test_commit four &&
>> +     git checkout master &&
>> +     git update-ref refs/odd/spot master
>
> I think you want more corner-cases here. For example,
> refs/headsfoo/master should not match refs/heads, and it's easy to write
> an incorrect implementation for this => test.

Like you mentioned below, currently it's redundant with t6300 and its purpose is to add tests for new functionality we provide to for-each-ref via ref-filter. This is done in the next few patches in this series.

Show 5 quoted lines

>
>> +'
>> +test_expect_success 'filtering by leading name' '
>
> Blank line between tests please.

Noted. Thanks.

Show 17 quoted lines

>
>> +     cat >expect <<-\EOF &&
>> +     refs/heads/master
>> +     refs/heads/side
>> +     EOF
>> +     git for-each-ref --format="%(refname)" refs/heads >actual &&
>> +     test_cmp expect actual
>> +'
>
> Isn't this test redundant with the content of t6300-for-each-ref.sh?
>
> At this point, you've done only internal refactoring, so you shouldn't
> need new tests (except to fix coverage holes in existing tests).
>
> I guess you're adding this to build more tests on top, but the commit
> message should clarify this.
>

I'll add a note to the commit message about how this test file is so that we can integrate more tests in the future for functionality given to for-each-ref via ref-filter APIs.

-- 
Regards,
Karthik Nayak