Re: [PATCH v2 2/2] ref-filter: add support for %(contents:size)

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 7, 2020, 15:28 UTC
Message-ID: <xmqqr1tn5oq2.fsf@gitster.c.googlers.com>
In-Reply-To: <CAP8UFD1JiBOhpXeADObFBgoGm292dxQ933TENrZNPnmSv+SbUg@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 8 quoted lines

>> I am fine if it just is silently ignored (which is consistent with
>> already existing behaviour of other requests that do not make sense
>> for the given type) if the thing is a blob or a tree, but we'd need
>> to cover the case with a test or two.  It seems you only expect this
>> with a tag object and do not have any test that checks for other
>> types of objects?
>
> My patch already adds 1 test with a commit:

But that is not an interesting case, no? Unless I missed that there were new tests to see what happens with a blob and a tree, that is.

Show 14 quoted lines

> --- a/t/t6300-for-each-ref.sh
> +++ b/t/t6300-for-each-ref.sh
> @@ -125,6 +125,7 @@ test_atom head contents:body ''
>  test_atom head contents:signature ''
>  test_atom head contents 'Initial
>  '
> +test_atom head contents:size '8'
>  test_atom head HEAD '*'
>
> There is only one test with a commit, because that's already the case
> for %(contents) too.
>
> I am ok with adding another preparatory patch to the series that would
> add a few more test cases with commits, trees and blobs though.

I am OK either way, because I know fairly well how these formatting atom system works (I had heavy involvement in the initial one) and I know it would be hard to make it do nonsensical things when given an object of an unexpected type.

But I was hoping some among us experienced contributors would lead beginners by example of making sure we test both positive ("see, my shiny new toy does work") and negative ("and it won't do nonsensical things when given an input it is not designed to work with") cases.

Thanks.