On Wed, Sep 03, 2008 at 08:40:08PM +0200, Michael Dressel wrote:
> This is a variation of the patch provided by Jeff King <peff@xxxxxxxx>:
> allow '%d' pretty format specifier to show decoration
Thank you for picking this up. I had been meaning to look at it further
but hadn't gotten around to it yet.
> would calculate decorations, but not show them. You can now
> do:
>
> git log --pretty=format:'%H (%d) %s'
I think Junio requested the enclosing parentheses be included. I am not
sure I agree, but we are already mandating the comma separator.
Personally, I think the right solution is more like
%(decorate:delim=, )
but that is a much bigger change (and part of what was holding me up on
just improving this patch). I think adding %d in the meantime (with or
without the surrounding parentheses) is reasonable.
> The difference is that you don't need --decorate when %d has been given
> because this would be "doppeltgemoppelt" (redundant).
This is good, but I don't like the implementation:
> +static int deco_in_format(int argc, const char **argv)
> +{
> + int i;
> + int deco=0;
> + for (i=0;i<argc;i++)
> + {
> + if ((strstr(argv[i], "pretty=format:") ||
> + strstr(argv[i], "pretty=tformat:")) &&
> + strstr(argv[i], "%d"))
> + {
> + deco=1;
> + break;
> + }
> +
> + }
> + return deco;
> +}
There are two bad things about this:
1. It looks through argv. Partly this is a bit ugly, and we should
just hook into the part where we have already parsed --pretty=.
Which in this case really means looking at user_format after
calling setup_revisions (which is sadly a static global; it really
should be cleaned up as a member of struct rev_list). But worse
here is that you don't even look through argv correctly. You really
want prefixcmp(argv[i], "--pretty=format:"), since otherwise this
would trigger on something like:
git log ':/implement %d in pretty:format'
which is unlikely, perhaps, but I don't think it's that hard to do
it right in this case.
2. It just looks for '%d' rather than using the same parser as the
rest of the code. I _think_ this is actually correct now, because
we don't even allow simple quoting like '%%d'. But it would be much
cleaner, I think, to have a library call next to
format_commit_message() that fills in a struct of "used" items.
So something like:
static void cmd_log_init(...)
{
struct user_format_need ufneed;
...
for (i = 1; i < argc; i++) {
...
if (!stcmp(arg, "--decorate"))
decorate = 1;
...
}
user_format_needs(&ufneed);
if (decorate || ufneed.decorate)
for_each_ref(add_ref_decoration, NULL);
}
Make sense?
-Peff
--
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
