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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2012, 06:33 UTC
Message-ID: <7vhayjbcna.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1329874130-16818-4-git-send-email-tmgrennan@gmail.com>

Tom Grennan <tmgrennan@gmail.com> writes:

Show 17 quoted lines

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

Show 20 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.

Show 11 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.

Thanks.