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

From: Jeff King <peff@peff.net>
Date: Mar 7, 2011, 19:26 UTC
Message-ID: <20110307192640.GB20930@sigill.intra.peff.net>
In-Reply-To: <1299523834.1835.17.camel@walleee>

On Mon, Mar 07, 2011 at 06:50:34PM +0000, Will Palmer wrote:

Show 8 quoted lines

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

I haven't looked at your code yet, but the breakdown of patches I would expect / hope for is something like:

  1. introduce infrastructure for creating parse-tree from strbuf_expand
     format, with some tests

  2. port format_commit_* over to new system; I would expect that the
     caller code will have to be part of both the parsing and the
     expansion, since the generic code can't know that "%ad" is
     meaningful (and we want to keep it for backwards compatibility).
     Leave format_commit_message as a parse + expand wrapper for simple
     callers who don't care about speed.

  3. Add generic "%(key:option)" support to the new infrastructure,
     forward-porting format_commit_* as necessary (and hopefully the
     change are minimal...).

So those are all big commits, obviously, but hopefully it lets us review in three stages: does the new infrastructure look good, does porting an existing caller (and probably the most complex caller) clean up the caller code, and then finally, does the new syntax look good?

But of course the devil is in the details, so probably that breakdown has some flaw in it. :) I'll see when I look at your code how close to reality I came.

-Peff