Re: [PATCH v5 6/6] ref-filter.c: parse trailers arguments with %(contents) atom

From: Jeff King <peff@peff.net>
Date: Oct 2, 2017, 05:03 UTC
Message-ID: <20171002050351.imuntcy3xnq4wxux@sigill.intra.peff.net>
In-Reply-To: <20171002003304.77514-6-me@ttaylorr.com>

On Sun, Oct 01, 2017 at 05:33:04PM -0700, Taylor Blau wrote:

Show 15 quoted lines

> The %(contents) atom takes a contents "field" as its argument. Since
> "trailers" is one of those fields, extend contents_atom_parser to parse
> "trailers"'s arguments when used through "%(contents)", like:
> 
>   %(contents:trailers:unfold,only)
> 
> A caveat: trailers_atom_parser expects NULL when no arguments are given
> (see: `parse_ref_filter_atom`). This is because string_list_split (given
> a maxsplit of -1) returns a 1-ary string_list* containing the given
> string if the delimiter could not be found using `strchr`.
> 
> To simulate this behavior without teaching trailers_atom_parser to
> accept strings with length zero, conditionally pass NULL to
> trailers_atom_parser if the arguments portion of the argument to
> %(contents) is empty.

Doh, that string_list behavior is what I was missing in my earlier comments. I agree this is probably the best way of doing it. I'm tempted to say that parse_ref_filter_atom() should do a similar thing. Right now we've got:

  $ git for-each-ref --format='%(refname)' | wc
     2206    2206   79929
  $ git for-each-ref --format='%(refname:short)' | wc
     2206    2206   53622
  $ git for-each-ref --format='%(refname:)' | wc
  fatal: unrecognized %(refname:) argument:
      0       0       0

which seems a bit unfriendly. As we're on v6 here, I don't want to suggest it as part of this series. But maybe a #leftoverbits candidate, if others agree it's a good direction.

-Peff