[3/5] notes: extract logic into set_display_notes()
diff mbox series

Message ID 62543250c4ea0e0327f974cb90b294c60b525982.1575896661.git.liu.denton@gmail.com
State New
Headers show
Series
  • format-patch: improve handling of `format.notes`
Related show

Commit Message

Denton Liu Dec. 9, 2019, 1:10 p.m. UTC
Instead of open coding the logic that tweaks the variables in
`struct display_notes_opt` within handle_revision_opt(), abstract away the
logic into set_display_notes() so that it can be reused.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 notes.c    | 24 ++++++++++++++++++++++++
 notes.h    | 10 ++++++++++
 revision.c | 20 ++++----------------
 3 files changed, 38 insertions(+), 16 deletions(-)

Comments

Eric Sunshine Dec. 9, 2019, 4:26 p.m. UTC | #1
On Mon, Dec 9, 2019 at 8:11 AM Denton Liu <liu.denton@gmail.com> wrote:
> Instead of open coding the logic that tweaks the variables in
> `struct display_notes_opt` within handle_revision_opt(), abstract away the
> logic into set_display_notes() so that it can be reused.
>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/notes.c b/notes.c
> @@ -1045,6 +1045,30 @@ void init_display_notes(struct display_notes_opt *opt)
> +int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref)
> +{
> +       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;
> +       }
> +
> +       return !!show_notes;
> +}

When you find yourself creating a function in which the entire body is
(effectively) a single giant 'if' statement and in which the 'then'
and 'else' arms are chosen by an input argument to that function (and
remaining input arguments are used only by one or the other branch),
it is usually a good sign that you should really be creating two
distinct functions. Doing so would reduce cognitive load of people
reading and trying to understand the code (as well as reduce the
indentation level). For instance, you might introduce these functions:

    void enable_display_notes(struct display_notes_opt *opt, const
char *opt_ref);
    void disable_display_notes(struct display_notes_opt *opt);

> diff --git a/notes.h b/notes.h
> @@ -265,6 +265,16 @@ struct display_notes_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.
> + */
> +int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref);

Please drop the meaningless return value. While I understand you did
this as a convenience to make calling code a bit more concise, it
nevertheless doesn't belong here since it conflates that convenience
code with the true purpose of this function (which to enable or
disable note display). Worse, it increases cognitive load on people
trying to comprehend the code since it requires that they consult the
documentation for set_display_notes() to understand what is going on.
That is, this is far less obvious:

    revs->show_notes = set_display_notes(&revs->notes_opt, 1, optarg);

than this:

    revs->show_notes = 1;
    enable_display_notes(&revs->notes_opt, optarg);

Alternately, if revs->show_notes and revs->notes_opt really ought
always be set in lockstep, then maybe it makes more sense to have the
"enable" and "disable" functions accept 'revs' directly in order to be
able to adjust both revs->show_notes and revs_notes_opt together:

    void enable_display_notes(struct rev_info *revs, const char *opt_ref);
    void disable_display_notes(struct rev_info *revs);
Denton Liu Dec. 9, 2019, 7:19 p.m. UTC | #2
Hi Eric,

On Mon, Dec 09, 2019 at 11:26:15AM -0500, Eric Sunshine wrote:
> On Mon, Dec 9, 2019 at 8:11 AM Denton Liu <liu.denton@gmail.com> wrote:
> > Instead of open coding the logic that tweaks the variables in
> > `struct display_notes_opt` within handle_revision_opt(), abstract away the
> > logic into set_display_notes() so that it can be reused.
> >
> > Signed-off-by: Denton Liu <liu.denton@gmail.com>
> > ---
> > diff --git a/notes.c b/notes.c
> > @@ -1045,6 +1045,30 @@ void init_display_notes(struct display_notes_opt *opt)
> > +int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref)
> > +{
> > +       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;
> > +       }
> > +
> > +       return !!show_notes;
> > +}
> 
> When you find yourself creating a function in which the entire body is
> (effectively) a single giant 'if' statement and in which the 'then'
> and 'else' arms are chosen by an input argument to that function (and
> remaining input arguments are used only by one or the other branch),
> it is usually a good sign that you should really be creating two
> distinct functions. Doing so would reduce cognitive load of people
> reading and trying to understand the code (as well as reduce the
> indentation level). For instance, you might introduce these functions:
> 
>     void enable_display_notes(struct display_notes_opt *opt, const
> char *opt_ref);
>     void disable_display_notes(struct display_notes_opt *opt);

Makes sense. I'll probably split it off into disable_display_notes(),
enable_default_display_notes() and enable_ref_display_notes() since
there is no logic shared between when opt_ref is NULL and when it isn't.

> 
> > diff --git a/notes.h b/notes.h
> > @@ -265,6 +265,16 @@ struct display_notes_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.
> > + */
> > +int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref);
> 
> Please drop the meaningless return value. While I understand you did
> this as a convenience to make calling code a bit more concise, it
> nevertheless doesn't belong here since it conflates that convenience
> code with the true purpose of this function (which to enable or
> disable note display). Worse, it increases cognitive load on people
> trying to comprehend the code since it requires that they consult the
> documentation for set_display_notes() to understand what is going on.
> That is, this is far less obvious:
> 
>     revs->show_notes = set_display_notes(&revs->notes_opt, 1, optarg);
> 
> than this:
> 
>     revs->show_notes = 1;
>     enable_display_notes(&revs->notes_opt, optarg);
> 
> Alternately, if revs->show_notes and revs->notes_opt really ought
> always be set in lockstep, then maybe it makes more sense to have the
> "enable" and "disable" functions accept 'revs' directly in order to be
> able to adjust both revs->show_notes and revs_notes_opt together:
> 
>     void enable_display_notes(struct rev_info *revs, const char *opt_ref);
>     void disable_display_notes(struct rev_info *revs);

Hmm, the "lockstep" thing was what I was going for. That's why I ended
up using one monolithic function instead of several smaller ones. That
way, the caller can just blindly pass in values and then use the values
returned to set its own state (i.e. how 4/5 does it). Also, it would
ensure that future developers using this function won't forget to set
the corresponding show_notes variable to whatever value is appropriate.

The reason why we can't accept `revs` is because this series attempts to
stop depending on `revs` for the notes configuration entirely. That way,
we can call git_config() before repo_init_revisions() since we won't
need to have `revs` initialised.

I considered accepting an `int *` instead of using the return value to
make the intent more explicit but I didn't do that because the return
value seemed easier to deal with. Perhaps we could revive that idea
with:

     void enable_display_notes(struct display_notes_opt *opt,
	     int *show_notes, const char *opt_ref);
     void disable_display_notes(struct display_notes_opt *opt,
	     int *show_notes);

I'll brood over the alternatives for a little bit before sending a
reroll.

Thanks,

Denton
Philip Oakley Dec. 10, 2019, 11:22 a.m. UTC | #3
HI Denton

On 09/12/2019 19:19, Denton Liu wrote:
> the "lockstep" thing was what I was going for. That's why I ended
> up using one monolithic function instead of several smaller ones. That
> way, the caller can just blindly pass in values and then use the values
> returned to set its own state (i.e. how 4/5 does it). Also, it would
> ensure that future developers using this function won't forget to set
> the corresponding show_notes variable to whatever value is appropriate.
>
> The reason why we can't accept `revs` is because this series attempts to
> stop depending on `revs` for the notes configuration entirely. That way,
> we can call git_config() before repo_init_revisions() since we won't
> need to have `revs` initialised.
>
> I considered accepting an `int *` instead of using the return value to
> make the intent more explicit but I didn't do that because the return
> value seemed easier to deal with.
Does your explanation: "this series attempts to stop depending on `revs` 
for the notes configuration entirely." need adding adding to the commit 
message? I.e. the "abstract away" phrase probably doesn't carry enough 
information/context.p

Maybe pick out some of the explanations for the commit message for 
future readers?

Philip

Patch
diff mbox series

diff --git a/notes.c b/notes.c
index 53d1e7767c..c93feff4ab 100644
--- a/notes.c
+++ b/notes.c
@@ -1045,6 +1045,30 @@  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)
+{
+	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;
+	}
+
+	return !!show_notes;
+}
+
 void load_display_notes(struct display_notes_opt *opt)
 {
 	char *display_ref_env;
diff --git a/notes.h b/notes.h
index c0b712371c..a476bfa066 100644
--- a/notes.h
+++ b/notes.h
@@ -265,6 +265,16 @@  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.
+ */
+int set_display_notes(struct display_notes_opt *opt, int show_notes, const char *opt_ref);
+
 /*
  * Load the notes machinery for displaying several notes trees.
  *
diff --git a/revision.c b/revision.c
index 24ad974590..c2d8d24939 100644
--- a/revision.c
+++ b/revision.c
@@ -2172,9 +2172,8 @@  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 = 1;
+		revs->show_notes = set_display_notes(&revs->notes_opt, 1, NULL);
 		revs->show_notes_given = 1;
-		revs->notes_opt.use_default_notes = 1;
 	} else if (!strcmp(arg, "--show-signature")) {
 		revs->show_signature = 1;
 	} else if (!strcmp(arg, "--no-show-signature")) {
@@ -2189,25 +2188,14 @@  static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->track_first_time = 1;
 	} else if (skip_prefix(arg, "--show-notes=", &optarg) ||
 		   skip_prefix(arg, "--notes=", &optarg)) {
-		struct strbuf buf = STRBUF_INIT;
-		revs->show_notes = 1;
-		revs->show_notes_given = 1;
 		if (starts_with(arg, "--show-notes=") &&
 		    revs->notes_opt.use_default_notes < 0)
 			revs->notes_opt.use_default_notes = 1;
-		strbuf_addstr(&buf, optarg);
-		expand_notes_ref(&buf);
-		string_list_append(&revs->notes_opt.extra_notes_refs,
-				   strbuf_detach(&buf, NULL));
+		revs->show_notes = set_display_notes(&revs->notes_opt, 1, optarg);
+		revs->show_notes_given = 1;
 	} else if (!strcmp(arg, "--no-notes")) {
-		revs->show_notes = 0;
+		revs->show_notes = set_display_notes(&revs->notes_opt, 0, NULL);
 		revs->show_notes_given = 1;
-		revs->notes_opt.use_default_notes = -1;
-		/* we have been strdup'ing ourselves, so trick
-		 * string_list into free()ing strings */
-		revs->notes_opt.extra_notes_refs.strdup_strings = 1;
-		string_list_clear(&revs->notes_opt.extra_notes_refs, 0);
-		revs->notes_opt.extra_notes_refs.strdup_strings = 0;
 	} else if (!strcmp(arg, "--standard-notes")) {
 		revs->show_notes_given = 1;
 		revs->notes_opt.use_default_notes = 1;