Re: [PATCH] pretty: Initialize notes if %N is used
On Sun, Apr 11, 2010 at 04:54:22PM +0200, Johannes Gilger wrote:
> something like this? I didn't see why userformat_fill_want had to have an extra
> argument for the format, since the user_format variable is static in pretty.c.
Yes, this is getting closer.
The reason to take the format argument is that there are places which call format_commit_message() with an arbitrary string. We would want them to be able to call userformat_fill_want(), too (right now, I don't think any of them should need it, though).
Maybe it should take a format string, and use the user_format string if you pass NULL?
> Sorry for the many very different patches to the bug, as you can see I'm not
> really familiar with best-practices in git.git.
Not at all. Sometimes seemingly simple bugs end up raising a whole host of other issues. I am glad you are sticking around to help come up with a good solution.
Show 10 quoted lines
> diff --git a/builtin/log.c b/builtin/log.c
> index b706a5f..f8f5d22 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -58,7 +58,11 @@ static void cmd_log_init(int argc, const char **argv, const char *prefix,
> usage(builtin_log_usage);
> argc = setup_revisions(argc, argv, rev, opt);
>
> - if (!rev->show_notes_given && !rev->pretty_given)
> + struct userformat_want w;
Don't declare variables in the middle of a function. It's a C99-ism that we avoid for older compilers.
Show 5 quoted lines
> + if (rev->commit_format == CMIT_FMT_USERFORMAT)
> + userformat_fill_want(&w);
> +
> + if (!rev->show_notes_given && (!rev->pretty_given || w.notes))
> rev->show_notes = 1;
Hmm. If we didn't get a userformat, what will be in w? It will be random cruft from the stack, because we didn't call userformat_fill_want.
You can just call it unconditionally, and it should do the right thing with a NULL user_format.
Show 9 quoted lines
> +void userformat_fill_want(struct userformat_want *w)
> +{
> + if (!user_format)
> + return;
> + struct strbuf dummy = STRBUF_INIT;
> + memset(w, 0, sizeof(*w));
> + strbuf_expand(&dummy, user_format, userformat_want_item, w);
> + strbuf_release(&dummy);
> +}This does nothing with a NULL user_format. It should probably still do the memset() to indicate that nothing is wanted (otherwise the caller has to be sure to initialize w themselves if they think user_format might be NULL).
So combined with my above suggestion:
void userformat_fill_want(const char *s, struct userformat_want *w)
{
struct strbuf dummy = STRBUF_INIT;
memset(w, 0, sizeof(*w));
if (!s) {
if (!user_format)
return;
s = user_format;
}
strbuf_expand(&dummy, user_format, userformat_want_item, w);
strbuf_release(&dummy);
}and then you can just call
userformat_fill_want(NULL, w);
safely from log.c (you don't even need to check rev->commit_format).
Also, even though I picked the name, userformat_fill_want is kind of a lousy name. It was the best I could come up with, but maybe somebody has a better suggestion.
-Peff
Johannes Gilger
