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

[Johannes Gilger <heipei@hackvalue.de>]

On 10/04/10 14:51, Junio C Hamano wrote:
> > -	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.

> > -	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