Re: [PATCH v4 02/11] Support showing notes from more than one notes tree

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 23, 2010, 00:21 UTC
Message-ID: <7vzl30u7hs.fsf@alter.siamese.dyndns.org>
In-Reply-To: <201002230025.57975.trast@student.ethz.ch>

Thomas Rast <trast@student.ethz.ch> writes:

Show 24 quoted lines

> Umm, since I'm currently working on another reroll that has pretty
> args to add notes refs for display...
>
> On Tuesday 23 February 2010 00:20:06 Junio C Hamano wrote:
>> Thomas Rast <trast@student.ethz.ch> writes:
>> > +	if (flags & NOTES_SHOW_HEADER_WITH_REF && t->ref) {
>> > +		const char *ref = t->ref;
>> > +		if (!strcmp(ref, GIT_NOTES_DEFAULT_REF)) {
>> > +			strbuf_addstr(sb, "\nNotes:\n");
>> > +		} else {
>> > +			if (!prefixcmp(ref, "refs/"))
>> > +				ref += 5;
>> > +			if (!prefixcmp(ref, "notes/"))
>> > +				ref += 6;
>> > +			strbuf_addf(sb, "\nNotes (%s):\n", ref);
>> > +		}
>> > +	} else if (flags & (NOTES_SHOW_HEADER|NOTES_SHOW_HEADER_WITH_REF))
>> >  		strbuf_addstr(sb, "\nNotes:\n");
>> 
>> It is not clear what the distinction between NOTES_SHOW_HEADER and
>> NOTES_SHOW_HEADER_WITH_REF.  Does anybody still call this function with
>> NOTES_SHOW_HEADER alone without NOTES_SHOW_HEADER_WITH_REF?
>
> No.

Meaning nobody will go through the latter "Notes:\n" codepath? Then what is that else clause for?

Perhaps I am not reading your code right in which case this part needs a bit more commenting?

Show 6 quoted lines

>> I expected to see "Notes:\n" regardless of the mode if the notes is coming
>> from the default refs/notes/commits tree, but it probably is better to say
>> "Notes (commits):\n" like your patch does.
>
> I special-cased GIT_NOTES_DEFAULT_REF (which is "refs/notes/commits")
> above *at your request* to not change the output in the default case.

> So which way do you want it?

I don't have strong preference anymore with the above code.

If some caller called the latter "always show Notes:" codepath, i.e. SHOW_HEADER is given but without HEADER_WITH_REF, then my preference would have been more clear. A caller that gives HEADER_WITH_REF clearly wants to show more than one notes (perhaps the user used displayref mechanism) and it would be better to always say which one in order to disambiguate, and the special case would not be a good idea. A caller would not give HEADER_WITH_REF when the user has refs/notes/commits hierarchy and nothing else, and the user didn't ask for multiple notes trees shown; in such a case we should show the good-old "Notes:".

But if everybody calls with HEADER_WITH_REF, no matter what the end user preference is, then it becomes unclear what the right answer would be.