Re: [PATCH] Notes: Connect the %N flag to --{show,no}-notes

From: Johannes Gilger <heipei@hackvalue.de>
Date: Apr 10, 2010, 22:20 UTC
Message-ID: <20100410222031.GA12507@dualtron.lan>
In-Reply-To: <7v1venvuv8.fsf@alter.siamese.dyndns.org>

On 10/04/10 14:51, Junio C Hamano wrote:

Show 11 quoted lines

> > -	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.
> 
> Admittedly, the real end result we want to see in such a case is just that
> notes are not shown (and rev->show_notes being false is one natural way to
> achieve that), and if ...

Yes, it might seem a little dirty looking at the name of the flags. If no --show-notes was given and --pretty was supplied, rev->show_notes should have a value of 'maybe' ;)

I was aiming for minimally invasive changes while keeping the former behaviour and only dealing with the "only %N" case, which is what this patch does.

Show 11 quoted lines

> > -	if (rev->show_notes)
> > +	if (rev->show_notes && (!rev->pretty_given || rev->show_notes_given))
> >  		init_display_notes(&rev->notes_opt);
> 
> ... this change is about ensuring the same outcome by not initializing the
> notes tree, that may work, but it somehow feels iffy.  It would leave some
> codepaths (and another one you just added, I think, with the other hunk in
> this patch) that say "do this only when rev->show_notes is set" and some
> other codepaths that say "unconditionally try to show notes and rely on
> the caller not have initialized the notes tree when it is not wanted."  Is
> that what is going on?

The implicit initialization of the notes_trees only happens if --pretty is used alone, and in no other case. I was under the impression that not initializing the notes_trees if one isn't sure of it's use was meant to be a performance criterion. While --show-notes will always use the notes when using plain log/show, it won't necessarily use the notes for certain --pretty/--format formats. Granted, right now I can use --pretty and --show-notes although I don't use %N and intentionally waste memory by initializing the trees.

> Unfortunately I don't think of a better and cleaner solution offhand
> (perhaps such a cleaner solution would involve adding a bit more state in
> the rev structure, but I haven't thought things through).

Yes, I came across that structure too but was happy enough my patch works as it is. I'll leave design decisions up to more involved contributors, my main agenda is simply to not have git segfault with something as harmless as "git log '%N'" ;)

Greetings, Jojo

-- 
Johannes Gilger <heipei@hackvalue.de>
http://heipei.net
GPG-Key: 0xD47A7FFC
GPG-Fingerprint: 5441 D425 6D4A BD33 B580  618C 3CDC C4D0 D47A 7FFC