Re: [PATCHv3 3/5] tag --exclude option
- From
Tom Grennan <tmgrennan@gmail.com>
- Date
- Feb 23, 2012, 00:22 UTC
- Message-ID
- <20120223002215.GE2410@tgrennan-laptop>
- In-Reply-To
- <7vhayjbcna.fsf@alter.siamese.dyndns.org>
On Tue, Feb 21, 2012 at 10:33:29PM -0800, Junio C Hamano wrote:
Show 27 quoted lines
>Tom Grennan <tmgrennan@gmail.com> 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@gmail.com> > >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. >
OK, I'll add the "exclude" option description from the respective man pages.
Show 29 quoted lines
>> 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.
OK, should I also describe the --no-exclude option?
Show 36 quoted lines
>> @@ -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.
I'd prefer, (1) precede each "--exclude" patch with a "pure style" patch to update the respective tests. However, since this will result in a lot of conflict with concurrent development; I'll separate the test patches from the code and documentation. I'll then cycle on rebasing the style and new test patches until the development of each is quiescent.
Thanks, TomG