notes: break set_display_notes() into smaller functions

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/builtin/log.c b/builtin/log.c
index 4225615e7fcca60a9b49a7f445d1992f87c0220b..b6d43a4a47ebbc39e36c5552b3bd0fdc5ae44404 100644 (file)
--- 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(&notes_opt, b, b < 0 ? value : NULL);
+               if (b < 0)
+                       enable_ref_display_notes(&notes_opt, &show_notes, value);
+               else if (b)
+                       enable_default_display_notes(&notes_opt, &show_notes);
+               else
+                       disable_display_notes(&notes_opt, &show_notes);
                return 0;
        }
 

diff --git a/notes.c b/notes.c
index c93feff4abd306bc9fdc59aa775e3a0c924cfd2a..3133bb181faaba089b8cb68b1b3eb18447b23db3 100644 (file)
--- 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 a476bfa06659a9d6c8bc12538f150cde06898b86..3e784481815c4d67740329ecb3ee91aafda24a9c 100644 (file)
--- 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 c2d8d24939dd7eb6b892accbd1680ec3cedf5ffb..1b12ed742bfe8862e2eb9e582490df3f5a208891 100644 (file)
--- 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 4134dc6029c40f39659b39927ba239aed845673b..72520780f4522681862385fc033d8ec878cd59f3 100644 (file)
--- 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,