Re: [PATCHv3 3/5] tag --exclude option
From: Junio C Hamano <hidden>
Date: 2016-06-15 22:53:07
Possibly related (same subject, not in this thread)
- 2016-06-15 · Re: [PATCHv3 3/5] tag --exclude option · Junio C Hamano <hidden>
Tom Grennan [off-list ref] writes:
Example, $ git tag -l --exclude "*-rc?" "v1.7.8*" v1.7.8 v1.7.8.1 v1.7.8.2 v1.7.8.3 v1.7.8.4 Which is equivalent to, $ git tag -l "v1.7.8*" | grep -v \\-rc. v1.7.8 v1.7.8.1 v1.7.8.2 v1.7.8.3 v1.7.8.4 Signed-off-by: Tom Grennan <redacted>
Having an example is a good way to illustrate your explanation, but it is not a substitution. Could we have at least one real sentence to describe what the added option *does*? This comment applies to all the patches in this series except for the second patch.
quoted hunk ↗ jump to hunk
diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt index 8d32b9a..470bd80 100644 --- a/Documentation/git-tag.txt +++ b/Documentation/git-tag.txt@@ -13,7 +13,7 @@ SYNOPSIS <tagname> [<commit> | <object>] 'git tag' -d <tagname>... 'git tag' [-n[<num>]] -l [--contains <commit>] [--points-at <object>] - [<pattern>...] + [--exclude <pattern>] [<pattern>...] 'git tag' -v <tagname>... DESCRIPTION@@ -90,6 +90,10 @@ OPTIONS --points-at <object>:: Only list tags of the given object. +--exclude <pattern>:: + Don't list tags matching the given pattern. This has precedence + over any other match pattern arguments.
As you do not specify what kind of pattern matching is done to this exclude pattern, it is important to use the same logic between positive and negative ones to give users a consistent UI. Unfortunately we use fnmatch without FNM_PATHNAME for positive ones, so this exclude pattern needs to follow the same semantics to reduce confusion. This comment applies to all the patches in this series to add this option to existing commands that take the positive pattern.
quoted hunk ↗ jump to hunk
@@ -202,6 +206,15 @@ test_expect_success \ ' cat >expect <<EOF +v0.2.1 +EOF +test_expect_success \ + 'listing tags with a suffix as pattern and prefix exclusion' ' + git tag -l --exclude "v1.*" "*.1" > actual && + test_cmp expect actual +'
I know you are imitating the style of surrounding tests that is an older
parts of this script, but it is an eyesore. More modern tests are written
like this:
test_expect_success 'label for the test' '
cat >expect <<-EOF &&
v0.2.1
EOF
git tag -l ... >actual &&
test_cmp expect actual
'
to avoid unnecessary backslash on the first line, and have the preparation
of test vectore _inside_ test_expect_success. We would eventually want to
update the older part to the newer style for consistency.
Two possible ways to go about this are (1) have a "pure style" patch at
the beginning to update older tests to a new style and then add new code
and new test as a follow-up patch written in modern, or (2) add new code
and new test in modern, and make a mental note to update the older ones
after the dust settles. Adding new tests written in older style to a file
that already has mixed styles is the worst thing you can do.
This comment applies to all the patches in this series with tests.
Thanks.