Re: [PATCH 1/2] ref-filter: refactor do_merge_filter()

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 18, 2020, 05:03 UTC
Message-ID: <CAPig+cRXJ5hCEM0dFsSSnGWaYu76gPhpM3fV4FaV+Db4r6dX4g@mail.gmail.com>
In-Reply-To: <20200918004909.32474-2-alipman88@gmail.com>

On Thu, Sep 17, 2020 at 8:49 PM Aaron Lipman <alipman88@gmail.com> wrote:

Show 9 quoted lines

> ref-filter: refactor do_merge_filter()
>
> Rename to reach_filter() to be more descriptive.
>
> Separate parameters to explicitly identify what data is used by the
> function, instead of passing an entire ref_filter_cbdata struct.
>
> Rename and move associated preprocessor macros from header to source
> file, as they're only used internally in a single location.

One thing that this commit message lacks is a high-level explanation of why these changes are being made. One possible rewrite would be:

    ref-filter: make internal reachable-filter API more precise

    The internal reachable-filter API is a bit loose and imprecise; it
    also bleeds unnecessarily into the public header. Tighten the API
    by:

    * renaming do_merge_filter() to reach_filter()

    * separating parameters to explicitly identify what data is used
      by the function instead of passing an entire ref_filter_cbdata
      struct

    * renaming and moving internal constants from header to source
      file

Show 11 quoted lines

> Signed-off-by: Aaron Lipman <alipman88@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -2231,19 +2231,18 @@ void ref_array_clear(struct ref_array *array)
> +static void reach_filter(struct ref_array *array,
> +                        struct commit_list *check_reachable,
> +                        int include_reached)
>  {
>         [...]
> +       if (!check_reachable)
>                 return;

I would probably drop this conditional return altogether since it incorrectly conveys to the reader that it is legal to call this function with NULL for 'check_reachable', whereas, in reality, it would be pointless to do so. If this function were public, and the possible list of callers open-ended, then it might be reasonable for it to do this to alert callers early of their error:

    if (!check_reachable)
        BUG("check_reachable must not be NULL");

However, as this function is private, and the set of callers can be precisely determined, it's not necessary to make the check at all. If anyone adds a new call in this file which incorrectly passes NULL, they will discover the error quickly enough when the program crashes at the new call site.

If you were to drop this conditional, then that change would deserve another bullet point in the commit message.