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

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

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

> Changes since v4:
> * introduce --show-notes=<ref> and --[no-]standard-notes

Nicer, much nicer.

> * remove NOTES_SHOW_HEADER_WITH_REF distinction

Note that there is a leftover caller that uses the symbol without noticing
that it has been retired.  I'll fix it up locally when I queue the series
to 'pu'.

> * parse config even if we don't care about it

Parsing is good, and not reading trees is very good.

It is silly to put yourself down after doing both of these very well by
saying "even if we don't care about it"---you obviously do care about
making it earier for other people to build on top of your code.  Otherwise
you would have left the code unchanged from the previous round ;-).

> diff --git a/notes.c b/notes.c
> index 3ba3e6d..ee54a42 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -5,6 +5,8 @@
>  #include "utf8.h"
>  #include "strbuf.h"
>  #include "tree-walk.h"
> +#include "string-list.h"
> +#include "refs.h"
>  
>  /*
>   * Use a non-balancing simple 16-tree structure with struct int_node as
> @@ -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;

Unlike default_notes_tree, the above two can become static, as you made
the new logic better contained to this file and accessible only via
accessor functions.

> @@ -828,6 +833,76 @@ int combine_notes_ignore(unsigned char *cur_sha1,
> ...
> +static const char *default_notes_ref()

I'll s/_ref()/_ref(void)/ here.

> diff --git a/notes.h b/notes.h
> index bad03cc..7650254 100644
> --- a/notes.h
> +++ b/notes.h
> @@ -198,4 +198,22 @@ int for_each_note(struct notes_tree *t, int flags, each_note_fn fn,
>  void format_note(struct notes_tree *t, const unsigned char *object_sha1,
>  		struct strbuf *sb, const char *output_encoding, int flags);
>  
> +
> +struct string_list;
> +
> +struct display_notes_opt
> +{
> +	int suppress_default_notes : 1;
> +	struct string_list *extra_notes_refs;
> +};
> +
> +void init_display_notes(struct display_notes_opt *opt);
> +void format_display_notes(const unsigned char *object_sha1,
> +			  struct strbuf *sb, const char *output_encoding, int flags);
> +

As you are retiring format_note() as a public interface, and instead
making format_display_notes() as the primary API for the callers, the
former should be made static to notes.c along with the large comment
describing how to call it.  It may also be worth telling people how to
call this new public interface with similar comment here.

> @@ -1096,8 +1096,8 @@ void pretty_print_commit(enum cmit_fmt fmt, const struct commit *commit,
>  		strbuf_addch(sb, '\n');
>  
>  	if (context->show_notes)
> -		format_note(NULL, commit->object.sha1, sb, encoding,
> -			    NOTES_SHOW_HEADER | NOTES_INDENT);
> +		format_display_notes(commit->object.sha1, sb, encoding,
> +				     NOTES_SHOW_HEADER_WITH_REF | NOTES_INDENT);

I'll s/_WITH_REF// here.

> diff --git a/revision.c b/revision.c
> index 29721ec..d6e842e 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -1191,9 +1192,29 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  	} else if (!strcmp(arg, "--show-notes")) {
>  		revs->show_notes = 1;
>  		revs->show_notes_given = 1;
> +	} else if (!prefixcmp(arg, "--show-notes=")) {
> +		struct strbuf buf = STRBUF_INIT;
> +		revs->show_notes = 1;
> +		revs->show_notes_given = 1;
> +		if (!revs->notes_opt.extra_notes_refs)
> +			revs->notes_opt.extra_notes_refs = xcalloc(1, sizeof(struct string_list));
> +		if (!prefixcmp(arg+13, "refs/"))
> +			/* happy */;
> +		else if (!prefixcmp(arg+13, "notes/"))
> +			strbuf_addstr(&buf, "refs/");
> +		else
> +			strbuf_addstr(&buf, "refs/notes/");
> +		strbuf_addstr(&buf, arg+13);
> +		string_list_append(strbuf_detach(&buf, NULL),
> +				   revs->notes_opt.extra_notes_refs);

Nice; multiple --show-notes=... will accumulate in the order given.  I
knew you won't be stupid to make this a colon separated string, but I had
to double check ;-).