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

On Mon, Oct 02, 2017 at 01:03:51AM -0400, Jeff King wrote:

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

quoted

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.

I think #leftoverbits is fine here, but I think addressing this before
2.15 is reasonable. I can take a look at this in a future patch series.

--
- Taylor