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

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