From: Aaron Lipman <firstname.lastname@example.org> To: email@example.com Cc: Aaron Lipman <firstname.lastname@example.org>, Eric Sunshine <email@example.com>, Junio C Hamano <firstname.lastname@example.org> Subject: [PATCH v4 0/3] git branch: allow combining merged and no-merged filters Date: Tue, 15 Sep 2020 22:08:37 -0400 [thread overview] Message-ID: <email@example.com> (raw) In-Reply-To: <firstname.lastname@example.org> 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)
next prev 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 \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --subject='Re: [PATCH v4 0/3] git branch: allow combining merged and no-merged filters' \ /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
Code repositories for project(s) associated with this inbox: https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).