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

From: Jeff King <peff@peff.net>
Date: Jan 19, 2018, 17:23 UTC
Message-ID: <20180119172353.GA5752@sigill.intra.peff.net>
In-Reply-To: <CAP8UFD1dcwEA9z+oQKFV=aFoKn73mtP4qkLGovW2XTu6N=N4dA@mail.gmail.com>

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

Show 6 quoted lines

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

Show 7 quoted lines

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