Re: [PATCH 3/5] notes: extract logic into set_display_notes()

From: Eric Sunshine <sunshine@sunshineco.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: Git Mailing List <git@vger.kernel.org>,
	Elijah Newren <newren@gmail.com>,
	Junio C Hamano <gitster@pobox.com>,
	Beat Bolli <dev+git@drbeat.li>, Pavel Roskin <plroskin@gmail.com>
Subject: Re: [PATCH 3/5] notes: extract logic into set_display_notes()
Date: Mon, 9 Dec 2019 11:26:15 -0500	[thread overview]
Message-ID: <CAPig+cToM+sHj-C5N_F2F+5B3LTPLVf39_-kCSih_WkuBOV+mA@mail.gmail.com> (raw)
In-Reply-To: <62543250c4ea0e0327f974cb90b294c60b525982.1575896661.git.liu.denton@gmail.com>

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);