Web lists-archives.com

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

On Mon, Oct 29, 2018 at 3:14 PM Jeff King <peff@xxxxxxxx> 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,


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?

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.

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