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

[Eric Sunshine <sunshine@sunshineco.com>]

On Thu, Sep 17, 2020 at 8:49 PM Aaron Lipman <alipman88@gmail.com> wrote:
> 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

> 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.