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

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

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.

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