git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Christian Couder <christian.couder@gmail.com>
Cc: git <git@vger.kernel.org>, Jeff King <peff@peff.net>,
	Christian Couder <chriscool@tuxfamily.org>
Subject: Re: [PATCH v2 2/2] ref-filter: add support for %(contents:size)
Date: Tue, 07 Jul 2020 08:28:21 -0700	[thread overview]
Message-ID: <xmqqr1tn5oq2.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <CAP8UFD1JiBOhpXeADObFBgoGm292dxQ933TENrZNPnmSv+SbUg@mail.gmail.com> (Christian Couder's message of "Tue, 7 Jul 2020 10:40:10 +0200")

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

>> 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.

> --- 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.

  reply	other threads:[~2020-07-07 15:31 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-07-02 14:08 [PATCH v2 0/2] Add support for %(contents:size) in ref-filter Christian Couder
2020-07-02 14:08 ` [PATCH v2 1/2] Documentation: clarify %(contents:XXXX) doc Christian Couder
2020-07-06 21:34   ` Junio C Hamano
2020-07-02 14:08 ` [PATCH v2 2/2] ref-filter: add support for %(contents:size) Christian Couder
2020-07-06 21:44   ` Junio C Hamano
2020-07-07  8:40     ` Christian Couder
2020-07-07 15:28       ` Junio C Hamano [this message]
2020-07-07  5:02 ` [PATCH v2 0/2] Add support for %(contents:size) in ref-filter Jeff King
2020-07-07  6:19   ` Christian Couder
2020-07-08  4:43     ` Jeff King

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqr1tn5oq2.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=chriscool@tuxfamily.org \
    --cc=christian.couder@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).