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

On Sun, Apr 25, 2010 at 8:48 PM, Jeff King [off-list ref] wrote:

On Sun, Apr 25, 2010 at 04:42:52PM +0100, Will Palmer wrote:
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.

Indeed, this is what I do now, and I really wasn't expecting anyone to
use it for a command other than log. There are three main
justifications:
 1) I hope to eventually have all builtin formats converted to use
this mechanism, and this is a good stepping-stone.
 2) defining a git alias to "log" when I really just want a shorter
way to type a format has always felt horribly clunky to me.
 3) it's so easy to add it's essentially free.

as you might guess, #2 is what really started the itch, and #3 is what
encouraged me to scratch it. #1 was more a side-effect when I realized
(when first trying to implement things) that it's not how things are
currently done.

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.

Thank you, any input is greatly appreciated.

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.

following the "diff" color seemed to be the right thing in most
places, but for some things it looked as if there may be more-specific
not-diff-related color options being passed int, which is why I
changed the show_log() calls. Looking back over the patch now, it
seems that the only place left which doesn't reference a rev_info
struct is show-branch.c, which doesn't even have a --format option. So
I suppose that can all get ripped out and replaced with a single check
of the diff options in  the next version of the patch.

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.

This one I was also a bit iffy on, but in my mind the fact that %H
currently completely ignores the --abbrev-commit option feels like a
bug to me. One of those "if it has no effect, there should be a
warning", things.
We can at least be assured that as --abbrev[-commit] previously had no
effect on --format options, that the only people who would have used
them together have probably been disappointed at the results, and did
not continue to do so.
There /are/ a couple of places in-code which use %H to fetch a
commit-hash-string, and I made sure to avoid modification of rev_info
when I spotted that, but really that feels like something that should
be using a wrapper get_commit_hash_string() function.

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.

I was writing them as pretty.<name>, until I saw that format.pretty
was already there. I'm not sure which is the weirder of the two.

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

Nice to be welcome. You may have seen a test-run I did to get my
confidence up by submitting a two-line documentation change a week or
so ago ;)
-- 
Will Palmer
wmpalmer@gmail.com