[PATCH] git-tag: Fix -l option to use better shell style globs.

[PATCH] git-tag: Fix -l option to use better shell style globs.

[Carlos Rica]

This patch removes certain behaviour of "git tag -l foo", currently
listing every tag name having "foo" as a substring.  The same
thing now could be achieved doing "git tag -l '*foo*'".

This feature was added recently when git-tag.sh got the -n option
for showing tag annotations, because that commit also replaced the
old "grep pattern" behaviour with a more preferable "shell pattern"
behaviour (although slightly modified as you can see).
Thus, the following builtin-tag.c implemented it in order to
ensure that tests were passing unchanged with both programs.

Since common "shell patterns" match names with a given substring
_only_ when * is inserted before and after (as in "*substring*"), and
the "plain" behaviour cannot be achieved easily with the current
implementation, this is mostly the right thing to do, in order to
make it more flexible and consistent.

Tests for "git tag" were also changed to reflect this.

Signed-off-by: Carlos Rica <jasampler@gmail.com>
---
 builtin-tag.c  |   11 ++---------
 t/t7004-tag.sh |   20 +++++++++-----------
 2 files changed, 11 insertions(+), 20 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index d6d38ad..348919c 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -123,22 +123,15 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 static int list_tags(const char *pattern, int lines)
 {
 	struct tag_filter filter;
-	char *newpattern;

 	if (pattern == NULL)
-		pattern = "";
+		pattern = "*";

-	/* prepend/append * to the shell pattern: */
-	newpattern = xmalloc(strlen(pattern) + 3);
-	sprintf(newpattern, "*%s*", pattern);
-
-	filter.pattern = newpattern;
+	filter.pattern = pattern;
 	filter.lines = lines;

 	for_each_tag_ref(show_reference, (void *) &filter);

-	free(newpattern);
-
 	return 0;
 }

diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index c4fa446..606d4f2 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -185,18 +185,17 @@ cba
 EOF
 test_expect_success \
 	'listing tags with substring as pattern must print those matching' '
-	git-tag -l a > actual &&
+	git-tag -l "*a*" > actual &&
 	git diff expect actual
 '

 cat >expect <<EOF
 v0.2.1
 v1.0.1
-v1.1.3
 EOF
 test_expect_success \
-	'listing tags with substring as pattern must print those matching' '
-	git-tag -l .1 > actual &&
+	'listing tags with a suffix as pattern must print those matching' '
+	git-tag -l "*.1" > actual &&
 	git diff expect actual
 '

@@ -205,37 +204,36 @@ t210
 t211
 EOF
 test_expect_success \
-	'listing tags with substring as pattern must print those matching' '
-	git-tag -l t21 > actual &&
+	'listing tags with a prefix as pattern must print those matching' '
+	git-tag -l "t21*" > actual &&
 	git diff expect actual
 '

 cat >expect <<EOF
 a1
-aa1
 EOF
 test_expect_success \
-	'listing tags using a name as pattern must print those matching' '
+	'listing tags using a name as pattern must print that one matching' '
 	git-tag -l a1 > actual &&
 	git diff expect actual
 '

 cat >expect <<EOF
 v1.0
-v1.0.1
 EOF
 test_expect_success \
-	'listing tags using a name as pattern must print those matching' '
+	'listing tags using a name as pattern must print that one matching' '
 	git-tag -l v1.0 > actual &&
 	git diff expect actual
 '

 cat >expect <<EOF
+v1.0.1
 v1.1.3
 EOF
 test_expect_success \
 	'listing tags with ? in the pattern should print those matching' '
-	git-tag -l "1.1?" > actual &&
+	git-tag -l "v1.?.?" > actual &&
 	git diff expect actual
 '

-- 
1.5.0

Re: [PATCH] git-tag: Fix -l option to use better shell style globs.

[Shawn O. Pearce]

Carlos Rica <jasampler@gmail.com> wrote:
> This patch removes certain behaviour of "git tag -l foo", currently
> listing every tag name having "foo" as a substring.  The same
> thing now could be achieved doing "git tag -l '*foo*'".

Even though this is a behavior change, I think its the right thing
to do.  The current behavior of searching "*$arg*" is downright
annoying and not what most users would expect I think, especially
when tools like for-each-ref don't do that.

Then again, I do "*$arg*" in git-gui's revision selection widget.
But there its immediately obvious what is happening and anyone I
have talked[*1*] to prefers it that way.


*1*: Disclaimer: people I talked to has thus far been limited to
     day-job coworkers.
 
-- 
Shawn.

Re: [PATCH] git-tag: Fix -l option to use better shell style globs.

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Carlos Rica <jasampler@gmail.com> wrote:
>> This patch removes certain behaviour of "git tag -l foo", currently
>> listing every tag name having "foo" as a substring.  The same
>> thing now could be achieved doing "git tag -l '*foo*'".
>
> Even though this is a behavior change, I think its the right thing
> to do.  The current behavior of searching "*$arg*" is downright
> annoying.

Yes, I concur.  It is very annoying that "git tag -l gui"
matches "gitgui-0.7.0".

Let's fix this.

Re: [PATCH] git-tag: Fix -l option to use better shell style globs.

[Junio C Hamano]

Carlos Rica <jasampler@gmail.com> writes:

>  	if (pattern == NULL)
> -		pattern = "";
> +		pattern = "*";
>
> -	/* prepend/append * to the shell pattern: */
> -	newpattern = xmalloc(strlen(pattern) + 3);
> -	sprintf(newpattern, "*%s*", pattern);
> -
> -	filter.pattern = newpattern;
> +	filter.pattern = pattern;
>  	filter.lines = lines;
>
>  	for_each_tag_ref(show_reference, (void *) &filter);

I think it is conceptually simpler on the show_reference side to
allow (filter.pattern == NULL) and say:

	if (!filter->pattern || !fnmatch(filter->pattern, refname, 0)) {
        	... show that ref ...
	}

It is not such a big deal now you do not do newpattern
allocation anymore, so I'll apply the patch as is.

Re: [PATCH] git-tag: Fix -l option to use better shell style globs.

[Carlos Rica]

[-- Attachment #1: Type: text/plain, Size: 581 bytes --]

2007/9/1, Junio C Hamano <gitster@pobox.com>:
> I think it is conceptually simpler on the show_reference side to
> allow (filter.pattern == NULL) and say:
>
>         if (!filter->pattern || !fnmatch(filter->pattern, refname, 0)) {
>                 ... show that ref ...
>         }
>
> It is not such a big deal now you do not do newpattern
> allocation anymore, so I'll apply the patch as is.
>

You are right. I changed the patch also to reflect this.
I cannot send a reply using my email client now, so I send it
attached to this response to avoid gmail breaks in long lines.

[-- Attachment #2: 0001-git-tag-Fix-l-option-to-use-better-shell-style-globs.patch --]
[-- Type: text/x-patch, Size: 3992 bytes --]

From d10170c5a2c3cf1fc6ad270e9a2c82ff29a98871 Mon Sep 17 00:00:00 2001
From: Carlos Rica <jasampler@gmail.com>
Date: Sat, 1 Sep 2007 06:58:40 +0200
Subject: [PATCH] git-tag: Fix -l option to use better shell style globs.

This patch removes the behaviour of "git tag -l foo", currently
listing every tag name having "foo" as a substring.  The same
thing now could be achieved doing "git tag -l '*foo*'".

The "feature" was added recently when git-tag.sh got the -n option
for showing tag annotations, because that commit also replaced the
old "grep pattern" behaviour with a more preferable "shell pattern"
behaviour (although slightly modified as you can see).
Thus, the following builtin-tag.c implemented it in order to
ensure that tests were passing unchanged with both programs.

Since common "shell patterns" match names with a given substring
_only_ when * is inserted before and after (as in "*substr*"), and
the "plain" behaviour cannot be achieved easily with the current
implementation, this is mostly the right thing to do, in order to
make it more flexible and consistent.

Tests for "git tag" were also changed to reflect this.

Signed-off-by: Carlos Rica <jasampler@gmail.com>
---
 builtin-tag.c  |   14 ++------------
 t/t7004-tag.sh |   20 +++++++++-----------
 2 files changed, 11 insertions(+), 23 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index d6d38ad..0e01b98 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -75,7 +75,7 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 {
 	struct tag_filter *filter = cb_data;
 
-	if (!fnmatch(filter->pattern, refname, 0)) {
+	if (!filter->pattern || !fnmatch(filter->pattern, refname, 0)) {
 		int i;
 		unsigned long size;
 		enum object_type type;
@@ -123,22 +123,12 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 static int list_tags(const char *pattern, int lines)
 {
 	struct tag_filter filter;
-	char *newpattern;
 
-	if (pattern == NULL)
-		pattern = "";
-
-	/* prepend/append * to the shell pattern: */
-	newpattern = xmalloc(strlen(pattern) + 3);
-	sprintf(newpattern, "*%s*", pattern);
-
-	filter.pattern = newpattern;
+	filter.pattern = pattern;
 	filter.lines = lines;
 
 	for_each_tag_ref(show_reference, (void *) &filter);
 
-	free(newpattern);
-
 	return 0;
 }
 
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index c4fa446..606d4f2 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -185,18 +185,17 @@ cba
 EOF
 test_expect_success \
 	'listing tags with substring as pattern must print those matching' '
-	git-tag -l a > actual &&
+	git-tag -l "*a*" > actual &&
 	git diff expect actual
 '
 
 cat >expect <<EOF
 v0.2.1
 v1.0.1
-v1.1.3
 EOF
 test_expect_success \
-	'listing tags with substring as pattern must print those matching' '
-	git-tag -l .1 > actual &&
+	'listing tags with a suffix as pattern must print those matching' '
+	git-tag -l "*.1" > actual &&
 	git diff expect actual
 '
 
@@ -205,37 +204,36 @@ t210
 t211
 EOF
 test_expect_success \
-	'listing tags with substring as pattern must print those matching' '
-	git-tag -l t21 > actual &&
+	'listing tags with a prefix as pattern must print those matching' '
+	git-tag -l "t21*" > actual &&
 	git diff expect actual
 '
 
 cat >expect <<EOF
 a1
-aa1
 EOF
 test_expect_success \
-	'listing tags using a name as pattern must print those matching' '
+	'listing tags using a name as pattern must print that one matching' '
 	git-tag -l a1 > actual &&
 	git diff expect actual
 '
 
 cat >expect <<EOF
 v1.0
-v1.0.1
 EOF
 test_expect_success \
-	'listing tags using a name as pattern must print those matching' '
+	'listing tags using a name as pattern must print that one matching' '
 	git-tag -l v1.0 > actual &&
 	git diff expect actual
 '
 
 cat >expect <<EOF
+v1.0.1
 v1.1.3
 EOF
 test_expect_success \
 	'listing tags with ? in the pattern should print those matching' '
-	git-tag -l "1.1?" > actual &&
+	git-tag -l "v1.?.?" > actual &&
 	git diff expect actual
 '
 
-- 
1.5.0

Re: [PATCH] git-tag: Fix -l option to use better shell style globs.

[Carlos Rica]

[-- Attachment #1: Type: text/plain, Size: 733 bytes --]

2007/9/1, Carlos Rica <jasampler@gmail.com>:
> You are right. I changed the patch also to reflect this.

Sorry, it seems that it also has a different "version" of
my comments for the commit, I have the good/bad habit of
reading again the comments in patches (and changing them)
before send it out to the list...

6c6
This patch removes certain behaviour of "git tag -l foo", currently
This patch removes the behaviour of "git tag -l foo", currently

10c10
This feature was added recently when git-tag.sh got the -n option
The "feature" was added recently when git-tag.sh got the -n option

18c18
_only_ when * is inserted before and after (as in "*substring*"), and
_only_ when * is inserted before and after (as in "*substr*"), and

[-- Attachment #2: 0001-git-tag-Fix-l-option-to-use-better-shell-style-globs.patch --]
[-- Type: text/x-patch, Size: 3998 bytes --]

From ea881fdfd39ce5fb5aaa273d90c6bf8ed5f8d9b0 Mon Sep 17 00:00:00 2001
From: Carlos Rica <jasampler@gmail.com>
Date: Sat, 1 Sep 2007 06:58:40 +0200
Subject: [PATCH] git-tag: Fix -l option to use better shell style globs.

This patch removes certain behaviour of "git tag -l foo", currently
listing every tag name having "foo" as a substring.  The same
thing now could be achieved doing "git tag -l '*foo*'".

This feature was added recently when git-tag.sh got the -n option
for showing tag annotations, because that commit also replaced the
old "grep pattern" behaviour with a more preferable "shell pattern"
behaviour (although slightly modified as you can see).
Thus, the following builtin-tag.c implemented it in order to
ensure that tests were passing unchanged with both programs.

Since common "shell patterns" match names with a given substring
_only_ when * is inserted before and after (as in "*substring*"), and
the "plain" behaviour cannot be achieved easily with the current
implementation, this is mostly the right thing to do, in order to
make it more flexible and consistent.

Tests for "git tag" were also changed to reflect this.

Signed-off-by: Carlos Rica <jasampler@gmail.com>
---
 builtin-tag.c  |   14 ++------------
 t/t7004-tag.sh |   20 +++++++++-----------
 2 files changed, 11 insertions(+), 23 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index d6d38ad..0e01b98 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -75,7 +75,7 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 {
 	struct tag_filter *filter = cb_data;
 
-	if (!fnmatch(filter->pattern, refname, 0)) {
+	if (!filter->pattern || !fnmatch(filter->pattern, refname, 0)) {
 		int i;
 		unsigned long size;
 		enum object_type type;
@@ -123,22 +123,12 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 static int list_tags(const char *pattern, int lines)
 {
 	struct tag_filter filter;
-	char *newpattern;
 
-	if (pattern == NULL)
-		pattern = "";
-
-	/* prepend/append * to the shell pattern: */
-	newpattern = xmalloc(strlen(pattern) + 3);
-	sprintf(newpattern, "*%s*", pattern);
-
-	filter.pattern = newpattern;
+	filter.pattern = pattern;
 	filter.lines = lines;
 
 	for_each_tag_ref(show_reference, (void *) &filter);
 
-	free(newpattern);
-
 	return 0;
 }
 
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index c4fa446..606d4f2 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -185,18 +185,17 @@ cba
 EOF
 test_expect_success \
 	'listing tags with substring as pattern must print those matching' '
-	git-tag -l a > actual &&
+	git-tag -l "*a*" > actual &&
 	git diff expect actual
 '
 
 cat >expect <<EOF
 v0.2.1
 v1.0.1
-v1.1.3
 EOF
 test_expect_success \
-	'listing tags with substring as pattern must print those matching' '
-	git-tag -l .1 > actual &&
+	'listing tags with a suffix as pattern must print those matching' '
+	git-tag -l "*.1" > actual &&
 	git diff expect actual
 '
 
@@ -205,37 +204,36 @@ t210
 t211
 EOF
 test_expect_success \
-	'listing tags with substring as pattern must print those matching' '
-	git-tag -l t21 > actual &&
+	'listing tags with a prefix as pattern must print those matching' '
+	git-tag -l "t21*" > actual &&
 	git diff expect actual
 '
 
 cat >expect <<EOF
 a1
-aa1
 EOF
 test_expect_success \
-	'listing tags using a name as pattern must print those matching' '
+	'listing tags using a name as pattern must print that one matching' '
 	git-tag -l a1 > actual &&
 	git diff expect actual
 '
 
 cat >expect <<EOF
 v1.0
-v1.0.1
 EOF
 test_expect_success \
-	'listing tags using a name as pattern must print those matching' '
+	'listing tags using a name as pattern must print that one matching' '
 	git-tag -l v1.0 > actual &&
 	git diff expect actual
 '
 
 cat >expect <<EOF
+v1.0.1
 v1.1.3
 EOF
 test_expect_success \
 	'listing tags with ? in the pattern should print those matching' '
-	git-tag -l "1.1?" > actual &&
+	git-tag -l "v1.?.?" > actual &&
 	git diff expect actual
 '
 
-- 
1.5.0