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

[Christian Couder <christian.couder@gmail.com>]

On Fri, Jan 19, 2018 at 6:23 PM, Jeff King <peff@peff.net> wrote:
> 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().

Yeah, ok, that makes sense.

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

Yeah, ok.