* [PATCH 0/2] dl/format-patch-notes-config-fixup: clean up some leftoverbits @ 2019-12-12 0:49 Denton Liu 2019-12-12 0:49 ` [PATCH 1/2] config/format.txt: clarify behavior of multiple format.notes Denton Liu 2019-12-12 0:49 ` [PATCH 2/2] notes: break set_display_notes() into smaller functions Denton Liu 0 siblings, 2 replies; 5+ messages in thread From: Denton Liu @ 2019-12-12 0:49 UTC (permalink / raw) To: Git Mailing List; +Cc: Elijah Newren, Eric Sunshine, Philip Oakley This series gives 'dl/format-patch-notes-config-fixup' a few polishing touches. First of all, we document the behaviour of multiple `format.notes` configuration variables so that end-users are aware of the change. Also, Eric Sunshine suggested some cleanup in the previous round, like breaking the monolithic set_display_notes() into multiple smaller functions and not using the return value of the function to assign to `show_notes`. Denton Liu (2): config/format.txt: clarify behavior of multiple format.notes notes: break set_display_notes() into smaller functions Documentation/config/format.txt | 18 +++++++++++++- builtin/log.c | 7 +++++- notes.c | 43 ++++++++++++++++++--------------- notes.h | 19 +++++++++------ revision.c | 6 ++--- revision.h | 2 +- 6 files changed, 62 insertions(+), 33 deletions(-) -- 2.24.0.627.geba02921db ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/2] config/format.txt: clarify behavior of multiple format.notes 2019-12-12 0:49 [PATCH 0/2] dl/format-patch-notes-config-fixup: clean up some leftoverbits Denton Liu @ 2019-12-12 0:49 ` Denton Liu 2019-12-12 0:49 ` [PATCH 2/2] notes: break set_display_notes() into smaller functions Denton Liu 1 sibling, 0 replies; 5+ messages in thread From: Denton Liu @ 2019-12-12 0:49 UTC (permalink / raw) To: Git Mailing List; +Cc: Elijah Newren, Eric Sunshine, Philip Oakley In 8164c961e1 (format-patch: use --notes behavior for format.notes, 2019-12-09), we slightly tweaked the behavior of having multiple `format.notes` configuration variables. We did not update the documentation to reflect this, however. Explictly state the behavior of having multiple `format.notes` configuration variables so users are clear on what to expect. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- Documentation/config/format.txt | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt index 414a5a8a9d..3d708b0aec 100644 --- a/Documentation/config/format.txt +++ b/Documentation/config/format.txt @@ -99,4 +99,20 @@ If one wishes to use the ref `ref/notes/true`, please use that literal instead. + This configuration can be specified multiple times in order to allow -multiple notes refs to be included. +multiple notes refs to be included. In that case, it will behave +similarly to multiple `--[no-]notes[=]` options passed in. That is, a +value of `true` will show the default notes, a value of `<ref>` will +also show notes from that notes ref and a value of `false` will negate +previous configurations and not show notes. ++ +For example, ++ +------------ +[format] + notes = true + notes = foo + notes = false + notes = bar +------------ ++ +will only show notes from `refs/notes/bar`. -- 2.24.0.627.geba02921db ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/2] notes: break set_display_notes() into smaller functions 2019-12-12 0:49 [PATCH 0/2] dl/format-patch-notes-config-fixup: clean up some leftoverbits Denton Liu 2019-12-12 0:49 ` [PATCH 1/2] config/format.txt: clarify behavior of multiple format.notes Denton Liu @ 2019-12-12 0:49 ` Denton Liu 2019-12-18 16:18 ` Eric Sunshine 1 sibling, 1 reply; 5+ messages in thread From: Denton Liu @ 2019-12-12 0:49 UTC (permalink / raw) To: Git Mailing List; +Cc: Elijah Newren, Eric Sunshine, Philip Oakley In 8164c961e1 (format-patch: use --notes behavior for format.notes, 2019-12-09), we introduced set_display_notes() which was a monolithic function with three mutually exclusive branches. Break the function up into three small and simple functions that each are only responsible for one task. This family of functions accepts an `int *show_notes` instead of returning a value suitable for assignment to `show_notes`. This is for two reasons. First of all, this guarantees that the external `show_notes` variable changes in lockstep with the `struct display_notes_opt`. Second, this prompts future developers to be careful about doing something meaningful with this value. In fact, a NULL check is intentionally omitted because causing a segfault here would tell the future developer that they are meant to use the value for something meaningful. One alternative was making the family of functions accept a `struct rev_info *` instead of the `struct display_notes_opt *`, since the former contains the `show_notes` field as well. This does not work because we have to call git_config() before repo_init_revisions(). However, if we had a `struct rev_info`, we'd need to initialize it before it gets assigned values from git_config(). As a result, we break the circular dependency by having standalone `int show_notes` and `struct display_notes_opt notes_opt` variables which temporarily hold values from git_config() until the values are copied over to `rev`. To implement this change, we need to get a pointer to `rev_info::show_notes`. Unfortunately, this is not possible with bitfields and only direct-assignment is possible. Change `rev_info::show_notes` to a non-bitfield int so that we can get its address. Signed-off-by: Denton Liu <liu.denton@gmail.com> --- builtin/log.c | 7 ++++++- notes.c | 43 +++++++++++++++++++++++-------------------- notes.h | 19 ++++++++++++------- revision.c | 6 +++--- revision.h | 2 +- 5 files changed, 45 insertions(+), 32 deletions(-) diff --git a/builtin/log.c b/builtin/log.c index 4225615e7f..b6d43a4a47 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -868,7 +868,12 @@ static int git_format_config(const char *var, const char *value, void *cb) } if (!strcmp(var, "format.notes")) { int b = git_parse_maybe_bool(value); - show_notes = set_display_notes(¬es_opt, b, b < 0 ? value : NULL); + if (b < 0) + enable_ref_display_notes(¬es_opt, &show_notes, value); + else if (b) + enable_default_display_notes(¬es_opt, &show_notes); + else + disable_display_notes(¬es_opt, &show_notes); return 0; } diff --git a/notes.c b/notes.c index c93feff4ab..3133bb181f 100644 --- a/notes.c +++ b/notes.c @@ -1045,28 +1045,31 @@ void init_display_notes(struct display_notes_opt *opt) opt->use_default_notes = -1; } -int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref) +void enable_default_display_notes(struct display_notes_opt *opt, int *show_notes) { - if (show_notes) { - if (opt_ref) { - struct strbuf buf = STRBUF_INIT; - strbuf_addstr(&buf, opt_ref); - expand_notes_ref(&buf); - string_list_append(&opt->extra_notes_refs, - strbuf_detach(&buf, NULL)); - } else { - opt->use_default_notes = 1; - } - } else { - opt->use_default_notes = -1; - /* we have been strdup'ing ourselves, so trick - * string_list into free()ing strings */ - opt->extra_notes_refs.strdup_strings = 1; - string_list_clear(&opt->extra_notes_refs, 0); - opt->extra_notes_refs.strdup_strings = 0; - } + opt->use_default_notes = 1; + *show_notes = 1; +} - return !!show_notes; +void enable_ref_display_notes(struct display_notes_opt *opt, int *show_notes, + const char *ref) { + struct strbuf buf = STRBUF_INIT; + strbuf_addstr(&buf, ref); + expand_notes_ref(&buf); + string_list_append(&opt->extra_notes_refs, + strbuf_detach(&buf, NULL)); + *show_notes = 1; +} + +void disable_display_notes(struct display_notes_opt *opt, int *show_notes) +{ + opt->use_default_notes = -1; + /* we have been strdup'ing ourselves, so trick + * string_list into free()ing strings */ + opt->extra_notes_refs.strdup_strings = 1; + string_list_clear(&opt->extra_notes_refs, 0); + opt->extra_notes_refs.strdup_strings = 0; + *show_notes = 0; } void load_display_notes(struct display_notes_opt *opt) diff --git a/notes.h b/notes.h index a476bfa066..3e78448181 100644 --- a/notes.h +++ b/notes.h @@ -266,14 +266,19 @@ struct display_notes_opt { void init_display_notes(struct display_notes_opt *opt); /* - * Set a display_notes_opt to a given state. 'show_notes' is a boolean - * representing whether or not to show notes. 'opt_ref' points to a - * string for the notes ref, or is NULL if the default notes should be - * used. - * - * Return 'show_notes' normalized to 1 or 0. + * This family of functions enables or disables the display of notes. In + * particular, 'enable_default_display_notes' will display the default notes, + * 'enable_default_display_notes' will display the notes ref 'ref' and + * 'disable_display_notes' will disable notes, including those added by previous + * invocations of the 'enable_*_display_notes' functions. + * + * 'show_notes' is a points to a boolean which will be set to 1 if notes are + * displayed, else 0. It must not be NULL. */ -int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref); +void enable_default_display_notes(struct display_notes_opt *opt, int *show_notes); +void enable_ref_display_notes(struct display_notes_opt *opt, int *show_notes, + const char *ref); +void disable_display_notes(struct display_notes_opt *opt, int *show_notes); /* * Load the notes machinery for displaying several notes trees. diff --git a/revision.c b/revision.c index c2d8d24939..1b12ed742b 100644 --- a/revision.c +++ b/revision.c @@ -2172,7 +2172,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg die("'%s': not a non-negative integer", arg); revs->expand_tabs_in_log = val; } else if (!strcmp(arg, "--show-notes") || !strcmp(arg, "--notes")) { - revs->show_notes = set_display_notes(&revs->notes_opt, 1, NULL); + enable_default_display_notes(&revs->notes_opt, &revs->show_notes); revs->show_notes_given = 1; } else if (!strcmp(arg, "--show-signature")) { revs->show_signature = 1; @@ -2191,10 +2191,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg if (starts_with(arg, "--show-notes=") && revs->notes_opt.use_default_notes < 0) revs->notes_opt.use_default_notes = 1; - revs->show_notes = set_display_notes(&revs->notes_opt, 1, optarg); + enable_ref_display_notes(&revs->notes_opt, &revs->show_notes, optarg); revs->show_notes_given = 1; } else if (!strcmp(arg, "--no-notes")) { - revs->show_notes = set_display_notes(&revs->notes_opt, 0, NULL); + disable_display_notes(&revs->notes_opt, &revs->show_notes); revs->show_notes_given = 1; } else if (!strcmp(arg, "--standard-notes")) { revs->show_notes_given = 1; diff --git a/revision.h b/revision.h index 4134dc6029..72520780f4 100644 --- a/revision.h +++ b/revision.h @@ -177,10 +177,10 @@ struct rev_info { always_show_header:1; /* Format info */ + int show_notes; unsigned int shown_one:1, shown_dashes:1, show_merge:1, - show_notes:1, show_notes_given:1, show_signature:1, pretty_given:1, -- 2.24.0.627.geba02921db ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 2/2] notes: break set_display_notes() into smaller functions 2019-12-12 0:49 ` [PATCH 2/2] notes: break set_display_notes() into smaller functions Denton Liu @ 2019-12-18 16:18 ` Eric Sunshine 2019-12-18 18:17 ` [PATCH] notes.h: fix typos in comment Denton Liu 0 siblings, 1 reply; 5+ messages in thread From: Eric Sunshine @ 2019-12-18 16:18 UTC (permalink / raw) To: Denton Liu; +Cc: Git Mailing List, Elijah Newren, Philip Oakley On Wed, Dec 11, 2019 at 7:48 PM Denton Liu <liu.denton@gmail.com> wrote: > In 8164c961e1 (format-patch: use --notes behavior for format.notes, > 2019-12-09), we introduced set_display_notes() which was a monolithic > function with three mutually exclusive branches. Break the function up > into three small and simple functions that each are only responsible for > one task. > [...] > Signed-off-by: Denton Liu <liu.denton@gmail.com> > --- > diff --git a/notes.c b/notes.c > @@ -1045,28 +1045,31 @@ void init_display_notes(struct display_notes_opt *opt) > +void disable_display_notes(struct display_notes_opt *opt, int *show_notes) > +{ > + opt->use_default_notes = -1; > + /* we have been strdup'ing ourselves, so trick > + * string_list into free()ing strings */ /* * Multi-line comment * style. */ Though, apparently the formatting used here predates your changes; you simply moved the code (and comment) around. > + opt->extra_notes_refs.strdup_strings = 1; > + string_list_clear(&opt->extra_notes_refs, 0); > + opt->extra_notes_refs.strdup_strings = 0; > + *show_notes = 0; > } > diff --git a/notes.h b/notes.h > @@ -266,14 +266,19 @@ struct display_notes_opt { > /* > + * This family of functions enables or disables the display of notes. In > + * particular, 'enable_default_display_notes' will display the default notes, > + * 'enable_default_display_notes' will display the notes ref 'ref' and s/enable_default_display_notes/enable_ref_display_notes/ > + * 'disable_display_notes' will disable notes, including those added by previous > + * invocations of the 'enable_*_display_notes' functions. > + * > + * 'show_notes' is a points to a boolean which will be set to 1 if notes are s/points/pointer/ > + * displayed, else 0. It must not be NULL. > */ ^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH] notes.h: fix typos in comment 2019-12-18 16:18 ` Eric Sunshine @ 2019-12-18 18:17 ` Denton Liu 0 siblings, 0 replies; 5+ messages in thread From: Denton Liu @ 2019-12-18 18:17 UTC (permalink / raw) To: Git Mailing List; +Cc: Eric Sunshine, Elijah Newren, Philip Oakley In 1d7297513d (notes: break set_display_notes() into smaller functions, 2019-12-11), we introduced a comment which had a couple of typos. In the first typo, we referenced 'enable_default_display_notes' instead of 'enable_ref_display_notes'. In the second typo, we wrote "is a points to" instead of "is a pointer to". Correct both of these typos. Reported-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: Denton Liu <liu.denton@gmail.com> --- This patch is based on top of 'dl/format-patch-notes-config-fixup'. Thanks for catching the typo, Eric! notes.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/notes.h b/notes.h index 3e78448181..bbab1961ca 100644 --- a/notes.h +++ b/notes.h @@ -268,11 +268,11 @@ void init_display_notes(struct display_notes_opt *opt); /* * This family of functions enables or disables the display of notes. In * particular, 'enable_default_display_notes' will display the default notes, - * 'enable_default_display_notes' will display the notes ref 'ref' and + * 'enable_ref_display_notes' will display the notes ref 'ref' and * 'disable_display_notes' will disable notes, including those added by previous * invocations of the 'enable_*_display_notes' functions. * - * 'show_notes' is a points to a boolean which will be set to 1 if notes are + * 'show_notes' is a pointer to a boolean which will be set to 1 if notes are * displayed, else 0. It must not be NULL. */ void enable_default_display_notes(struct display_notes_opt *opt, int *show_notes); -- 2.24.1.703.g2f499f1283 ^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-12-18 18:17 UTC | newest] Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-12 0:49 [PATCH 0/2] dl/format-patch-notes-config-fixup: clean up some leftoverbits Denton Liu 2019-12-12 0:49 ` [PATCH 1/2] config/format.txt: clarify behavior of multiple format.notes Denton Liu 2019-12-12 0:49 ` [PATCH 2/2] notes: break set_display_notes() into smaller functions Denton Liu 2019-12-18 16:18 ` Eric Sunshine 2019-12-18 18:17 ` [PATCH] notes.h: fix typos in comment Denton Liu
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).