Re: [PATCH v5 02/11] Support showing notes from more than one notes tree
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 23, 2010, 01:47 UTC
- Message-ID
- <7vsk8sn2o3.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <3dbcdcf1a364d14968c07e99564acb232c6a5c43.1266885599.git.trast@student.ethz.ch>
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 ;-).
Show 19 quoted lines
> 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.
Show 21 quoted lines
> 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.
Show 8 quoted lines
> @@ -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.
Show 23 quoted lines
> 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 ;-).