Re: [PATCHv3 3/5] tag --exclude option

[Tom Grennan <tmgrennan@gmail.com>]

On Tue, Feb 21, 2012 at 10:33:29PM -0800, Junio C Hamano wrote:
>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.

>> 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?

>> @@ -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