From: Jeff King <peff@peff.net>
Date: Sat, 10 Apr 2010 22:08:43 GMT
Subject: Re: [PATCH] Notes: Connect the %N flag to --{show,no}-notes
Message-ID: <20100410220843.GA29987@coredump.intra.peff.net>
In-Reply-To: <7v1venvuv8.fsf@alter.siamese.dyndns.org>

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