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)
