Re: Fwd: [PATCH 2/2] pretty.c: allow date formats in user format strings

[Will Palmer <wmpalmer@gmail.com>]

On Mon, 2011-03-07 at 11:17 -0500, Jeff King wrote:
> On Sun, Mar 06, 2011 at 09:54:01PM +0000, Will Palmer wrote:
> 
> > On Sat, Mar 5, 2011 at 8:00 PM, Jeff King <peff@peff.net> wrote:
> > > You can now do "%ad(short)" or similar (using any format
> > > that works for --date). This makes some formats like %aD
> > > redundant (since you can do "%ad(rfc)"), but of course we
> > > keep them for compatibility.
> > >
> > 
> > The more I see long formats like this, the more I think it would make
> > sense to make formats %(likeThis), the way for-each-ref does.
> > Ideally, these formats could even be unified, at some point.
> 
> Yeah, I totally agree. One problem is that everytime an extended format
> comes up it gets bikeshedded to death as everybody mentions their
> favorite format and/or feature, and then nobody codes it.
> 
> > I tried this a long while ago, as part of my attempt to make all
> > pre-defined formats work in terms of format strings, but that turned
> > into too much of a bloated mess to bother submitting. I don't know
> > if there's enough interest in such a thing to justify trying again (or to
> > justify rebasing the bloated version, cleaning it up and submitting it
> > as-is, for that matter)
> 
> I think there is interest. I'd be curious to see what you have. A few
> days ago, when working on this series, I tried to make a
> minimally-invasive change to allow "%(ad)" to work alongside "%ad", with
> a generic arguments format like %(ad:flag:key=value). Which would allow
> existing shorthand, for-each-ref-style %(refname:short), and leave room
> for arbitrary extension of each placeholder (alongside more
> human-readable placeholder names).
> 
> The problem I ran into was the internal code interface. We parse the
> format string each time we expand it. This works OK for simple
> printf-like stuff. But ideally we can handle something like:
>   %(ad:key=embedded\:colon:key2=embedded\)paren)
> 
> It's hard to make a nice interface to that which doesn't involve copying
> the quoted string out into a non-quoted version. But we don't want to be
> doing a bunch of parsing and allocation per-expansion. It's slow, and
> this expansion happens inside a fairly tight loop in many cases (e.g.,
> during rev-list).

Exactly the problem I ran into.

> 
> So I think the whole thing needs to be factored into two phases: a
> parsing phase where we build some internal parse tree, and then an
> expansion phase where we walk the parse tree for each commit (or ref, or
> whatever is being expanded).

And exactly the solution I implemented.
At the time, it felt like needless bloat, but perhaps the problem has
gotten to the point where it's worth it.

I assume rebasing what I have right now would be problematic, but it
sounds like it's about time to give it another go.

The code was ever only in a "proof of concept" stage- I had it working
for single revisions, but in a way which wasn't yet compatible with any
of the other parts of log, iirc.

I'll try getting a rebase started tonight, but in the mean time
I /think/ the latest code is at 
https://github.com/wpalmer/git/tree/pretty/parse-format-poc

Warning: quite ugly.

If you have comments, I would not mind hearing them (though off-list
might be better)

> 
> > Point is: we're going to keep having more and more format options,
> > I think that's a given. At some point, these short mnemonics will just
> > stop making sense, and it makes sense to have an escape plan when
> > that happens.
> 
> Agreed. And I think it is possible to do it in a backwards-compatible
> way; support %(longname:options) for everything, and keep short-hands
> like %h and %ad for existing elements without options.
> 
> -Peff