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

Message

Aaron Lipman Sept. 16, 2020, 2:08 a.m. UTC

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

Comments

Junio C Hamano Sept. 16, 2020, 4:53 a.m. UTC | #1

Aaron Lipman <[e-mail]> writes:

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

Even though I said "I do not mind", using DO_MERGE_FILTER prefix is
way too much noise.  The common prefix is appropriate for external
API, but this is merely an internal contract between a private
helper do_merge_filter() and its only caller.

If I were redesigning the code around this area, I probably would
rename do_merge_filter() to something more descriptive, and tweak
its parameters a bit more focused, e.g.

    #define EXCLUDE_REACHED 0
    #define INCLUDE_REACHED 1
    static void reach_filter(struct ref_array *array, 
			     struct commit_list *check_reachable,
			     int include_reached) {
		...
		for (i = 0; i < old_nr; i++) {
			struct commit *c = array->items[i];
			int is_reached = !!(c->object.flags & UNINTERESTING);

			if (is_reached == include_reached)
				array->items[array->nr++] = array->items[i];
			else
				free_array_item(array->items[i]);
		}
		...
    }

    /* the caller */
    ...
    reach_filter(array, filter->reachable_from, INCLUDE_REACHED);
    reach_filter(array, filter->unreachable_from, EXCLUDE_REACHED);

to make it clear which part of the callback struct is really passed
between the caller and the helper.  Even if we are not renaming
things that much, a locally defined preprocessor macro with shorter
names, defined just before the callee, would be more appropriate for
a case like this, with a single callee called by only one caller.

Thanks.

Eric Sunshine Sept. 16, 2020, 5:08 a.m. UTC | #2

On Wed, Sep 16, 2020 at 12:53 AM Junio C Hamano <[e-mail]> wrote:
> Even though I said "I do not mind", using DO_MERGE_FILTER prefix is
> way too much noise.  The common prefix is appropriate for external
> API, but this is merely an internal contract between a private
> helper do_merge_filter() and its only caller.
>
>     #define EXCLUDE_REACHED 0
>     #define INCLUDE_REACHED 1
>     static void reach_filter(struct ref_array *array,
>                              struct commit_list *check_reachable,
>                              int include_reached) {
>
> to make it clear which part of the callback struct is really passed
> between the caller and the helper.  Even if we are not renaming
> things that much, a locally defined preprocessor macro with shorter
> names, defined just before the callee, would be more appropriate for
> a case like this, with a single callee called by only one caller.

Thanks for stating what I was planning on saying, with particular
emphasis on keeping these #defines in the .c file rather than the .h
file since they are not part of the public API.

Aside from that, this re-roll seems to address all of my previous
review comments. Thanks.