Re: [PATCH] pretty: Add %(trailer:X) to display single trailer

From: Anders Waldenborg <anders@0x63.nu>
Date: Oct 29, 2018, 17:05 UTC
Message-ID: <CADsOX3Cbn7jjqFERptxMm59mn0qYnkf9bmFvJS20VBPedZHwqQ@mail.gmail.com>
In-Reply-To: <20181029141402.GA17668@sigill.intra.peff.net>

On Mon, Oct 29, 2018 at 3:14 PM Jeff King <peff@peff.net> wrote:

> Junio's review already covered my biggest question, which is why not
> something like "%(trailers:key=ticket)". And likewise making things like
> comma-separation options.

Jeff, Junio,

thanks!

Your questions pretty much matches what I (and a colleague I discussed this with before posting) was concerned about.

My first try actually had it as an option to "trailers". But it got a bit messy with the argument parsing, and the fact that there was a fast path making it work when only specified. I did not want to spend lot of time reworking fixing that before I had some feedback, so I went for a smallest possible patch to float the idea with (a patch is worth a 1000 words).

I'll start by reworking my patch to handle %(trailers:key=X) (I'll assume keys never contain ')' or ','), and ignore any formatting until the way forward there is decided (see below).

> But my second question is whether we want to provide something more
> flexible than the always-parentheses that "%d" provides. That has been a
> problem in the past when people want to format the decoration in some
> other way.

Maybe just like +/-/space can be used directly after %, a () pair can be allowed.. E.g "%d" would just be an alias for "%()D", and for trailers it would be something like "%()(trailers:key=foo)"

There is another special cased placeholder %f (sanitized subject line, suitable for a filename). Which also could be changed to be a format specifiier, allowing sanitize any thing, e.g "%!an" for sanitized author name.

Is even the linebreak to commaseparation a generic thing? "% ,()(trailers:key=Ticket)" it starts go look a bit silly.

Then there are the padding modifiers. %<() %<|(). They operate on next placeholder. "%<(10)%s" Is that a better syntax? "%()%(trailers:key=Ticket,comma)"

I can also imagine moving all these modifiers into a generic modifier syntax in brackets (and keeping old for backwards compat) %[lpad=10,ltrunc=10]s == %<(10,trunc)%s %[nonempty-prefix="%n"]GS == %+GS %[nonempty-prefix=" (",nonempty-suffix=")"]D == %d Which would mean something like this for tickets thing: %[nonempty-prefix=" (Tickets:",nonempty-suffix=")",commaseparatelines](trailers:key=Ticket,nokey) which is kinda verbose.

Show 5 quoted lines

> We have formatting magic for "if this thing is non-empty, then show this
> prefix" in the for-each-ref formatter, but I'm not sure that we do in
> the commit pretty-printer beyond "% ". I wonder if we could/should add a
> a placeholder for "if this thing is non-empty, put in a space and
> enclose it in parentheses".

Would there be any interest in consolidating those formatters? Even though they are totally separate beasts today. I think having all attributes available on long form (e.g "%(authorname)") in addition to existing short forms in pretty-formatter would make sense.

 anders