git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Johan Herland <johan@herland.net>
Date
Mar 11, 2010, 10:03 UTC
Message-ID
<201003111103.22671.johan@herland.net>
In-Reply-To
<e69a916ca6afb53fb665951d499d7e6543007008.1268229087.git.trast@student.ethz.ch>
On Wednesday 10 March 2010, Thomas Rast wrote:
Show 13 quoted lines
> 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:
Show 24 quoted lines
> 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.

Show 13 quoted lines
> 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
Show 7 quoted lines
> +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?

Show 18 quoted lines
> +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'
Show 12 quoted lines
> @@ -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).

Show 15 quoted lines
> 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.
Show 9 quoted lines
> + */
> +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)...

Show 12 quoted lines
> +/*
> + * 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
Previous: Thomas RastNext: Thomas Rast
Message 45 of 66 in “several notes refs, post-rewrite, notes rewriting”
  1. 00/12 several notes refs, post-rewrite, notes rewritingThomas Rast, Feb 20, 2010
  2. 01/12 Support showing notes from more than one notes treeThomas Rast, Feb 20, 2010
  3. Junio C HamanoFeb 21, 2010
  4. 02/12 Documentation: document post-rewrite hookThomas Rast, Feb 20, 2010
  5. 03/12 commit --amend: invoke post-rewrite hookThomas Rast, Feb 20, 2010
  6. Junio C HamanoFeb 21, 2010
  7. 04/12 rebase: invoke post-rewrite hookThomas Rast, Feb 20, 2010
  8. 05/12 rebase -i: invoke post-rewrite hookThomas Rast, Feb 20, 2010
  9. 06/12 notes: implement 'git notes copy --stdin'Thomas Rast, Feb 20, 2010
  10. Junio C HamanoFeb 21, 2010
  11. 07/12 notes: implement helpers needed for note copying during rewriteThomas Rast, Feb 20, 2010
  12. Junio C HamanoFeb 21, 2010
  13. 08/12 rebase: support automatic notes copyingThomas Rast, Feb 20, 2010
  14. 09/12 commit --amend: copy notes to the new commitThomas Rast, Feb 20, 2010
  15. 10/12 filter-branch: invoke post-rewrite hookThomas Rast, Feb 20, 2010
  16. 11/12 filter-branch: learn how to filter notesThomas Rast, Feb 20, 2010
  17. 12/12 notes: add shorthand --ref to override GIT_NOTES_REFThomas Rast, Feb 20, 2010
  18. Junio C HamanoFeb 21, 2010
  19. Thomas RastFeb 21, 2010
  20. Junio C HamanoFeb 22, 2010
  21. 00/11 several notes refs, post-rewrite, notes rewritingThomas Rast, Feb 23, 2010
  22. 01/11 test-lib: unset GIT_NOTES_REF to stop it from influencing testsThomas Rast, Feb 23, 2010
  23. 02/11 Support showing notes from more than one notes treeThomas Rast, Feb 23, 2010
  24. Junio C HamanoFeb 23, 2010
  25. Thomas RastFeb 23, 2010
  26. format-patch: learn to fill comment section of email from notesThomas Rast, Feb 23, 2010
  27. BROKEN -- format-patch: learn to fill comment section of email from notesThomas Rast, Feb 23, 2010
  28. Thomas RastFeb 23, 2010
  29. Stephen BoydFeb 24, 2010
  30. Junio C HamanoFeb 23, 2010
  31. Thomas RastMar 10, 2010
  32. 03/11 Documentation: document post-rewrite hookThomas Rast, Feb 23, 2010
  33. 04/11 commit --amend: invoke post-rewrite hookThomas Rast, Feb 23, 2010
  34. 05/11 rebase: invoke post-rewrite hookThomas Rast, Feb 23, 2010
  35. 06/11 rebase -i: invoke post-rewrite hookThomas Rast, Feb 23, 2010
  36. Junio C HamanoFeb 24, 2010
  37. 07/11 notes: implement 'git notes copy --stdin'Thomas Rast, Feb 23, 2010
  38. 08/11 notes: implement helpers needed for note copying during rewriteThomas Rast, Feb 23, 2010
  39. 09/11 rebase: support automatic notes copyingThomas Rast, Feb 23, 2010
  40. Junio C HamanoFeb 25, 2010
  41. 00/13 several notes refs, post-rewrite, notes rewritingThomas Rast, Mar 10, 2010
  42. 01/13 test-lib: unset GIT_NOTES_REF to stop it from influencing testsThomas Rast, Mar 10, 2010
  43. Johan HerlandMar 11, 2010
  44. 02/13 Support showing notes from more than one notes treeThomas Rast, Mar 10, 2010
  45. Johan HerlandMar 11, 2010
  46. 03/13 Documentation: document post-rewrite hookThomas Rast, Mar 10, 2010
  47. 04/13 commit --amend: invoke post-rewrite hookThomas Rast, Mar 10, 2010
  48. 05/13 rebase: invoke post-rewrite hookThomas Rast, Mar 10, 2010
  49. 06/13 rebase -i: invoke post-rewrite hookThomas Rast, Mar 10, 2010
  50. 07/13 notes: implement 'git notes copy --stdin'Thomas Rast, Mar 10, 2010
  51. Johan HerlandMar 11, 2010
  52. 08/13 notes: implement helpers needed for note copying during rewriteThomas Rast, Mar 10, 2010
  53. Johan HerlandMar 11, 2010
  54. 09/13 rebase: support automatic notes copyingThomas Rast, Mar 10, 2010
  55. 10/13 commit --amend: copy notes to the new commitThomas Rast, Mar 10, 2010
  56. 11/13 notes: add shorthand --ref to override GIT_NOTES_REFThomas Rast, Mar 10, 2010
  57. Johan HerlandMar 11, 2010
  58. 12/13 notes: track whether notes_trees were changed at allThomas Rast, Mar 10, 2010
  59. Johan HerlandMar 11, 2010
  60. 13/13 git-notes(1): add a section about the meaning of historyThomas Rast, Mar 10, 2010
  61. Johan HerlandMar 11, 2010
  62. Junio C HamanoMar 10, 2010
  63. 10/11 commit --amend: copy notes to the new commitThomas Rast, Feb 23, 2010
  64. 11/11 notes: add shorthand --ref to override GIT_NOTES_REFThomas Rast, Feb 23, 2010
  65. Junio C HamanoFeb 23, 2010
  66. Thomas RastFeb 23, 2010

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.