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

From: Will Palmer <wmpalmer@gmail.com>
Date: Mar 7, 2011, 18:50 UTC
Message-ID: <1299523834.1835.17.camel@walleee>
In-Reply-To: <1299518898.3024.10.camel@wpalmer.simply-domain>

On Mon, 2011-03-07 at 17:28 +0000, Will Palmer wrote:

Show 67 quoted lines

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

I'm home now, and apparently that should have been: https://github.com/wpalmer/git/tree/pretty/parse-format

I assume the code is very hard to follow, as it was pretty much written with the mindset of "get it done now, fix it later". Looking into it again, I see that part of the reason I abandoned it was not being able to determine a good way to split things into logical commits. It's almost entirely an "everything works or nothing works" change.

There was of course one section of it that I managed to split out, which is the "format aliases" code, already merged. I assume that this code has absolutely never been used since inclusion, as what it was actually intended to support was never finished.

To see it in action, try:
 ./git log --pretty='%h%(opt-color ? %Cred) foo'

uncommenting the //parts_debug(parsed, 0); line in pretty.c will show off the built format tree.

Show 18 quoted lines

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