[PATCH v4 0/3] git branch: allow combining merged and no-merged filters

From: Aaron Lipman <alipman88@gmail.com>
Date: Sep 16, 2020, 02:08 UTC
Message-ID: <20200916020840.84892-1-alipman88@gmail.com>
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.

Show 11 quoted lines

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