{"thread":{"id":"52414","subject":"[PATCH 0/5] format-patch: improve handling of `format.notes`","startedAt":"2019-12-09T13:10:41Z","lastAt":"2019-12-10T11:22:48Z","messageCount":11,"participants":["Denton Liu","Eric Sunshine","Junio C Hamano","Elijah Newren","Philip Oakley"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"387765","messageId":"cover.1575896661.git.liu.denton@gmail.com","threadId":"52414","inReplyTo":null,"subject":"[PATCH 0/5] format-patch: improve handling of `format.notes`","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-09T13:10:36Z","receivedAt":"2019-12-09T13:10:41Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In this email[1], Elijah pointed out that the handling of multiple\n`format.notes` configurations could be buggy. If we had\n`format.notes = <ref1>`, `format.notes = false` and\n`format.notes = <ref2>`, the behaviour is ambiguous. This series uses\nthe way `--notes=<ref1> --no-notes --notes=<ref2>` is handled as a model\nand structures the handling of `format.notes` in a similar manner,\nallowing one `format.notes = false` to override previous configs.\n\nAlso, in the same email, it was pointed out that git_config() should be\ncalled before repo_init_revisions(). In 13cdf78094 (format-patch: teach\nformat.notes config option, 2019-05-16), the order was reversed. This\nseries changes it back such that git_config() is called before\nrepo_init_revisions().\n\nThis series is based on top of 'dl/format-patch-notes-config'.\n\nIt has minor textual conflicts with 'pu'. The merge resolution can be found at\nhttps://github.com/Denton-L/git.git on branch\n'published/published/pu-format-patch-notes-config'.\n\n[1]: https://lore.kernel.org/git/CABPp-BF44+6gvZVNimKf-k7AWbOjw3OK-cJeFunNR96wvZGkcw@mail.gmail.com/\n\nDenton Liu (5):\n  notes: rename to load_display_notes()\n  notes: create init_display_notes() helper\n  notes: extract logic into set_display_notes()\n  format-patch: use --notes behavior for format.notes\n  format-patch: move git_config() before repo_init_revisions()\n\n builtin/log.c           | 26 +++++++++-----------------\n notes.c                 | 30 ++++++++++++++++++++++++++++++\n notes.h                 | 21 ++++++++++++++++++---\n revision.c              | 22 +++++-----------------\n t/t4014-format-patch.sh | 32 ++++++++++++++++++++++++++++++++\n 5 files changed, 94 insertions(+), 37 deletions(-)\n\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"387766","messageId":"4132d5bb7b354355a6dbe8049601f166b5de8fa8.1575896661.git.liu.denton@gmail.com","threadId":"52414","inReplyTo":"cover.1575896661.git.liu.denton@gmail.com","subject":"[PATCH 1/5] notes: rename to load_display_notes()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-09T13:10:39Z","receivedAt":"2019-12-09T13:10:44Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"According to the function comment, init_display_notes() was supposed to\n\"Load the notes machinery for displaying several notes trees.\" Rename\nthis function to load_display_notes() so that its use is more accurately\nrepresented.\n\nThis is done because, in a future commit, we will reuse the name\ninit_display_notes().\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n builtin/log.c | 4 ++--\n notes.c       | 2 +-\n notes.h       | 6 +++---\n 3 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex dad63cffc6..622d6a6cb1 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -202,7 +202,7 @@ static void cmd_log_init_finish(int argc, const char **argv, const char *prefix,\n \tif (!rev->show_notes_given && (!rev->pretty_given || w.notes))\n \t\trev->show_notes = 1;\n \tif (rev->show_notes)\n-\t\tinit_display_notes(&rev->notes_opt);\n+\t\tload_display_notes(&rev->notes_opt);\n \n \tif ((rev->diffopt.pickaxe_opts & DIFF_PICKAXE_KINDS_MASK) ||\n \t    rev->diffopt.filter || rev->diffopt.flags.follow_renames)\n@@ -1749,7 +1749,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\trev.diffopt.flags.binary = 1;\n \n \tif (rev.show_notes)\n-\t\tinit_display_notes(&rev.notes_opt);\n+\t\tload_display_notes(&rev.notes_opt);\n \n \tif (!output_directory && !use_stdout)\n \t\toutput_directory = config_output_directory;\ndiff --git a/notes.c b/notes.c\nindex 532ec37865..fd6cef14a3 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1039,7 +1039,7 @@ struct notes_tree **load_notes_trees(struct string_list *refs, int flags)\n \treturn trees;\n }\n \n-void init_display_notes(struct display_notes_opt *opt)\n+void load_display_notes(struct display_notes_opt *opt)\n {\n \tchar *display_ref_env;\n \tint load_config_refs = 0;\ndiff --git a/notes.h b/notes.h\nindex 414bc6855a..1ce528442a 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -272,18 +272,18 @@ struct display_notes_opt {\n  * - extra_notes_refs may contain a list of globs (in the same style\n  *   as notes.displayRef) where notes should be loaded from.\n  */\n-void init_display_notes(struct display_notes_opt *opt);\n+void load_display_notes(struct display_notes_opt *opt);\n \n /*\n  * Append notes for the given 'object_sha1' from all trees set up by\n- * init_display_notes() to 'sb'.  The 'flags' are a bitwise\n+ * load_display_notes() to 'sb'.  The 'flags' are a bitwise\n  * combination of\n  *\n  * - NOTES_SHOW_HEADER: add a 'Notes (refname):' header\n  *\n  * - NOTES_INDENT: indent the notes by 4 places\n  *\n- * You *must* call init_display_notes() before using this function.\n+ * You *must* call load_display_notes() before using this function.\n  */\n void format_display_notes(const struct object_id *object_oid,\n \t\t\t  struct strbuf *sb, const char *output_encoding, int raw);\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"387767","messageId":"bb66c95f43e2cb727a9aafbce8c70f25df513a13.1575896661.git.liu.denton@gmail.com","threadId":"52414","inReplyTo":"cover.1575896661.git.liu.denton@gmail.com","subject":"[PATCH 2/5] notes: create init_display_notes() helper","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-09T13:10:41Z","receivedAt":"2019-12-09T13:10:46Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"We currently open code the initialization for revs->notes_opt. Abstract\nthis away into a helper function so that the logic can be reused in a\nfuture commit.\n\nThis is slightly wasteful as we memset the struct twice but this is only\nrun once so it shouldn't have any major effect.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n notes.c    | 6 ++++++\n notes.h    | 5 +++++\n revision.c | 2 +-\n 3 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/notes.c b/notes.c\nindex fd6cef14a3..53d1e7767c 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1039,6 +1039,12 @@ struct notes_tree **load_notes_trees(struct string_list *refs, int flags)\n \treturn trees;\n }\n \n+void init_display_notes(struct display_notes_opt *opt)\n+{\n+\tmemset(opt, 0, sizeof(*opt));\n+\topt->use_default_notes = -1;\n+}\n+\n void load_display_notes(struct display_notes_opt *opt)\n {\n \tchar *display_ref_env;\ndiff --git a/notes.h b/notes.h\nindex 1ce528442a..c0b712371c 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -260,6 +260,11 @@ struct display_notes_opt {\n \tstruct string_list extra_notes_refs;\n };\n \n+/*\n+ * Initialize a display_notes_opt to its default value.\n+ */\n+void init_display_notes(struct display_notes_opt *opt);\n+\n /*\n  * Load the notes machinery for displaying several notes trees.\n  *\ndiff --git a/revision.c b/revision.c\nindex d4aaf0ef25..24ad974590 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1637,7 +1637,7 @@ void repo_init_revisions(struct repository *r,\n \t\trevs->diffopt.prefix_length = strlen(prefix);\n \t}\n \n-\trevs->notes_opt.use_default_notes = -1;\n+\tinit_display_notes(&revs->notes_opt);\n }\n \n static void add_pending_commit_list(struct rev_info *revs,\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"387768","messageId":"62543250c4ea0e0327f974cb90b294c60b525982.1575896661.git.liu.denton@gmail.com","threadId":"52414","inReplyTo":"cover.1575896661.git.liu.denton@gmail.com","subject":"[PATCH 3/5] notes: extract logic into set_display_notes()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-09T13:10:44Z","receivedAt":"2019-12-09T13:10:48Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Instead of open coding the logic that tweaks the variables in\n`struct display_notes_opt` within handle_revision_opt(), abstract away the\nlogic into set_display_notes() so that it can be reused.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n notes.c    | 24 ++++++++++++++++++++++++\n notes.h    | 10 ++++++++++\n revision.c | 20 ++++----------------\n 3 files changed, 38 insertions(+), 16 deletions(-)\n\ndiff --git a/notes.c b/notes.c\nindex 53d1e7767c..c93feff4ab 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1045,6 +1045,30 @@ 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+{\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+\n+\treturn !!show_notes;\n+}\n+\n void load_display_notes(struct display_notes_opt *opt)\n {\n \tchar *display_ref_env;\ndiff --git a/notes.h b/notes.h\nindex c0b712371c..a476bfa066 100644\n--- a/notes.h\n+++ b/notes.h\n@@ -265,6 +265,16 @@ struct display_notes_opt {\n  */\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+ */\n+int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref);\n+\n /*\n  * Load the notes machinery for displaying several notes trees.\n  *\ndiff --git a/revision.c b/revision.c\nindex 24ad974590..c2d8d24939 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -2172,9 +2172,8 @@ 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 = 1;\n+\t\trevs->show_notes = set_display_notes(&revs->notes_opt, 1, NULL);\n \t\trevs->show_notes_given = 1;\n-\t\trevs->notes_opt.use_default_notes = 1;\n \t} else if (!strcmp(arg, \"--show-signature\")) {\n \t\trevs->show_signature = 1;\n \t} else if (!strcmp(arg, \"--no-show-signature\")) {\n@@ -2189,25 +2188,14 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->track_first_time = 1;\n \t} else if (skip_prefix(arg, \"--show-notes=\", &optarg) ||\n \t\t   skip_prefix(arg, \"--notes=\", &optarg)) {\n-\t\tstruct strbuf buf = STRBUF_INIT;\n-\t\trevs->show_notes = 1;\n-\t\trevs->show_notes_given = 1;\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\tstrbuf_addstr(&buf, optarg);\n-\t\texpand_notes_ref(&buf);\n-\t\tstring_list_append(&revs->notes_opt.extra_notes_refs,\n-\t\t\t\t   strbuf_detach(&buf, NULL));\n+\t\trevs->show_notes = set_display_notes(&revs->notes_opt, 1, optarg);\n+\t\trevs->show_notes_given = 1;\n \t} else if (!strcmp(arg, \"--no-notes\")) {\n-\t\trevs->show_notes = 0;\n+\t\trevs->show_notes = set_display_notes(&revs->notes_opt, 0, NULL);\n \t\trevs->show_notes_given = 1;\n-\t\trevs->notes_opt.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\trevs->notes_opt.extra_notes_refs.strdup_strings = 1;\n-\t\tstring_list_clear(&revs->notes_opt.extra_notes_refs, 0);\n-\t\trevs->notes_opt.extra_notes_refs.strdup_strings = 0;\n \t} else if (!strcmp(arg, \"--standard-notes\")) {\n \t\trevs->show_notes_given = 1;\n \t\trevs->notes_opt.use_default_notes = 1;\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"387769","messageId":"67c88a37d24c5481ff0fcc314368ad20c5ce65e4.1575896661.git.liu.denton@gmail.com","threadId":"52414","inReplyTo":"cover.1575896661.git.liu.denton@gmail.com","subject":"[PATCH 4/5] format-patch: use --notes behavior for format.notes","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-09T13:10:46Z","receivedAt":"2019-12-09T13:10:51Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"When we had multiple `format.notes` config values where we had `<ref1>`,\n`false`, `<ref2>` (in that order), then we would print out the notes for\nboth `<ref1>` and `<ref2>`. This doesn't make sense, however, since we\nparse the config in a top-down manner and a `false` should be able to\noverride previous configurations, just like how `--no-notes` will\noverride previous `--notes`.\n\nDuplicate the logic that handles the `--[no-]notes[=]` option to\n`format.notes` for consistency. As a result, when parsing the config\nfrom top to bottom, `format.notes = true` will behave like `--notes`,\n`format.notes = <ref>` will behave like `--notes=<ref>` and\n`format.notes = false` will behave like `--no-notes`.\n\nThis change isn't strictly backwards compatible but since it is an edge\ncase where a sane user would not mix notes refs with `false` and this\nfeature is relatively new (released only in v2.23.0), this change should\nbe harmless.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n builtin/log.c           | 13 +------------\n t/t4014-format-patch.sh | 32 ++++++++++++++++++++++++++++++++\n 2 files changed, 33 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 622d6a6cb1..1f0405f72b 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -867,19 +867,8 @@ static int git_format_config(const char *var, const char *value, void *cb)\n \t\treturn 0;\n \t}\n \tif (!strcmp(var, \"format.notes\")) {\n-\t\tstruct strbuf buf = STRBUF_INIT;\n \t\tint b = git_parse_maybe_bool(value);\n-\t\tif (!b)\n-\t\t\treturn 0;\n-\t\trev->show_notes = 1;\n-\t\tif (b < 0) {\n-\t\t\tstrbuf_addstr(&buf, value);\n-\t\t\texpand_notes_ref(&buf);\n-\t\t\tstring_list_append(&rev->notes_opt.extra_notes_refs,\n-\t\t\t\t\tstrbuf_detach(&buf, NULL));\n-\t\t} else {\n-\t\t\trev->notes_opt.use_default_notes = 1;\n-\t\t}\n+\t\trev->show_notes = set_display_notes(&rev->notes_opt, b, b < 0 ? value : NULL);\n \t\treturn 0;\n \t}\n \ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 4d5719fe2c..5c40ea4397 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -808,6 +808,38 @@ test_expect_success 'format-patch with multiple notes refs' '\n \t! grep \"this is note 2\" out\n '\n \n+test_expect_success 'format-patch with multiple notes refs in config' '\n+\ttest_when_finished \"test_unconfig format.notes\" &&\n+\n+\tgit notes --ref note1 add -m \"this is note 1\" HEAD &&\n+\ttest_when_finished git notes --ref note1 remove HEAD &&\n+\tgit notes --ref note2 add -m \"this is note 2\" HEAD &&\n+\ttest_when_finished git notes --ref note2 remove HEAD &&\n+\n+\tgit config format.notes note1 &&\n+\tgit format-patch -1 --stdout >out &&\n+\tgrep \"this is note 1\" out &&\n+\t! grep \"this is note 2\" out &&\n+\tgit config format.notes note2 &&\n+\tgit format-patch -1 --stdout >out &&\n+\t! grep \"this is note 1\" out &&\n+\tgrep \"this is note 2\" out &&\n+\tgit config --add format.notes note1 &&\n+\tgit format-patch -1 --stdout >out &&\n+\tgrep \"this is note 1\" out &&\n+\tgrep \"this is note 2\" out &&\n+\n+\tgit config --replace-all format.notes note1 &&\n+\tgit config --add format.notes false &&\n+\tgit format-patch -1 --stdout >out &&\n+\t! grep \"this is note 1\" out &&\n+\t! grep \"this is note 2\" out &&\n+\tgit config --add format.notes note2 &&\n+\tgit format-patch -1 --stdout >out &&\n+\t! grep \"this is note 1\" out &&\n+\tgrep \"this is note 2\" out\n+'\n+\n echo \"fatal: --name-only does not make sense\" > expect.name-only\n echo \"fatal: --name-status does not make sense\" > expect.name-status\n echo \"fatal: --check does not make sense\" > expect.check\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"387770","messageId":"709565e734820fcb6d5db027dcf644f53ce2d4c9.1575896661.git.liu.denton@gmail.com","threadId":"52414","inReplyTo":"cover.1575896661.git.liu.denton@gmail.com","subject":"[PATCH 5/5] format-patch: move git_config() before repo_init_revisions()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-09T13:10:48Z","receivedAt":"2019-12-09T13:10:52Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In 13cdf78094 (format-patch: teach format.notes config option,\n2019-05-16), the order in which git_config() and repo_init_revisions()\nwere swapped so that `rev.notes_opt` would be initialized before\ngit_config() was called. This is problematic, however, as git_config()\nshould generally be called before repo_init_revisions().\n\nBreak this circular dependency by creating `show_notes` and `notes_opt`\nwhich git_config() reads into. Then, copy these values over to\n`rev.show_notes` and `rev.notes_opt` after repo_init_revisions() is\ncalled.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n builtin/log.c | 11 +++++++----\n 1 file changed, 7 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 1f0405f72b..4225615e7f 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -769,6 +769,8 @@ static const char *signature = git_version_string;\n static const char *signature_file;\n static int config_cover_letter;\n static const char *config_output_directory;\n+static int show_notes;\n+static struct display_notes_opt notes_opt;\n \n enum {\n \tCOVER_UNSET,\n@@ -779,8 +781,6 @@ enum {\n \n static int git_format_config(const char *var, const char *value, void *cb)\n {\n-\tstruct rev_info *rev = cb;\n-\n \tif (!strcmp(var, \"format.headers\")) {\n \t\tif (!value)\n \t\t\tdie(_(\"format.headers without value\"));\n@@ -868,7 +868,7 @@ 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\trev->show_notes = set_display_notes(&rev->notes_opt, b, b < 0 ? value : NULL);\n+\t\tshow_notes = set_display_notes(&notes_opt, b, b < 0 ? value : NULL);\n \t\treturn 0;\n \t}\n \n@@ -1624,8 +1624,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \textra_to.strdup_strings = 1;\n \textra_cc.strdup_strings = 1;\n \tinit_log_defaults();\n+\tinit_display_notes(&notes_opt);\n+\tgit_config(git_format_config, NULL);\n \trepo_init_revisions(the_repository, &rev, prefix);\n-\tgit_config(git_format_config, &rev);\n+\trev.show_notes = show_notes;\n+\tmemcpy(&rev.notes_opt, &notes_opt, sizeof(notes_opt));\n \trev.commit_format = CMIT_FMT_EMAIL;\n \trev.expand_tabs_in_log_default = 0;\n \trev.verbose_header = 1;\n-- \n2.24.0.627.geba02921db\n\n"},{"id":"387805","messageId":"CAPig+cToM+sHj-C5N_F2F+5B3LTPLVf39_-kCSih_WkuBOV+mA@mail.gmail.com","threadId":"52414","inReplyTo":"62543250c4ea0e0327f974cb90b294c60b525982.1575896661.git.liu.denton@gmail.com","subject":"Re: [PATCH 3/5] notes: extract logic into set_display_notes()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-09T16:26:15Z","receivedAt":"2019-12-09T16:26:31Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 9, 2019 at 8:11 AM Denton Liu <liu.denton@gmail.com> wrote:\n> Instead of open coding the logic that tweaks the variables in\n> `struct display_notes_opt` within handle_revision_opt(), abstract away the\n> logic into set_display_notes() so that it can be reused.\n>\n> Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> ---\n> diff --git a/notes.c b/notes.c\n> @@ -1045,6 +1045,30 @@ void init_display_notes(struct display_notes_opt *opt)\n> +int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref)\n> +{\n> +       if (show_notes) {\n> +               if (opt_ref) {\n> +                       struct strbuf buf = STRBUF_INIT;\n> +                       strbuf_addstr(&buf, opt_ref);\n> +                       expand_notes_ref(&buf);\n> +                       string_list_append(&opt->extra_notes_refs,\n> +                                          strbuf_detach(&buf, NULL));\n> +               } else {\n> +                       opt->use_default_notes = 1;\n> +               }\n> +       } else {\n> +               opt->use_default_notes = -1;\n> +               /* we have been strdup'ing ourselves, so trick\n> +                * string_list into free()ing strings */\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> +       }\n> +\n> +       return !!show_notes;\n> +}\n\nWhen you find yourself creating a function in which the entire body is\n(effectively) a single giant 'if' statement and in which the 'then'\nand 'else' arms are chosen by an input argument to that function (and\nremaining input arguments are used only by one or the other branch),\nit is usually a good sign that you should really be creating two\ndistinct functions. Doing so would reduce cognitive load of people\nreading and trying to understand the code (as well as reduce the\nindentation level). For instance, you might introduce these functions:\n\n    void enable_display_notes(struct display_notes_opt *opt, const\nchar *opt_ref);\n    void disable_display_notes(struct display_notes_opt *opt);\n\n> diff --git a/notes.h b/notes.h\n> @@ -265,6 +265,16 @@ struct display_notes_opt {\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> + */\n> +int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref);\n\nPlease drop the meaningless return value. While I understand you did\nthis as a convenience to make calling code a bit more concise, it\nnevertheless doesn't belong here since it conflates that convenience\ncode with the true purpose of this function (which to enable or\ndisable note display). Worse, it increases cognitive load on people\ntrying to comprehend the code since it requires that they consult the\ndocumentation for set_display_notes() to understand what is going on.\nThat is, this is far less obvious:\n\n    revs->show_notes = set_display_notes(&revs->notes_opt, 1, optarg);\n\nthan this:\n\n    revs->show_notes = 1;\n    enable_display_notes(&revs->notes_opt, optarg);\n\nAlternately, if revs->show_notes and revs->notes_opt really ought\nalways be set in lockstep, then maybe it makes more sense to have the\n\"enable\" and \"disable\" functions accept 'revs' directly in order to be\nable to adjust both revs->show_notes and revs_notes_opt together:\n\n    void enable_display_notes(struct rev_info *revs, const char *opt_ref);\n    void disable_display_notes(struct rev_info *revs);\n"},{"id":"387820","messageId":"20191209191924.GA24159@generichostname","threadId":"52414","inReplyTo":"CAPig+cToM+sHj-C5N_F2F+5B3LTPLVf39_-kCSih_WkuBOV+mA@mail.gmail.com","subject":"Re: [PATCH 3/5] notes: extract logic into set_display_notes()","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-09T19:19:24Z","receivedAt":"2019-12-09T19:19:01Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Eric,\n\nOn Mon, Dec 09, 2019 at 11:26:15AM -0500, Eric Sunshine wrote:\n> On Mon, Dec 9, 2019 at 8:11 AM Denton Liu <liu.denton@gmail.com> wrote:\n> > Instead of open coding the logic that tweaks the variables in\n> > `struct display_notes_opt` within handle_revision_opt(), abstract away the\n> > logic into set_display_notes() so that it can be reused.\n> >\n> > Signed-off-by: Denton Liu <liu.denton@gmail.com>\n> > ---\n> > diff --git a/notes.c b/notes.c\n> > @@ -1045,6 +1045,30 @@ void init_display_notes(struct display_notes_opt *opt)\n> > +int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref)\n> > +{\n> > +       if (show_notes) {\n> > +               if (opt_ref) {\n> > +                       struct strbuf buf = STRBUF_INIT;\n> > +                       strbuf_addstr(&buf, opt_ref);\n> > +                       expand_notes_ref(&buf);\n> > +                       string_list_append(&opt->extra_notes_refs,\n> > +                                          strbuf_detach(&buf, NULL));\n> > +               } else {\n> > +                       opt->use_default_notes = 1;\n> > +               }\n> > +       } else {\n> > +               opt->use_default_notes = -1;\n> > +               /* we have been strdup'ing ourselves, so trick\n> > +                * string_list into free()ing strings */\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> > +       }\n> > +\n> > +       return !!show_notes;\n> > +}\n> \n> When you find yourself creating a function in which the entire body is\n> (effectively) a single giant 'if' statement and in which the 'then'\n> and 'else' arms are chosen by an input argument to that function (and\n> remaining input arguments are used only by one or the other branch),\n> it is usually a good sign that you should really be creating two\n> distinct functions. Doing so would reduce cognitive load of people\n> reading and trying to understand the code (as well as reduce the\n> indentation level). For instance, you might introduce these functions:\n> \n>     void enable_display_notes(struct display_notes_opt *opt, const\n> char *opt_ref);\n>     void disable_display_notes(struct display_notes_opt *opt);\n\nMakes sense. I'll probably split it off into disable_display_notes(),\nenable_default_display_notes() and enable_ref_display_notes() since\nthere is no logic shared between when opt_ref is NULL and when it isn't.\n\n> \n> > diff --git a/notes.h b/notes.h\n> > @@ -265,6 +265,16 @@ struct display_notes_opt {\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> > + */\n> > +int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref);\n> \n> Please drop the meaningless return value. While I understand you did\n> this as a convenience to make calling code a bit more concise, it\n> nevertheless doesn't belong here since it conflates that convenience\n> code with the true purpose of this function (which to enable or\n> disable note display). Worse, it increases cognitive load on people\n> trying to comprehend the code since it requires that they consult the\n> documentation for set_display_notes() to understand what is going on.\n> That is, this is far less obvious:\n> \n>     revs->show_notes = set_display_notes(&revs->notes_opt, 1, optarg);\n> \n> than this:\n> \n>     revs->show_notes = 1;\n>     enable_display_notes(&revs->notes_opt, optarg);\n> \n> Alternately, if revs->show_notes and revs->notes_opt really ought\n> always be set in lockstep, then maybe it makes more sense to have the\n> \"enable\" and \"disable\" functions accept 'revs' directly in order to be\n> able to adjust both revs->show_notes and revs_notes_opt together:\n> \n>     void enable_display_notes(struct rev_info *revs, const char *opt_ref);\n>     void disable_display_notes(struct rev_info *revs);\n\nHmm, the \"lockstep\" thing was what I was going for. That's why I ended\nup using one monolithic function instead of several smaller ones. That\nway, the caller can just blindly pass in values and then use the values\nreturned to set its own state (i.e. how 4/5 does it). Also, it would\nensure that future developers using this function won't forget to set\nthe corresponding show_notes variable to whatever value is appropriate.\n\nThe reason why we can't accept `revs` is because this series attempts to\nstop depending on `revs` for the notes configuration entirely. That way,\nwe can call git_config() before repo_init_revisions() since we won't\nneed to have `revs` initialised.\n\nI considered accepting an `int *` instead of using the return value to\nmake the intent more explicit but I didn't do that because the return\nvalue seemed easier to deal with. Perhaps we could revive that idea\nwith:\n\n     void enable_display_notes(struct display_notes_opt *opt,\n\t     int *show_notes, const char *opt_ref);\n     void disable_display_notes(struct display_notes_opt *opt,\n\t     int *show_notes);\n\nI'll brood over the alternatives for a little bit before sending a\nreroll.\n\nThanks,\n\nDenton\n"},{"id":"387848","messageId":"xmqqmuc1tc4d.fsf@gitster-ct.c.googlers.com","threadId":"52414","inReplyTo":"cover.1575896661.git.liu.denton@gmail.com","subject":"Re: [PATCH 0/5] format-patch: improve handling of `format.notes`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-09T21:36:34Z","receivedAt":"2019-12-09T21:36:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> ... This series uses\n> the way `--notes=<ref1> --no-notes --notes=<ref2>` is handled as a model\n> and structures the handling of `format.notes` in a similar manner,\n> allowing one `format.notes = false` to override previous configs.\n\nMakes sense.\n\n> Also, in the same email, it was pointed out that git_config() should be\n> called before repo_init_revisions(). In 13cdf78094 (format-patch: teach\n> format.notes config option, 2019-05-16), the order was reversed. This\n> series changes it back such that git_config() is called before\n> repo_init_revisions().\n>\n> This series is based on top of 'dl/format-patch-notes-config'.\n>\n> It has minor textual conflicts with 'pu'. The merge resolution can be found at\n> https://github.com/Denton-L/git.git on branch\n> 'published/published/pu-format-patch-notes-config'.\n>\n> [1]: https://lore.kernel.org/git/CABPp-BF44+6gvZVNimKf-k7AWbOjw3OK-cJeFunNR96wvZGkcw@mail.gmail.com/\n>\n> Denton Liu (5):\n>   notes: rename to load_display_notes()\n>   notes: create init_display_notes() helper\n>   notes: extract logic into set_display_notes()\n>   format-patch: use --notes behavior for format.notes\n>   format-patch: move git_config() before repo_init_revisions()\n>\n>  builtin/log.c           | 26 +++++++++-----------------\n>  notes.c                 | 30 ++++++++++++++++++++++++++++++\n>  notes.h                 | 21 ++++++++++++++++++---\n>  revision.c              | 22 +++++-----------------\n>  t/t4014-format-patch.sh | 32 ++++++++++++++++++++++++++++++++\n>  5 files changed, 94 insertions(+), 37 deletions(-)\n"},{"id":"387849","messageId":"CABPp-BFstgQya9mKXdDVWkf=U_3RQqY=_+2MpZpw_u9ci+0mqA@mail.gmail.com","threadId":"52414","inReplyTo":"cover.1575896661.git.liu.denton@gmail.com","subject":"Re: [PATCH 0/5] format-patch: improve handling of `format.notes`","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2019-12-09T21:42:16Z","receivedAt":"2019-12-09T21:42:29Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Mon, Dec 9, 2019 at 5:10 AM Denton Liu <liu.denton@gmail.com> wrote:\n>\n> In this email[1], Elijah pointed out that the handling of multiple\n> `format.notes` configurations could be buggy. If we had\n> `format.notes = <ref1>`, `format.notes = false` and\n> `format.notes = <ref2>`, the behaviour is ambiguous. This series uses\n> the way `--notes=<ref1> --no-notes --notes=<ref2>` is handled as a model\n> and structures the handling of `format.notes` in a similar manner,\n> allowing one `format.notes = false` to override previous configs.\n>\n> Also, in the same email, it was pointed out that git_config() should be\n> called before repo_init_revisions(). In 13cdf78094 (format-patch: teach\n> format.notes config option, 2019-05-16), the order was reversed. This\n> series changes it back such that git_config() is called before\n> repo_init_revisions().\n\nSweet, thanks for working on this.  I took a very cursory glance over\nthe patches and didn't spot anything egregiously wrong, though I'm\nalso not familiar with the notes machinery.  Mostly I'm just piping up\nto thank you for tackling this so quickly.  :-)\n"},{"id":"387881","messageId":"95295e75-7d8d-bc6e-c41f-dd52c9826dfe@iee.email","threadId":"52414","inReplyTo":"20191209191924.GA24159@generichostname","subject":"Re: [PATCH 3/5] notes: extract logic into set_display_notes()","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.email","sentAt":"2019-12-10T11:22:38Z","receivedAt":"2019-12-10T11:22:48Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"HI Denton\n\nOn 09/12/2019 19:19, Denton Liu wrote:\n> the \"lockstep\" thing was what I was going for. That's why I ended\n> up using one monolithic function instead of several smaller ones. That\n> way, the caller can just blindly pass in values and then use the values\n> returned to set its own state (i.e. how 4/5 does it). Also, it would\n> ensure that future developers using this function won't forget to set\n> the corresponding show_notes variable to whatever value is appropriate.\n>\n> The reason why we can't accept `revs` is because this series attempts to\n> stop depending on `revs` for the notes configuration entirely. That way,\n> we can call git_config() before repo_init_revisions() since we won't\n> need to have `revs` initialised.\n>\n> I considered accepting an `int *` instead of using the return value to\n> make the intent more explicit but I didn't do that because the return\n> value seemed easier to deal with.\nDoes your explanation: \"this series attempts to stop depending on `revs` \nfor the notes configuration entirely.\" need adding adding to the commit \nmessage? I.e. the \"abstract away\" phrase probably doesn't carry enough \ninformation/context.p\n\nMaybe pick out some of the explanations for the commit message for \nfuture readers?\n\nPhilip\n"}]}