Re: [PATCH/RFC 0/9] add long forms for format placeholders

From: Will Palmer <wmpalmer@gmail.com>
Date: Mar 29, 2011, 07:27 UTC
Message-ID: <1301383620.2335.50.camel@dreddbeard>
In-Reply-To: <4D918032.3010608@drmicha.warpmail.net>

On Tue, 2011-03-29 at 08:46 +0200, Michael J Gruber wrote:

Show 28 quoted lines

> Junio C Hamano venit, vidit, dixit 29.03.2011 02:28:
> > Will Palmer <wmpalmer@gmail.com> writes:
> > 
> >> I've been kicking around this series for a while now as part of a larger
> >> effort of refactoring the pretty formats. A recent discussion on the
> >> list has lead me to believe that this smaller subset may be of use
> >> sooner, rather than later.
> >>
> >> This series attempts to add "long forms" for common format placeholders
> >> in the "git log" family of commands, making the way for yet more
> >> placeholders to be added without needing to worry too much about the
> >> increasingly limited set of available one-letter mnemonics. It also
> >> moves towards the possibility of eventual unification with the format
> >> options in for-each-ref.
> > 
> > I don't claim that I read 1300+ long [PATCH 5/9] carefully, but I like the
> > direction in which this topic is going very much.
> > 
> > Except that [PATCH 2/9] looked quite out of place---more like "I wanted to
> > sneak this feature in" than "this was needed to keep the resulting code
> > backward compatible" or anything like that.
> > 
> > Off the top of my head, I don't think of a reason to say that [PATCH 3/9]
> > is going in a wrong direction---is there a reason to make you worried in
> > the particular change?
> 
> I'm wondering how much of this could and should be shared with
> for-each-ref. ......................................

I agree with this. Not only that, but I think the "list" modes of branch and tag should also call for-each-ref internally, and I hope that some of the conditional formats that this series is moving slowly towards will help with that.

Show 9 quoted lines

> ............. Notable differences that I'm aware of:
> 
> - for-each-ref is about (named) refs which can point to any type of
> object; rev-list/log is about commit objects
> 
> - for-each-ref deals with "few" objects typically, rev-list/log with many
> 
> So, other than %(refname), %(upstream) and %(tagger...), all
> for-each-ref placeholders make sense for rev-list/log.

I think the "right thing to do" here is to allow the parser to accept any of the for-each-ref specifications, but for the formatter to return an empty string for anything that doesn't make sense in context. This is what for-each-ref currently does. for-each-ref also gives an empty string for some invalid specifications, such as %(tree:short), but I assume this is a bug.

I'm not sure what the implications are in terms of what additional structures we'll need to pass in to the formatter, as I haven't looked much at the for-each-ref code. It may also be that there are some commit-related things which for-each-ref doesn't currently bother to grab, since its placeholder list is comparatively smaller than the rev-list one.

Show 9 quoted lines

> 
> Sharing the parser would serve several purposes:
> 
> - reduced code
> - increased test coverage (for-each-ref tests would test the parser)
> - speed up for for-each-ref (due to your nice separation)
> - short placeholders for for-each-ref
> - automatic consistency between the two
>

This is already a part of my longer-term plans, though those were mostly as a "I bet it would be fairly simple to do this once the rest is done". What I'm actually working towards is strictly related to the --pretty= formats, so I expect it will be a while before I get to anything like for-each-ref unification. It may also be worth noting that the last part of this work I submitted, "pretty aliases", was sent nearly a year ago. I am not going to be working on any of this full-time. The point here is: I would not be offended if someone were to snatch for-each-ref unification up from me, since I really don't know when I would get to it myself.

> Michael

-- 
-- Will