Re: [RFC PATCH v3 01/12] Support showing notes from more than one notes tree

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 21, 2010, 03:06 UTC
Message-ID: <7viq9r478z.fsf@alter.siamese.dyndns.org>
In-Reply-To: <34f0e6d201f6bddfc8f4f374493b1b54d60b82cd.1266703765.git.trast@student.ethz.ch>

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

Show 10 quoted lines

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 4c36aa9..3c5cfec 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -517,7 +517,7 @@ core.notesRef::
>  	after the full SHA-1 of the commit they annotate.
>  +
>  If such a file exists in the given ref, the referenced blob is read, and
> -appended to the commit message, separated by a "Notes:" line.  If the
> +appended to the commit message, separated by a "Notes from <refname>:" line.  If the

I haven't looked at your code, but I think the output should be kept the same when <refname> is "refs/notes/commits", and it probably is a good idea to omit "refs/notes/" part. E.g. "Notes (amlog):" for notes stored in the refs/notes/amlog hierarchy.

Show 7 quoted lines

> @@ -1300,6 +1300,17 @@ mergetool.keepTemporaries::
>  mergetool.prompt::
>  	Prompt before each invocation of the merge resolution program.
>  
> +notes.displayRef::
> +	When showing commit messages, also show notes which are stored
> +	in the given ref.  The ref may also be a glob, in which case

If this were a description that follows an example

	[notes]
		displayRef = <ref>

then the above sentences make sense, but otherwise "in the given ref" does not make much sense. "The ref may also be a glob" is a lot worse. A "ref" is never be a glob. You can specify what refs to display by setting the _value_ of this variable to a glob.

    notes.displayRef::
        The refname in the notes namespace from which to show notes when
        showing commit messages.  The value of this variable can be set to
        a glob, in whch case ...

> +	...  If a ref
> +	does not exist, it will be skipped silently.

Hmph...  Even when it is not a globbing specification?  If I said

	[notes]
		displayRef = refs/notes/tr/*

and there is no notes in the tr/ hierarchy yet, I would appreciate if the code doesn't complain, but if I said a more concrete

	[notes]
		displayRef = refs/notes/anlog

and there is no "anlog", because I meant "amlog" and the above was a typo, I would appreciate if the code somehow lets me know about it.

> +static int notes_display_config(const char *k, const char *v, void *cb)
> +{
> +	if (!strcmp(k, "notes.displayref")) {
> +		string_list_add_refs_by_glob(&display_notes_refs, v);

Somebody in the callchain should say

	if (!v)
        	config_error_nonbool(k)

before you use v here.

I don't think string_list_insert() has a desirable property for this. If I wrote

	[notes]
            displayRef = refs/notes/commit
            displayRef = refs/notes/amlog

I would expect that the normal notes come first and then notes from the amlog tree, not in the alphabetical "amlog happens to sort before commit" order. If I used a glob, e.g. "refs/notes/tr/*", I am saying "all notes in the tr/ namespace, I do not care about the particular order among them. Just use something sensible", and it would make sense to use some stable ordering, e.g. the order for_each_glob_ref() calls you back, but receiving the data and calling string_list_insert() will destroy the order the user gave you the configuration, no?

Show 10 quoted lines

> diff --git a/t/t3301-notes.sh b/t/t3301-notes.sh
> index 90178f9..0dfff82 100755
> --- a/t/t3301-notes.sh
> +++ b/t/t3301-notes.sh
> @@ -98,7 +98,7 @@ Date:   Thu Apr 7 15:14:13 2005 -0700
>  
>      2nd
>  
> -Notes:
> +Notes from notes/commits:

Please don't.