On Sun, Apr 25, 2010 at 8:48 PM, Jeff King <peff@xxxxxxxx> 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@xxxxxxxxx -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html