Re: [PATCH v6 02/13] Support showing notes from more than one notes tree

[Johan Herland <johan@herland.net>]

On Wednesday 10 March 2010, Thomas Rast wrote:
> With this patch, you can set notes.displayRef to a glob that points at
> your favourite notes refs, e.g.,
> 
> [notes]
> 	displayRef = refs/notes/*
> 
> Then git-log and friends will show notes from all trees.
> 
> Thanks to Junio C Hamano for lots of feedback, which greatly
> influenced the design of the entire series and this commit in
> particular.
> 
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>

Acked-by: Johan Herland <johan@herland.net>

...except for the following small niggles:

> diff --git a/Documentation/pretty-options.txt
> b/Documentation/pretty-options.txt index aa96cae..1d56926 100644
> --- a/Documentation/pretty-options.txt
> +++ b/Documentation/pretty-options.txt
> @@ -30,9 +30,18 @@ people using 80-column terminals.
>  	defaults to UTF-8.
> 
>  --no-notes::
> ---show-notes::
> +--show-notes[=<ref>]::
>  	Show the notes (see linkgit:git-notes[1]) that annotate the
>  	commit, when showing the commit log message.  This is the default
>  	for `git log`, `git show` and `git whatchanged` commands when
>  	there is no `--pretty`, `--format` nor `--oneline` option is
>  	given on the command line.
> ++
> +With an optional argument, add this ref to the list of notes.  The ref
> +is taken to be in `refs/notes/` if it is not qualified.
> +
> +--[no-]standard-notes::
> +	Enable or disable populating the notes ref list from the
> +	'core.notesRef' and 'notes.displayRef' variables (or
> +	corresponding environment overrides).  See
> +	linkgit:git-config[1].

Should probably mention that --standard-notes describes the default 
behaviour.

> diff --git a/notes.c b/notes.c
> index a4f9926..0d4b892 100644
> --- a/notes.c
> +++ b/notes.c
> +static int notes_display_config(const char *k, const char *v, void *cb)
> +{
> +	int *load_refs = cb;
> +
> +	if (*load_refs && !strcmp(k, "notes.displayref")) {
> +		if (!v)
> +			config_error_nonbool(k);
> +		string_list_add_refs_by_glob(&display_notes_refs, v);
> +		return 0;

This "return 0" is unnecessary

> +static int string_list_add_refs_from_list(struct string_list_item *item,
> +					  void *cb)
> +{
> +	struct string_list *list = cb;
> +	string_list_add_refs_by_glob(list, item->string);
> +	return 0;
> +}

Shouldn't string_list_add_refs_from_list() be placed up with the other 
string_list_add_* functions? 

> +void init_display_notes(struct display_notes_opt *opt)
> +{
> +	char *display_ref_env;
> +	int load_config_refs = 0;
> +	display_notes_refs.strdup_strings = 1;
> +
> +	assert(!display_notes_trees);
> +
> +	if (!opt || !opt->suppress_default_notes) {
> +		string_list_append(default_notes_ref(), &display_notes_refs);
> +		display_ref_env = getenv(GIT_NOTES_DISPLAY_REF_ENVIRONMENT);
> +		if (display_ref_env) {
> +			string_list_add_refs_from_colon_sep(&display_notes_refs,
> +							    display_ref_env);
> +			load_config_refs = 0;
> +		} else {
> +			load_config_refs = 1;
> +		}

Drop unnecessary braces in the 'else'

> @@ -1030,3 +1178,14 @@ void format_note(struct notes_tree *t, const
> unsigned char *object_sha1,
> 
>  	free(msg);
>  }
> +
> +void format_display_notes(const unsigned char *object_sha1,
> +			  struct strbuf *sb, const char *output_encoding, int flags)
> +{
> +	int i;
> +	if (!display_notes_trees)
> +		init_display_notes(NULL);

Not sure I like this "fallback" call to init_display_notes(NULL). There's 
already a call from builtin/log.c which passes "&rev->notes_opt", and this 
fallback is at best useless (since init_display_notes() has already been 
called from builtin/log.c), and at worst confusing, since it would _mask_ a 
missing init_display_notes(options) call from elsewhere.

I'd rather suggest that format_display_notes() should fail because of a 
missing init_display_notes(), and not silently compensate for missing 
initialization by self-initializing with default options (thus disregarding 
any notes-related options the user might have passed on the command-line).

> diff --git a/notes.h b/notes.h
> index bad03cc..db47b67 100644
> --- a/notes.h
> +++ b/notes.h
> +/*
> + * Append notes for the given 'object_sha1' from all trees set up by
> + * init_display_notes() to 'sb'.  The 'flags' are a bitwise
> + * combination of
> + *
> + * - NOTES_SHOW_HEADER: add a 'Notes (refname):' header
> + *
> + * - NOTES_INDENT: indent the notes by 4 places
> + *
> + * init_display_notes() is called implicitly for you if you haven't
> + * already.

As I said above, the implicit initialization turns a missing 
init_display_notes(with_approriate_options) call into a 
init_display_notes(NULL) call. This makes it harder to pinpoint future bugs 
in this area (which are likely to crop up as we add notes functionality to 
more Git commands).

But I don't think this is important enough to hold up the patch series.

> + */
> +void format_display_notes(const unsigned char *object_sha1,
> +			  struct strbuf *sb, const char *output_encoding, int flags);
> +
> +/*
> + * Load the notes tree from each ref listed in 'refs'.  The output is
> + * an array of notes_tree*, terminated by a NULL.
> + */
> +struct notes_tree **load_notes_trees(struct string_list *refs);

Although this looks like a useful utility function, I'm not sure how useful 
it is in practice: What would you really _do_ with the returned struct 
notes_tree **? You can't pass it to format_display_notes() (which only uses 
the internal display_notes_trees)...

> +/*
> + * Add all refs that match 'glob' to the 'list'.
> + */
> +void string_list_add_refs_by_glob(struct string_list *list, const char
> *glob); +
> +/*
> + * Add all refs from a colon-separated glob list 'globs' to the end of
> + * 'list'.  Empty components are ignored.  This helper is used to
> + * parse GIT_NOTES_DISPLAY_REF style environment variables.
> + */
> +void string_list_add_refs_from_colon_sep(struct string_list *list,
> +					 const char *globs);

The same goes for these. Do they have actual outside callers? AFAICS we only 
ever use the internal display_notes_refs and display_notes_trees lists in 
notes.c, so there's limited potential for external use.


Otherwise, it all looks good to me. :)


...Johan

-- 
Johan Herland, <johan@herland.net>
www.herland.net