On Sat, Apr 10, 2010 at 02:51:55PM -0700, Junio C Hamano wrote:
> > +++ b/builtin/log.c
> > @@ -58,9 +58,9 @@ 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)
> > + if (!rev->show_notes_given)
> > rev->show_notes = 1;
>
> I am puzzled by this change and its possible interaction with codepaths
> that do not have anything to do with %N. When there is no show-notes and
> an explicit --pretty, we do not want to have rev->show_notes set.
Could we perhaps just do:
if (!rev->show_notes_given &&
(!rev->pretty_given ||
(rev->commit_format == CMIT_FMT_USERFORMAT && fmt_wants_notes(...))
where fmt_wants_notes is similar to what I posted earlier, or even just
strstr(fmt, "%N")? As I discussed earlier, it is not 100% accurate, but
it is extremely unlikely for it to be wrong, and when it is, we will
load notes when we don't need to. Which is an optimization failure, but
not a correctness failure.
And then just guard the '%N' placeholder by checking show_notes. That
will protect random codepaths that call format_commit_message() but
aren't log (they can't trigger an assert, but will just get '%N'
unexpanded or whatever). And doing:
git log --no-notes --format='%N'
should also just fail to expand %N. Which is maybe a little crazy, but
what the user is asking for is crazy, and it makes the most sense to me.
-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