Re: [PATCH v2 03/18] ref-filter: make valid_atom as function parameter

[Jeff King <peff@peff.net>]

On Fri, Jan 19, 2018 at 06:14:56PM +0100, Christian Couder wrote:

> > Let's discuss, what behavior we are waiting for
> > when atom seems useless for the command. Die or ignore?
> 
> We could alternatively just emit a warning, but it looks like there
> are a lot of die() calls already in ref-filter.c, so I would suggest
> die().

I actually think it makes sense to just expand nonsense into the empty
string, for two reasons:

  1. That's what ref-filter already does for the existing cases. For
     example, try:

       git for-each-ref --format='%(objecttype) %(authordate)'

     and you will see that the annotated tags just get a blank author
     field.

  2. I think we may end up with a case where we feed multiple objects
     via --batch-check, and the format is only nonsense for _some_ of
     them. E.g., I envision a world where you can do:

       git cat-file --batch-check='%(objecttype) %(refname)' <<-\EOF
       master
       12345abcde12345abcde12345abcde12345abcde
       EOF

     and get output like:

       commit refs/heads/master
       commit

     (i.e., if we would remember the refname discovered during the name
     resolution, we could still report it). It would be annoying if the
     second line caused us to die().

> > And, which
> > atoms are useless (as I understand, "rest" and "deltabase" from
> > cat-file are useless for all ref-filter users, so the question is
> > about - am I right in it, and about ref-filter atoms for cat-file).
> 
> For now and I think until the migration process is finished, you could
> just die() in case of any atom not already supported by the command.

I'm OK with dying in the interim if it's easier, though I suspect it is
not much extra work to just expand to the empty string in such cases. If
that's where we want to end up eventually, it may be worth going
straight there.

I also think %(deltabase) does make sense for anything that points to an
object. I suspect it's not all that _useful_ for for-each-ref, but that
doesn't mean we can't return the sensible thing if somebody asks for it.

I agree that %(rest) probably doesn't make any sense for a caller which
isn't parsing input.

-Peff