Tom Grennan <tmgrennan@xxxxxxxxx> 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 <tmgrennan@xxxxxxxxx>
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.
> 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.
> @@ -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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html