Re: [PATCH 2/2] notes: break set_display_notes() into smaller functions

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>,
	Philip Oakley <philipoakley@iee.email>
Subject: Re: [PATCH 2/2] notes: break set_display_notes() into smaller functions
Date: Wed, 18 Dec 2019 11:18:10 -0500	[thread overview]
Message-ID: <CAPig+cRVyfWceVZFgRuRyL-B3TMnX9fObC84t2LHsNoXm8TuqQ@mail.gmail.com> (raw)
In-Reply-To: <96b908c91c19bc11da9f8d3b0d6e853517284540.1576111643.git.liu.denton@gmail.com>

On Wed, Dec 11, 2019 at 7:48 PM Denton Liu <liu.denton@gmail.com> wrote:
> 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.
> [...]
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/notes.c b/notes.c
> @@ -1045,28 +1045,31 @@ void init_display_notes(struct display_notes_opt *opt)
> +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 */

    /*
     * Multi-line comment
     * style.
     */

Though, apparently the formatting used here predates your changes; you
simply moved the code (and comment) around.

> +       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;
>  }
> diff --git a/notes.h b/notes.h
> @@ -266,14 +266,19 @@ struct display_notes_opt {
>  /*
> + * 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

s/enable_default_display_notes/enable_ref_display_notes/

> + * '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

s/points/pointer/

> + * displayed, else 0. It must not be NULL.
>   */