Re: [PATCH v4 2/7] pretty: allow %(trailers) options with explicit value

From: Jeff King <peff@peff.net>
Date: Jan 29, 2019, 16:55 UTC
Message-ID: <20190129165523.GA7349@sigill.intra.peff.net>
In-Reply-To: <87o99iwmjn.fsf@0x63.nu>

On Tue, Dec 18, 2018 at 10:30:04PM +0100, Anders Waldenborg wrote:

Show 12 quoted lines

> 
> Junio C Hamano writes:
> > That way, we can handle %(trailers:only=bogo) more sensibly,
> > no?  Syntactically we can recognize that the user wanted to give
> > 'bogo' as the value to 'only', and say "'bogo' is not a boolean" if
> > we did so.
> 
> I agree that proper error reporting for the pretty formatting strings
> would be great. But that would depart from the current extremely crude
> error handling where incorrect formatting placeholders are just left
> unexpanded. How would such change in error handling be done safely, wrt
> backwards compatibility changes?

I think we'd want to move in the direction of enforcing valid expressions for %(foo) placeholders. There's some small value in leaving %X alone if we do not understand "X" (not to mention the backwards %compatibility you mentioned), but I think %() is a pretty deliberate indication that a placeholder was meant there.

We already do this for ref-filter expansions:

  $ git for-each-ref --format='%(foo)'
  fatal: unknown field name: foo

We don't for "--pretty" formats, but I do wonder if anybody would be really mad (after all, we have declared ourselves free to add new placeholders, so such formats are not future-proof).

-Peff