From: Johan Herland <johan@herland.net>
To: Thomas Rast <trast@student.ethz.ch>
Cc: git@vger.kernel.org, Johannes Sixt <j6t@kdbg.org>
Subject: Re: [PATCH v6 02/13] Support showing notes from more than one notes tree
Date: Thu, 11 Mar 2010 11:03:22 +0100 [thread overview]
Message-ID: <201003111103.22671.johan@herland.net> (raw)
In-Reply-To: <e69a916ca6afb53fb665951d499d7e6543007008.1268229087.git.trast@student.ethz.ch>
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
next prev parent reply other threads:[~2010-03-11 10:03 UTC|newest]
Thread overview: 135+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-02-14 16:17 [RFC PATCH 0/6] post-rewrite hook and copying notes Thomas Rast
2010-02-14 16:17 ` [RFC PATCH 1/6] Documentation: document post-rewrite hook Thomas Rast
2010-02-14 16:17 ` [RFC PATCH 2/6] commit --amend: invoke " Thomas Rast
2010-02-14 16:17 ` [RFC PATCH 3/6] filter-branch: " Thomas Rast
2010-02-15 20:36 ` Johannes Sixt
2010-02-14 16:17 ` [RFC PATCH 4/6] rebase: " Thomas Rast
2010-02-14 16:17 ` [RFC PATCH 5/6] rebase -i: " Thomas Rast
2010-02-14 16:17 ` [RFC PATCH 6/6] contrib: add a hook that copies notes over rewrites Thomas Rast
2010-02-14 16:21 ` Thomas Rast
2010-02-14 21:46 ` [PATCH] WIP: git notes copy --stdin Thomas Rast
2010-02-15 1:25 ` Johan Herland
2010-02-16 23:25 ` [RFC PATCH v2 00/11] post-rewrite / automatic notes copying Thomas Rast
2010-02-16 23:25 ` [RFC PATCH v2 01/11] Documentation: document post-rewrite hook Thomas Rast
2010-02-16 23:59 ` Junio C Hamano
2010-02-17 0:18 ` Thomas Rast
2010-02-17 0:29 ` Junio C Hamano
2010-02-16 23:25 ` [RFC PATCH v2 02/11] commit --amend: invoke " Thomas Rast
2010-02-16 23:25 ` [RFC PATCH v2 03/11] rebase: " Thomas Rast
2010-02-16 23:26 ` [RFC PATCH v2 04/11] rebase -i: " Thomas Rast
2010-02-16 23:26 ` [RFC PATCH v2 05/11] notes: clean up t3301 Thomas Rast
2010-02-16 23:52 ` Junio C Hamano
2010-02-16 23:26 ` [RFC PATCH v2 06/11] notes: implement 'git notes copy --stdin' Thomas Rast
2010-02-16 23:26 ` [RFC PATCH v2 07/11] notes: implement helpers needed for note copying during rewrite Thomas Rast
2010-02-16 23:58 ` Junio C Hamano
2010-02-17 0:09 ` Thomas Rast
2010-02-17 0:18 ` Junio C Hamano
2010-02-20 14:58 ` [WIP/RFC PATCH] Support showing notes from more than one notes tree Thomas Rast
2010-02-20 15:23 ` Thomas Rast
2010-02-16 23:26 ` [RFC PATCH v2 08/11] rebase: support automatic notes copying Thomas Rast
2010-02-16 23:26 ` [RFC PATCH v2 09/11] commit --amend: copy notes to the new commit Thomas Rast
2010-02-16 23:26 ` [RFC PATCH v2 10/11] filter-branch: invoke post-rewrite hook Thomas Rast
2010-02-16 23:26 ` [RFC PATCH v2 11/11] filter-branch: learn how to filter notes Thomas Rast
2010-02-17 19:59 ` Johannes Sixt
2010-02-17 23:06 ` Thomas Rast
2010-02-20 22:16 ` [RFC PATCH v3 00/12] several notes refs, post-rewrite, notes rewriting Thomas Rast
2010-02-20 22:16 ` [RFC PATCH v3 01/12] Support showing notes from more than one notes tree Thomas Rast
2010-02-21 3:06 ` Junio C Hamano
2010-02-20 22:16 ` [RFC PATCH v3 02/12] Documentation: document post-rewrite hook Thomas Rast
2010-02-20 22:16 ` [RFC PATCH v3 03/12] commit --amend: invoke " Thomas Rast
2010-02-21 3:12 ` Junio C Hamano
2010-02-20 22:16 ` [RFC PATCH v3 04/12] rebase: " Thomas Rast
2010-02-20 22:16 ` [RFC PATCH v3 05/12] rebase -i: " Thomas Rast
2010-02-20 22:16 ` [RFC PATCH v3 06/12] notes: implement 'git notes copy --stdin' Thomas Rast
2010-02-21 3:31 ` Junio C Hamano
2010-02-20 22:16 ` [RFC PATCH v3 07/12] notes: implement helpers needed for note copying during rewrite Thomas Rast
2010-02-21 3:34 ` Junio C Hamano
2010-02-20 22:16 ` [RFC PATCH v3 08/12] rebase: support automatic notes copying Thomas Rast
2010-02-20 22:16 ` [RFC PATCH v3 09/12] commit --amend: copy notes to the new commit Thomas Rast
2010-02-20 22:16 ` [RFC PATCH v3 10/12] filter-branch: invoke post-rewrite hook Thomas Rast
2010-02-20 22:16 ` [RFC PATCH v3 11/12] filter-branch: learn how to filter notes Thomas Rast
2010-02-20 22:16 ` [RFC PATCH v3 12/12] notes: add shorthand --ref to override GIT_NOTES_REF Thomas Rast
2010-02-21 3:47 ` [RFC PATCH v3 00/12] several notes refs, post-rewrite, notes rewriting Junio C Hamano
2010-02-21 6:14 ` Thomas Rast
2010-02-22 0:18 ` Junio C Hamano
2010-02-22 0:10 ` [PATCH v4 00/11] " Thomas Rast
2010-02-22 0:10 ` [PATCH v4 01/11] test-lib: unset GIT_NOTES_REF to stop it from influencing tests Thomas Rast
2010-02-22 0:10 ` [PATCH v4 02/11] Support showing notes from more than one notes tree Thomas Rast
2010-02-22 23:20 ` Junio C Hamano
2010-02-22 23:25 ` Thomas Rast
2010-02-23 0:21 ` Junio C Hamano
2010-02-22 0:10 ` [PATCH v4 03/11] Documentation: document post-rewrite hook Thomas Rast
2010-02-22 0:10 ` [PATCH v4 04/11] commit --amend: invoke " Thomas Rast
2010-02-22 0:10 ` [PATCH v4 05/11] rebase: " Thomas Rast
2010-02-22 0:10 ` [PATCH v4 06/11] rebase -i: " Thomas Rast
2010-02-22 0:10 ` [PATCH v4 07/11] notes: implement 'git notes copy --stdin' Thomas Rast
2010-02-22 0:10 ` [PATCH v4 08/11] notes: implement helpers needed for note copying during rewrite Thomas Rast
2010-02-22 0:10 ` [PATCH v4 09/11] rebase: support automatic notes copying Thomas Rast
2010-02-22 0:10 ` [PATCH v4 10/11] commit --amend: copy notes to the new commit Thomas Rast
2010-02-22 0:10 ` [PATCH v4 11/11] notes: add shorthand --ref to override GIT_NOTES_REF Thomas Rast
2010-02-22 0:25 ` [PATCH v4 00/11] several notes refs, post-rewrite, notes rewriting Junio C Hamano
2010-02-22 0:32 ` Thomas Rast
2010-02-23 0:42 ` [PATCH v5 " Thomas Rast
2010-02-23 0:42 ` [PATCH v5 01/11] test-lib: unset GIT_NOTES_REF to stop it from influencing tests Thomas Rast
2010-02-23 0:42 ` [PATCH v5 02/11] Support showing notes from more than one notes tree Thomas Rast
2010-02-23 1:47 ` Junio C Hamano
2010-02-23 17:10 ` Thomas Rast
2010-02-23 17:34 ` [PATCH] format-patch: learn to fill comment section of email from notes Thomas Rast
2010-02-23 17:34 ` [PATCH] BROKEN -- " Thomas Rast
2010-02-23 17:37 ` Thomas Rast
2010-02-24 7:45 ` Stephen Boyd
2010-02-23 21:56 ` [PATCH] " Junio C Hamano
2010-03-10 14:08 ` Thomas Rast
2010-02-23 0:42 ` [PATCH v5 03/11] Documentation: document post-rewrite hook Thomas Rast
2010-02-23 0:42 ` [PATCH v5 04/11] commit --amend: invoke " Thomas Rast
2010-02-23 0:42 ` [PATCH v5 05/11] rebase: " Thomas Rast
2010-02-23 0:42 ` [PATCH v5 06/11] rebase -i: " Thomas Rast
2010-02-24 6:15 ` Junio C Hamano
2010-02-23 0:42 ` [PATCH v5 07/11] notes: implement 'git notes copy --stdin' Thomas Rast
2010-02-23 0:42 ` [PATCH v5 08/11] notes: implement helpers needed for note copying during rewrite Thomas Rast
2010-02-23 0:42 ` [PATCH v5 09/11] rebase: support automatic notes copying Thomas Rast
2010-02-25 3:58 ` Junio C Hamano
2010-03-10 14:03 ` [PATCH v6 00/13] several notes refs, post-rewrite, notes rewriting Thomas Rast
2010-03-10 14:03 ` [PATCH v6 01/13] test-lib: unset GIT_NOTES_REF to stop it from influencing tests Thomas Rast
2010-03-11 8:55 ` Johan Herland
2010-03-10 14:03 ` [PATCH v6 02/13] Support showing notes from more than one notes tree Thomas Rast
2010-03-11 10:03 ` Johan Herland [this message]
2010-03-12 17:04 ` [PATCH v7 00/13] tr/display-notes Thomas Rast
2010-03-12 17:04 ` [PATCH v7 01/13] test-lib: unset GIT_NOTES_REF to stop it from influencing tests Thomas Rast
2010-03-12 17:04 ` [PATCH v7 02/13] Support showing notes from more than one notes tree Thomas Rast
2010-03-12 17:04 ` [PATCH v7 03/13] Documentation: document post-rewrite hook Thomas Rast
2010-03-12 17:04 ` [PATCH v7 04/13] commit --amend: invoke " Thomas Rast
2010-03-12 17:04 ` [PATCH v7 05/13] rebase: " Thomas Rast
2010-03-12 17:04 ` [PATCH v7 06/13] rebase -i: " Thomas Rast
2010-03-12 17:04 ` [PATCH v7 07/13] notes: implement 'git notes copy --stdin' Thomas Rast
2010-06-14 23:40 ` [PATCH] notes: Initialize variable to appease Sun Studio Ævar Arnfjörð Bjarmason
2010-06-19 4:52 ` Junio C Hamano
2010-06-19 11:58 ` Ævar Arnfjörð Bjarmason
2010-06-21 20:53 ` Ramsay Jones
2010-03-12 17:04 ` [PATCH v7 08/13] notes: implement helpers needed for note copying during rewrite Thomas Rast
2010-03-12 17:04 ` [PATCH v7 09/13] rebase: support automatic notes copying Thomas Rast
2010-03-12 17:04 ` [PATCH v7 10/13] commit --amend: copy notes to the new commit Thomas Rast
2010-03-12 17:04 ` [PATCH v7 11/13] notes: add shorthand --ref to override GIT_NOTES_REF Thomas Rast
2010-03-12 17:04 ` [PATCH v7 12/13] notes: track whether notes_trees were changed at all Thomas Rast
2010-03-12 17:04 ` [PATCH v7 13/13] git-notes(1): add a section about the meaning of history Thomas Rast
2010-03-10 14:03 ` [PATCH v6 03/13] Documentation: document post-rewrite hook Thomas Rast
2010-03-10 14:05 ` [PATCH v6 04/13] commit --amend: invoke " Thomas Rast
2010-03-10 14:05 ` [PATCH v6 05/13] rebase: " Thomas Rast
2010-03-10 14:05 ` [PATCH v6 06/13] rebase -i: " Thomas Rast
2010-03-10 14:05 ` [PATCH v6 07/13] notes: implement 'git notes copy --stdin' Thomas Rast
2010-03-11 10:30 ` Johan Herland
2010-03-10 14:05 ` [PATCH v6 08/13] notes: implement helpers needed for note copying during rewrite Thomas Rast
2010-03-11 10:50 ` Johan Herland
2010-03-10 14:05 ` [PATCH v6 09/13] rebase: support automatic notes copying Thomas Rast
2010-03-10 14:05 ` [PATCH v6 10/13] commit --amend: copy notes to the new commit Thomas Rast
2010-03-10 14:05 ` [PATCH v6 11/13] notes: add shorthand --ref to override GIT_NOTES_REF Thomas Rast
2010-03-11 10:56 ` Johan Herland
2010-03-10 14:05 ` [PATCH v6 12/13] notes: track whether notes_trees were changed at all Thomas Rast
2010-03-11 10:58 ` Johan Herland
2010-03-10 14:06 ` [PATCH v6 13/13] git-notes(1): add a section about the meaning of history Thomas Rast
2010-03-11 10:59 ` Johan Herland
2010-03-10 21:23 ` [PATCH v6 00/13] several notes refs, post-rewrite, notes rewriting Junio C Hamano
2010-02-23 0:42 ` [PATCH v5 10/11] commit --amend: copy notes to the new commit Thomas Rast
2010-02-23 0:42 ` [PATCH v5 11/11] notes: add shorthand --ref to override GIT_NOTES_REF Thomas Rast
2010-02-23 0:49 ` [PATCH v5 00/11] several notes refs, post-rewrite, notes rewriting Junio C Hamano
2010-02-23 0:49 ` Thomas Rast
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201003111103.22671.johan@herland.net \
--to=johan@herland.net \
--cc=git@vger.kernel.org \
--cc=j6t@kdbg.org \
--cc=trast@student.ethz.ch \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).