Re: [PATCH 0/3] pretty: format aliases

From: Jeff King <peff@peff.net>
Date: Apr 25, 2010, 19:48 UTC
Message-ID: <20100425194800.GB14736@coredump.intra.peff.net>
In-Reply-To: <1272210175-20484-1-git-send-email-wmpalmer@gmail.com>

On Sun, Apr 25, 2010 at 04:42:52PM +0100, Will Palmer wrote:

Show 5 quoted lines

> The following patch series adds the ability to configure aliases for
> user-defined formats. The first two patches define new placeholders and
> modify the output of existing placeholders to allow aliases to be more
> consistent with the way builtin formats are handled. The final patch
> adds support for the aliases themselves.

Interesting idea. I think right now most people just want to use their format with "log", so they would do something like:

  git config alias.mylog "log --format='...'"

Your method allows the same format to be used with multiple commands, which is more flexible. But I wonder how many people would find it useful in practice. I can think of only "log" and "show" that I would really want to share between.

I skimmed your patches and have included a few comments below, as I don't have time at the moment to do a thorough review.

Show 7 quoted lines

> There were a couple of places where I wasn't entirely sure about which
> color setting I should be following, but I've tried to be consistent
> throughout. It may be that I could have simply followed diffopt's color
> option in all cases, in which case various modifications to show_log()
> were entirely unnecessary. I'll await judgement at the hands of one who
> groks those sections more than I do, but I think what I've done feels
> correct.

I think that makes some sense. The "diff" color option was the first, and so the commit-coloring in "git log" follows that already. So short of making a new "color.log" variable, that makes the most sense.

> My original goal was to make it possible to define all of the builtin
> formats as builtin aliases to format strings, but complications

I like that goal. As you found out, it may be harder than you would like. But the pretty-print code is IMHO a bit messy with conditionals, and most of it can now be expressed as much simpler user-formats.

>   pretty: add conditional %C?colorname placeholders

This one seems reasonable overall.

>   pretty: make %H/%h dependent on --abbrev[-commit]

This is really a variation on the first one. And there are more variations, like %T/%t, %P/%p, etc. I'm a little hesitant to just change the meaning of "%H", which has always explicitly meant the full sha1. Should we perhaps introduce some universal syntax for "abbreviate if --abbrev was given, otherwise full sha1". Like "%?H" as you did above, except that "?" doesn't really make sense.

>   pretty: add aliases for pretty formats

The config variables format.* are traditionally about format-patch. I see we have format.pretty these days, too. I'm not sure if that is a deliberate attempt to make format.* more inclusive, or simply an error. If the latter, we should probably not make it worse with format.pretty.name.

-Peff

PS Welcome to the git list. It is nice to see a submission from a newcomer who has clearly read SubmittingPatches, and that even has appropriate documentation and test updates. :)