{"thread":{"id":"52437","subject":"[PATCH 0/2] dl/format-patch-notes-config-fixup: clean up some leftoverbits","startedAt":"2019-12-12T00:48:46Z","lastAt":"2019-12-18T18:17:19Z","messageCount":5,"participants":["Denton Liu","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"387996","messageId":"cover.1576111643.git.liu.denton@gmail.com","threadId":"52437","inReplyTo":null,"subject":"[PATCH 0/2] dl/format-patch-notes-config-fixup: clean up some leftoverbits","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-12T00:49:44Z","receivedAt":"2019-12-12T00:48:46Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"This series gives 'dl/format-patch-notes-config-fixup' a few polishing\ntouches. First of all, we document the behaviour of multiple\n`format.notes` configuration variables so that end-users are aware of\nthe change.\n\nAlso, Eric Sunshine suggested some cleanup in the previous round, like\nbreaking the monolithic set_display_notes() into multiple smaller\nfunctions and not using the return value of the function to assign to\n`show_notes`.\n\nDenton Liu (2):\n  config/format.txt: clarify behavior of multiple format.notes\n  notes: break set_display_notes() into smaller functions\n\n Documentation/config/format.txt | 18 +++++++++++++-\n builtin/log.c                   |  7 +++++-\n notes.c                         | 43 ++++++++++++++++++---------------\n notes.h                         | 19 +++++++++------\n revision.c                      |  6 ++---\n revision.h                      |  2 +-\n 6 files changed, 62 insertions(+), 33 deletions(-)\n\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"387997","messageId":"af0991b2d7cd77fc077b31d4d67a556bcbd235fe.1576111643.git.liu.denton@gmail.com","threadId":"52437","inReplyTo":"cover.1576111643.git.liu.denton@gmail.com","subject":"[PATCH 1/2] config/format.txt: clarify behavior of multiple format.notes","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-12T00:49:47Z","receivedAt":"2019-12-12T00:48:48Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In 8164c961e1 (format-patch: use --notes behavior for format.notes,\n2019-12-09), we slightly tweaked the behavior of having multiple\n`format.notes` configuration variables. We did not update the\ndocumentation to reflect this, however.\n\nExplictly state the behavior of having multiple `format.notes`\nconfiguration variables so users are clear on what to expect.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n Documentation/config/format.txt | 18 +++++++++++++++++-\n 1 file changed, 17 insertions(+), 1 deletion(-)\n\ndiff --git a/Documentation/config/format.txt b/Documentation/config/format.txt\nindex 414a5a8a9d..3d708b0aec 100644\n--- a/Documentation/config/format.txt\n+++ b/Documentation/config/format.txt\n@@ -99,4 +99,20 @@ If one wishes to use the ref `ref/notes/true`, please use that literal\n instead.\n +\n This configuration can be specified multiple times in order to allow\n-multiple notes refs to be included.\n+multiple notes refs to be included. In that case, it will behave\n+similarly to multiple `--[no-]notes[=]` options passed in. That is, a\n+value of `true` will show the default notes, a value of `<ref>` will\n+also show notes from that notes ref and a value of `false` will negate\n+previous configurations and not show notes.\n++\n+For example,\n++\n+------------\n+[format]\n+\tnotes = true\n+\tnotes = foo\n+\tnotes = false\n+\tnotes = bar\n+------------\n++\n+will only show notes from `refs/notes/bar`.\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"387998","messageId":"96b908c91c19bc11da9f8d3b0d6e853517284540.1576111643.git.liu.denton@gmail.com","threadId":"52437","inReplyTo":"cover.1576111643.git.liu.denton@gmail.com","subject":"[PATCH 2/2] notes: break set_display_notes() into smaller functions","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-12T00:49:50Z","receivedAt":"2019-12-12T00:48:50Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In 8164c961e1 (format-patch: use --notes behavior for format.notes,\n2019-12-09), we introduced set_display_notes() which was a monolithic\nfunction with three mutually exclusive branches. Break the function up\ninto three small and simple functions that each are only responsible for\none task.\n\nThis family of functions accepts an `int *show_notes` instead of\nreturning a value suitable for assignment to `show_notes`. This is for\ntwo reasons. First of all, this guarantees that the external\n`show_notes` variable changes in lockstep with the\n`struct display_notes_opt`. Second, this prompts future developers to be\ncareful about doing something meaningful with this value. In fact, a\nNULL check is intentionally omitted because causing a segfault here\nwould tell the future developer that they are meant to use the value for\nsomething meaningful.\n\nOne alternative was making the family of functions accept a\n`struct rev_info *` instead of the `struct display_notes_opt *`, since\nthe former contains the `show_notes` field as well. This does not work\nbecause we have to call git_config() before repo_init_revisions().\nHowever, if we had a `struct rev_info`, we'd need to initialize it before\nit gets assigned values from git_config(). As a result, we break the\ncircular dependency by having standalone `int show_notes` and\n`struct display_notes_opt notes_opt` variables which temporarily hold\nvalues from git_config() until the values are copied over to `rev`.\n\nTo implement this change, we need to get a pointer to\n`rev_info::show_notes`. Unfortunately, this is not possible with\nbitfields and only direct-assignment is possible. Change\n`rev_info::show_notes` to a non-bitfield int so that we can get its\naddress.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n builtin/log.c |  7 ++++++-\n notes.c       | 43 +++++++++++++++++++++++--------------------\n notes.h       | 19 ++++++++++++-------\n revision.c    |  6 +++---\n revision.h    |  2 +-\n 5 files changed, 45 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 4225615e7f..b6d43a4a47 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -868,7 +868,12 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \t}\n \tif (!strcmp(var, \"format.notes\")) {\n \t\tint b = git_parse_maybe_bool(value);\n-\t\tshow_notes = set_display_notes(&notes_opt, b, b < 0 ? value : NULL);\n+\t\tif (b < 0)\n+\t\t\tenable_ref_display_notes(&notes_opt, &show_notes, value);\n+\t\telse if (b)\n+\t\t\tenable_default_display_notes(&notes_opt, &show_notes);\n+\t\telse\n+\t\t\tdisable_display_notes(&notes_opt, &show_notes);\n \t\treturn 0;\n \t}\n \ndiff --git a/notes.c b/notes.c\nindex c93feff4ab..3133bb181f 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1045,28 +1045,31 @@ void init_display_notes(struct display_notes_opt *opt)\n \topt->use_default_notes = -1;\n }\n \n-int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref)\n+void enable_default_display_notes(struct display_notes_opt *opt, int *show_notes)\n {\n-\tif (show_notes) {\n-\t\tif (opt_ref) {\n-\t\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\t\tstrbuf_addstr(&buf, opt_ref);\n-\t\t\texpand_notes_ref(&buf);\n-\t\t\tstring_list_append(&opt->extra_notes_refs,\n-\t\t\t\t\t   strbuf_detach(&buf, NULL));\n-\t\t} else {\n-\t\t\topt->use_default_notes = 1;\n-\t\t}\n-\t} else {\n-\t\topt->use_default_notes = -1;\n-\t\t/* we have been strdup'ing ourselves, so trick\n-\t\t * string_list into free()ing strings */\n-\t\topt->extra_notes_refs.strdup_strings = 1;\n-\t\tstring_list_clear(&opt->extra_notes_refs, 0);\n-\t\topt->extra_notes_refs.strdup_strings = 0;\n-\t}\n+\topt->use_default_notes = 1;\n+\t*show_notes = 1;\n+}\n \n-\treturn !!show_notes;\n+void enable_ref_display_notes(struct display_notes_opt *opt, int *show_notes,\n+\t\tconst char *ref) {\n+\tstruct strbuf buf = STRBUF_INIT;\n+\tstrbuf_addstr(&buf, ref);\n+\texpand_notes_ref(&buf);\n+\tstring_list_append(&opt->extra_notes_refs,\n+\t\t\tstrbuf_detach(&buf, NULL));\n+\t*show_notes = 1;\n+}\n+\n+void disable_display_notes(struct display_notes_opt *opt, int *show_notes)\n+{\n+\topt->use_default_notes = -1;\n+\t/* we have been strdup'ing ourselves, so trick\n+\t * string_list into free()ing strings */\n+\topt->extra_notes_refs.strdup_strings = 1;\n+\tstring_list_clear(&opt->extra_notes_refs, 0);\n+\topt->extra_notes_refs.strdup_strings = 0;\n+\t*show_notes = 0;\n }\n \n void load_display_notes(struct display_notes_opt *opt)\ndiff --git a/notes.h b/notes.h\nindex a476bfa066..3e78448181 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -266,14 +266,19 @@ struct display_notes_opt {\n void init_display_notes(struct display_notes_opt *opt);\n \n /*\n- * Set a display_notes_opt to a given state. 'show_notes' is a boolean\n- * representing whether or not to show notes. 'opt_ref' points to a\n- * string for the notes ref, or is NULL if the default notes should be\n- * used.\n- *\n- * Return 'show_notes' normalized to 1 or 0.\n+ * This family of functions enables or disables the display of notes. In\n+ * particular, 'enable_default_display_notes' will display the default notes,\n+ * 'enable_default_display_notes' will display the notes ref 'ref' and\n+ * 'disable_display_notes' will disable notes, including those added by previous\n+ * invocations of the 'enable_*_display_notes' functions.\n+ *\n+ * 'show_notes' is a points to a boolean which will be set to 1 if notes are\n+ * displayed, else 0. It must not be NULL.\n  */\n-int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref);\n+void enable_default_display_notes(struct display_notes_opt *opt, int *show_notes);\n+void enable_ref_display_notes(struct display_notes_opt *opt, int *show_notes,\n+\t\tconst char *ref);\n+void disable_display_notes(struct display_notes_opt *opt, int *show_notes);\n \n /*\n  * Load the notes machinery for displaying several notes trees.\ndiff --git a/revision.c b/revision.c\nindex c2d8d24939..1b12ed742b 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2172,7 +2172,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\t\tdie(\"'%s': not a non-negative integer\", arg);\n \t\trevs->expand_tabs_in_log = val;\n \t} else if (!strcmp(arg, \"--show-notes\") || !strcmp(arg, \"--notes\")) {\n-\t\trevs->show_notes = set_display_notes(&revs->notes_opt, 1, NULL);\n+\t\tenable_default_display_notes(&revs->notes_opt, &revs->show_notes);\n \t\trevs->show_notes_given = 1;\n \t} else if (!strcmp(arg, \"--show-signature\")) {\n \t\trevs->show_signature = 1;\n@@ -2191,10 +2191,10 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\tif (starts_with(arg, \"--show-notes=\") &&\n \t\t    revs->notes_opt.use_default_notes < 0)\n \t\t\trevs->notes_opt.use_default_notes = 1;\n-\t\trevs->show_notes = set_display_notes(&revs->notes_opt, 1, optarg);\n+\t\tenable_ref_display_notes(&revs->notes_opt, &revs->show_notes, optarg);\n \t\trevs->show_notes_given = 1;\n \t} else if (!strcmp(arg, \"--no-notes\")) {\n-\t\trevs->show_notes = set_display_notes(&revs->notes_opt, 0, NULL);\n+\t\tdisable_display_notes(&revs->notes_opt, &revs->show_notes);\n \t\trevs->show_notes_given = 1;\n \t} else if (!strcmp(arg, \"--standard-notes\")) {\n \t\trevs->show_notes_given = 1;\ndiff --git a/revision.h b/revision.h\nindex 4134dc6029..72520780f4 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -177,10 +177,10 @@ struct rev_info {\n \t\t\talways_show_header:1;\n \n \t/* Format info */\n+\tint\t\tshow_notes;\n \tunsigned int\tshown_one:1,\n \t\t\tshown_dashes:1,\n \t\t\tshow_merge:1,\n-\t\t\tshow_notes:1,\n \t\t\tshow_notes_given:1,\n \t\t\tshow_signature:1,\n \t\t\tpretty_given:1,\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"388443","messageId":"CAPig+cRVyfWceVZFgRuRyL-B3TMnX9fObC84t2LHsNoXm8TuqQ@mail.gmail.com","threadId":"52437","inReplyTo":"96b908c91c19bc11da9f8d3b0d6e853517284540.1576111643.git.liu.denton@gmail.com","subject":"Re: [PATCH 2/2] notes: break set_display_notes() into smaller functions","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-18T16:18:10Z","receivedAt":"2019-12-18T16:18:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 11, 2019 at 7:48 PM Denton Liu <liu.denton@gmail.com> wrote:\n> In 8164c961e1 (format-patch: use --notes behavior for format.notes,\n> 2019-12-09), we introduced set_display_notes() which was a monolithic\n> function with three mutually exclusive branches. Break the function up\n> into three small and simple functions that each are only responsible for\n> one task.\n> [...]\n> Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> ---\n> diff --git a/notes.c b/notes.c\n> @@ -1045,28 +1045,31 @@ void init_display_notes(struct display_notes_opt *opt)\n> +void disable_display_notes(struct display_notes_opt *opt, int *show_notes)\n> +{\n> +       opt->use_default_notes = -1;\n> +       /* we have been strdup'ing ourselves, so trick\n> +        * string_list into free()ing strings */\n\n    /*\n     * Multi-line comment\n     * style.\n     */\n\nThough, apparently the formatting used here predates your changes; you\nsimply moved the code (and comment) around.\n\n> +       opt->extra_notes_refs.strdup_strings = 1;\n> +       string_list_clear(&opt->extra_notes_refs, 0);\n> +       opt->extra_notes_refs.strdup_strings = 0;\n> +       *show_notes = 0;\n>  }\n> diff --git a/notes.h b/notes.h\n> @@ -266,14 +266,19 @@ struct display_notes_opt {\n>  /*\n> + * This family of functions enables or disables the display of notes. In\n> + * particular, 'enable_default_display_notes' will display the default notes,\n> + * 'enable_default_display_notes' will display the notes ref 'ref' and\n\ns/enable_default_display_notes/enable_ref_display_notes/\n\n> + * 'disable_display_notes' will disable notes, including those added by previous\n> + * invocations of the 'enable_*_display_notes' functions.\n> + *\n> + * 'show_notes' is a points to a boolean which will be set to 1 if notes are\n\ns/points/pointer/\n\n> + * displayed, else 0. It must not be NULL.\n>   */\n"},{"id":"388453","messageId":"34bbdc1af3d76f556e26ac92fff35bf2ac853470.1576692900.git.liu.denton@gmail.com","threadId":"52437","inReplyTo":"CAPig+cRVyfWceVZFgRuRyL-B3TMnX9fObC84t2LHsNoXm8TuqQ@mail.gmail.com","subject":"[PATCH] notes.h: fix typos in comment","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-18T18:17:43Z","receivedAt":"2019-12-18T18:17:19Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In 1d7297513d (notes: break set_display_notes() into smaller functions,\n2019-12-11), we introduced a comment which had a couple of typos. In the\nfirst typo, we referenced 'enable_default_display_notes' instead of\n'enable_ref_display_notes'. In the second typo, we wrote \"is a points to\"\ninstead of \"is a pointer to\". Correct both of these typos.\n\nReported-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\nThis patch is based on top of 'dl/format-patch-notes-config-fixup'.\nThanks for catching the typo, Eric!\n\n notes.h | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/notes.h b/notes.h\nindex 3e78448181..bbab1961ca 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -268,11 +268,11 @@ void init_display_notes(struct display_notes_opt *opt);\n /*\n  * This family of functions enables or disables the display of notes. In\n  * particular, 'enable_default_display_notes' will display the default notes,\n- * 'enable_default_display_notes' will display the notes ref 'ref' and\n+ * 'enable_ref_display_notes' will display the notes ref 'ref' and\n  * 'disable_display_notes' will disable notes, including those added by previous\n  * invocations of the 'enable_*_display_notes' functions.\n  *\n- * 'show_notes' is a points to a boolean which will be set to 1 if notes are\n+ * 'show_notes' is a pointer to a boolean which will be set to 1 if notes are\n  * displayed, else 0. It must not be NULL.\n  */\n void enable_default_display_notes(struct display_notes_opt *opt, int *show_notes);\n-- \n2.24.1.703.g2f499f1283\n\n"}]}