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

From: Will Palmer <wmpalmer@gmail.com>
Date: Mar 8, 2011, 08:29 UTC
Message-ID: <1299572985.4071.30.camel@walleee>
In-Reply-To: <20110307192640.GB20930@sigill.intra.peff.net>

On Mon, 2011-03-07 at 14:26 -0500, Jeff King wrote:

Show 29 quoted lines

> On Mon, Mar 07, 2011 at 06:50:34PM +0000, Will Palmer wrote:
> 
> > 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,

Yeah, looks about right. It's mostly the "those commits will still be pretty big" that I was concerned with. There's also the question of: as my end-goal is conditional formatting, should these "smaller, but still big" commits try to make sense independently, or, for example, should I lay out a basic structure in the earlier commits, filled-out with a relatively simple loop, and only later expand that into the recursive function / parse-tree structure; or, should I start with the "fancy" structures, even before they have a justification?

 - the "simple first" way sounds tempting, but it has the result of
   pretty much "inventing" in-between code which is never intended to
   actually be used. (even if it is intended to compile and work just 
   fine)
 - the "write it as it will be", however, is going to result in commits
   which may not make any sense one after another, and really only make
   sense in the end. I don't know if that's okay.

Neither of these options sound fun for bisecting, and yet it's such a big change (in terms of "everyone uses log, so every user is effected") that ease of bisectability seems like a very important consideration.

What I don't want to do is start the patch over from scratch, with only the "long formats"/"unification with for-each-ref" in mind, only to submit another patch following up later on that needs to completely change the structures again to fit with the "parse tree" idea. Given that the basic %(opt-color?...) test works, I expect that the current state of the tree-structure is at least fairly close to what it should be, though I also expect that someone with more experience writing parsers may want to slap me for the way that structure is built. Criticism is anticipated and appreciated.

Show 11 quoted lines

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

Er, good luck :) As a side-note: It turns out that rebasing to the current "next" was not too difficult. The result hasn't been pushed yet (I need to do a little be of forensic work to make sure a behaviour I'm seeing isn't a rebase-induced regression), but it does imply that at least pretty.c should still make a fair amount of sense, even though it's about a year old. Most of the problems I expected of the rebase, it turns out would have been in sections which I hadn't actually done yet.