git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: Aaron Lipman <alipman88@gmail.com>
Cc: Git List <git@vger.kernel.org>
Subject: Re: [PATCH 1/2] ref-filter: refactor do_merge_filter()
Date: Fri, 18 Sep 2020 01:03:00 -0400
Message-ID: <CAPig+cRXJ5hCEM0dFsSSnGWaYu76gPhpM3fV4FaV+Db4r6dX4g@mail.gmail.com> (raw)
In-Reply-To: <20200918004909.32474-2-alipman88@gmail.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.

  reply	other threads:[~2020-09-18  5:03 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-08 15:37 [PATCH] ref-filter: allow merged and no-merged filters Aaron Lipman
2020-09-08 22:46 ` Junio C Hamano
2020-09-08 23:57   ` Aaron Lipman
2020-09-09  0:00     ` Junio C Hamano
2020-09-11 18:57 ` [PATCH v2 0/2] git branch: allow combining " Aaron Lipman
2020-09-11 18:57   ` [PATCH v2 1/2] t3201: test multiple branch filter combinations Aaron Lipman
2020-09-11 18:57   ` [PATCH v2 2/2] ref-filter: allow merged and no-merged filters Aaron Lipman
2020-09-11 21:47     ` Junio C Hamano
2020-09-13 19:31     ` [PATCH v3 0/3] git branch: allow combining " Aaron Lipman
2020-09-13 19:31       ` [PATCH v3 1/3] t3201: test multiple branch filter combinations Aaron Lipman
2020-09-13 20:58         ` Eric Sunshine
2020-09-14 20:07           ` Junio C Hamano
2020-09-13 19:31       ` [PATCH v3 2/3] Doc: cover multiple contains/no-contains filters Aaron Lipman
2020-09-13 21:10         ` Eric Sunshine
2020-09-14 20:07           ` Junio C Hamano
2020-09-13 19:31       ` [PATCH v3 3/3] ref-filter: allow merged and no-merged filters Aaron Lipman
2020-09-13 21:36         ` Eric Sunshine
2020-09-13 21:56           ` Junio C Hamano
2020-09-13 22:31             ` Eric Sunshine
2020-09-16  2:08       ` [PATCH v4 0/3] git branch: allow combining " Aaron Lipman
2020-09-16  2:08         ` [PATCH v4 1/3] t3201: test multiple branch filter combinations Aaron Lipman
2020-09-16  2:08         ` [PATCH v4 2/3] Doc: cover multiple contains/no-contains filters Aaron Lipman
2020-09-16 19:45           ` Junio C Hamano
2020-09-16  2:08         ` [PATCH v4 3/3] ref-filter: allow merged and no-merged filters Aaron Lipman
2020-09-16  4:53         ` [PATCH v4 0/3] git branch: allow combining " Junio C Hamano
2020-09-16  5:08           ` Eric Sunshine
2020-09-18  0:49         ` [PATCH 0/2] ref-filter: merged & no-merged touch-ups Aaron Lipman
2020-09-18  0:49           ` [PATCH 1/2] ref-filter: refactor do_merge_filter() Aaron Lipman
2020-09-18  5:03             ` Eric Sunshine [this message]
2020-09-18  5:04               ` Eric Sunshine
2020-09-18 17:01               ` Junio C Hamano
2020-09-18  0:49           ` [PATCH 2/2] Doc: prefer more specific file name Aaron Lipman
2020-09-18  2:52           ` [PATCH 0/2] ref-filter: merged & no-merged touch-ups Eric Sunshine
2020-09-18 21:58           ` [PATCH v2 " Aaron Lipman
2020-09-18 21:58             ` [PATCH v2 1/2] ref-filter: make internal reachable-filter API more precise Aaron Lipman
2020-09-18 21:58             ` [PATCH v2 2/2] Doc: prefer more specific file name Aaron Lipman

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAPig+cRXJ5hCEM0dFsSSnGWaYu76gPhpM3fV4FaV+Db4r6dX4g@mail.gmail.com \
    --to=sunshine@sunshineco.com \
    --cc=alipman88@gmail.com \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for the project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git