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 v3 3/3] ref-filter: allow merged and no-merged filters
Date: Sun, 13 Sep 2020 17:36:39 -0400
Message-ID: <CAPig+cTNp84Dm=0n-Bb9o=1nZNDFRE20XDWUPJT8yjdefv15rA@mail.gmail.com> (raw)
In-Reply-To: <20200913193140.66906-4-alipman88@gmail.com>

On Sun, Sep 13, 2020 at 3:32 PM Aaron Lipman <alipman88@gmail.com> wrote:
> Enable ref-filter to process multiple merged and no-merged filters, and
> extend functionality to git branch, git tag and git for-each-ref. This
> provides an easy way to check for branches that are "graduation
> candidates:"
>
> $ git branch --no-merged master --merged next

A few subjective comments below, not necessarily worth a re-roll...

> To accomplish this, store the merged and no-merged filters in two
> commit_list structs (reachable_from and unreachable_from) rather than
> using a single commit struct (merge_commit).

This sort of implementation detail is readily discoverable by reading
the patch itself, and since there is no complexity about it which
requires extensive explanation, we'd normally leave it out of the
commit message.

> If passed more than one merged (or more than one no-merged) filter, refs
> must be reachable from any one of the merged commits, and reachable from
> none of the no-merged commits.
>
> Signed-off-by: Aaron Lipman <alipman88@gmail.com>
> ---
> diff --git a/ref-filter.c b/ref-filter.c
> @@ -2231,13 +2231,20 @@ void ref_array_clear(struct ref_array *array)
> -static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata)
> +static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable)
>  {
>         [...]
> +       struct commit_list *check_reachable_list = reachable ?
> +               ref_cbdata->filter->reachable_from :
> +               ref_cbdata->filter->unreachable_from;
> +
> +       if (!check_reachable_list)
> +               return;

Rather than adding a boolean 'reachable' parameter to the function
signature, you could instead directly pass in the `struct commit_list
*` upon which to operate, which would allow you to drop the ternary
operator here, and...

> @@ -2322,8 +2337,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int
> -       if (filter->merge_commit)
> -               do_merge_filter(&ref_cbdata);
> +       do_merge_filter(&ref_cbdata, 1);
> +       do_merge_filter(&ref_cbdata, 0);

... make the callers a bit less opaque by eliminating the
less-than-meaningful 0-or-1, and making it obvious which list is being
consulted:

    do_merge_filter(&ref_cbdata, ref_cbdata->filter->reachable_from);
    do_merge_filter(&ref_cbdata, ref_cbdata->filter->unreachable_from);

> @@ -2541,31 +2556,22 @@ int parse_opt_merge_filter(const struct option *opt, const char *arg, int unset)
> +       if (starts_with(opt->long_name, "no"))
> +               commit_list_insert(merge_commit, &rf->unreachable_from);
> +       else
> +               commit_list_insert(merge_commit, &rf->reachable_from);

... quite like it's obvious here into which list the item is being inserted.

> diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
> @@ -1299,8 +1299,8 @@ test_expect_success '--merged catches invalid object names' '
> -test_expect_success '--merged is incompatible with --no-merged' '
> -       test_must_fail git branch --merged HEAD --no-merged HEAD
> +test_expect_success '--merged is compatible with --no-merged' '
> +       git branch --merged master --no-merged master
>  '

This revised test doesn't seem to have all that much value since this
combination is checked by new tests added elsewhere by the patch.
Therefore, another option would be simply to drop the test rather than
revising it. (But, again, it's a judgment call.)

> diff --git a/t/t3201-branch-contains.sh b/t/t3201-branch-contains.sh
> @@ -226,6 +226,16 @@ test_expect_success 'multiple branch --contains' '
> +test_expect_success 'multiple branch --merged' '
> +test_expect_success 'multiple branch --no-merged' '
> +test_expect_success 'branch --merged combined with --no-merged' '

Would it make sense to also test multiple --merged and multiple
--no-merged? (Genuine question, not a demand to add more tests.)

> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> @@ -2016,7 +2016,7 @@ test_expect_success '--merged can be used in non-list mode' '
>  test_expect_success '--merged is incompatible with --no-merged' '
> -       test_must_fail git tag --merged HEAD --no-merged HEAD
> +       git tag --merged HEAD --no-merged HEAD
>  '

I think you forgot s/incompatible/compatible/ in the test title (which
you changed in the other cases).

  reply	other threads:[~2020-09-13 21:36 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-08 15:37 [PATCH] " 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 [this message]
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
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+cTNp84Dm=0n-Bb9o=1nZNDFRE20XDWUPJT8yjdefv15rA@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