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

[Junio C Hamano <gitster@pobox.com>]

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

> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 4c36aa9..a0f2b0f 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

Hmm...

> diff --git a/notes.c b/notes.c
> index 3ba3e6d..c480370 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -68,6 +70,9 @@ struct non_note {
>  
>  struct notes_tree default_notes_tree;
>  
> +struct string_list display_notes_refs;
> +struct notes_tree **display_notes_trees;

Do these need to be extern?

> +void string_list_add_refs_from_glob_list(struct string_list *list,
> +					 const char *globs)
> +{
> +	struct strbuf globbuf = STRBUF_INIT;
> +	struct strbuf **split;
> +	int i;
> +
> +	strbuf_addstr(&globbuf, globs);
> +	split = strbuf_split(&globbuf, ':');
> +
> +	for (i = 0; split[i]; i++) {
> +		if (!split[i]->len)
> +			continue;
> +		if (split[i]->buf[split[i]->len-1] == ':')
> +			strbuf_setlen(split[i], split[i]->len-1);
> +		string_list_add_refs_by_glob(list, split[i]->buf);
> +	}

Nice use of strbuf_split().  I wonder if we should automatically add
"refs/" and/or "refs/notes/" when the input is missing the prefix.  I
don't have strong preference myself but the users might make noises.

Either way it needs to be documented in the final version before the
series goes to 'master'.

> +static int notes_display_config(const char *k, const char *v, void *cb)
> +{
> +	/* Warning!  This is currently not executed if
> +	 * GIT_NOTES_DISPLAY_REF is set.  Move the git_config() call
> +	 * outside the test if you add more options. */

Yuck.  If you know what needs to be done, do that before other poeple add
more options, please.

> @@ -1016,7 +1135,18 @@ void format_note(struct notes_tree *t, const unsigned char *object_sha1,
>  	if (msglen && msg[msglen - 1] == '\n')
>  		msglen--;
>  
> -	if (flags & NOTES_SHOW_HEADER)
> +	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?

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.