git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Aaron Lipman <alipman88@gmail.com>
To: git@vger.kernel.org
Cc: Aaron Lipman <alipman88@gmail.com>,
	Eric Sunshine <sunshine@sunshineco.com>,
	Junio C Hamano <gitster@pobox.com>
Subject: [PATCH v4 0/3] git branch: allow combining merged and no-merged filters
Date: Tue, 15 Sep 2020 22:08:37 -0400
Message-ID: <20200916020840.84892-1-alipman88@gmail.com> (raw)
In-Reply-To: <20200913193140.66906-1-alipman88@gmail.com>

Thanks for the review, Eric & Junio.

Eric -

> I didn't examine it too closely, so this may be a silly question, but
> is there a reason to start from scratch (by deleting all the branches)
> rather than simply using or extending the existing branches like the
> other tests do?

I went back and forth on this. There were a couple reasons I leaned
towards starting fresh - I found branch names like feature_a &
feature_b more illustrative, and I didn't want readers to have to
scroll up further to find where branches came from.

But, with the tests re-ordered so "branch --merged with --verbose"
comes last (which adds new branches that might otherwise clutter up
the output of my new tests), I'm happy with using the existing test
setup - rewritten accordingly.

> It's a bit concerning to see output from porcelain git-branch being
> fed to 'grep' and 'xargs'. More typically, you would instead rely upon
> the (stable) output of a plumbing command...

Thanks, useful knowledge for future contributions.

> We normally avoid repeating in the commit message what the patch
> itself already says. The first paragraph alone (without the example
> text) would be plenty sufficient. (Not itself worth a re-roll,
> though.)

Got it, removed.

> Missing sign-off.

Whoops, fixed.

> This is repeated nearly verbatim in the other two documentation pages.
> It makes one wonder if it can be generalized and placed in its own
> file which is included in the other documents via
> `include::contains.txt[]`. Perhaps like this:
> 
>     When combining multiple `--contains` and `--no-contains` filters,
>     `git branch` shows references containing at least one of the named
>     `--contains` commits and none of the named `--no-contains`
>     commits.
> 
> But perhaps that's too generic?

Cool, I hadn't realized we could embed snippets like that. Slightly
generic, but I have no strong opinion either way. Going with the
passive wording provided by Junio.

(Looking at AsciiDoc's documentation, I think we could also set a
:command-name: variable to insert some dynamic content into an
include:: file.)

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

Removed.

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

Agreed, dropped.

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

I don't see a reason to. The --merged and --no-merged filters are
applied in separate passes, so I feel it's sufficient to test them
independently. (When doing my own QA testing, I did combine multiple
merged & multiple no-merged, multiple contains & multiple no-contains,
merged/no-merged & contains/no-contains, etc.)

On the other hand, extra test cases could help prevent regressions
should someone significantly refactor ref-filter.c. If anyone has a
preference to add more tests, I'm happy to oblige.

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

Thanks, fixed.

Junio -

> I do not mind making the 0/1 a symbolic constant between
> do_merge_filter() and filter_refs() for enhanced readability,
> though.

If I understand the convention, the constant names should be prefixed
with DO_MERGE_FILTER. I suggest DO_MERGE_FILTER_REACHABLE and
DO_MERGE_FILTER_UNREACHABLE. Happy to re-roll if others have a
different preference - or feel free to edit.)

Aaron Lipman (3):
  t3201: test multiple branch filter combinations
  Doc: cover multiple contains/no-contains filters
  ref-filter: allow merged and no-merged filters

 Documentation/filters.txt          |  7 +++
 Documentation/git-branch.txt       | 10 ++--
 Documentation/git-for-each-ref.txt | 13 ++++--
 Documentation/git-tag.txt          | 11 +++--
 builtin/branch.c                   |  6 +--
 builtin/for-each-ref.c             |  2 +-
 builtin/tag.c                      |  8 ++--
 ref-filter.c                       | 64 ++++++++++++++------------
 ref-filter.h                       | 12 ++---
 t/t3200-branch.sh                  |  4 --
 t/t3201-branch-contains.sh         | 74 +++++++++++++++++++++++++-----
 t/t6302-for-each-ref-filter.sh     |  4 +-
 t/t7004-tag.sh                     |  4 +-
 13 files changed, 143 insertions(+), 76 deletions(-)
 create mode 100644 Documentation/filters.txt

-- 
2.24.3 (Apple Git-128)


  parent reply	other threads:[~2020-09-16  2:09 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-08 15:37 [PATCH] ref-filter: allow " 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       ` Aaron Lipman [this message]
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=20200916020840.84892-1-alipman88@gmail.com \
    --to=alipman88@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=sunshine@sunshineco.com \
    /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