[RFC/PATCH] tag: make list exclude !<pattern>

Please see the following patch which filters the tag list of "!" prefaced
patterns.  If this is deemed desirable and correct, I'll resubmit with updated
documentation and unit tests.

Thanks,
Tom Grennan (1):
  tag: make list exclude !<pattern>

 builtin/tag.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

-- 
1.7.8

Use the "!" prefix to ignore tags of the given pattern.
This has precedence over other matching patterns.
For example,

  $ git tag -l \!*-rc? v1.7.8*
  v1.7.8
  v1.7.8.1
  v1.7.8.2
  v1.7.8.3
  v1.7.8.4
  $ git tag -l v1.7.8* \!*-rc?
  v1.7.8
  v1.7.8.1
  v1.7.8.2
  v1.7.8.3
  v1.7.8.4

This 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 <redacted>
---
 builtin/tag.c |   13 +++++++++----
 1 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 31f02e8..b9ef718 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -32,13 +32,18 @@ struct tag_filter {
 
 static int match_pattern(const char **patterns, const char *ref)
 {
+	int ret;
+
 	/* no pattern means match everything */
 	if (!*patterns)
 		return 1;
-	for (; *patterns; patterns++)
-		if (!fnmatch(*patterns, ref, 0))
-			return 1;
-	return 0;
+	for (ret = 0; *patterns; patterns++)
+		if (**patterns == '!') {
+		    if (!fnmatch(*patterns+1, ref, 0))
+			    return 0;
+		} else if (!fnmatch(*patterns, ref, 0))
+			ret = 1;
+	return ret;
 }
 
 static int in_commit_list(const struct commit_list *want, struct commit *c)

-- 
1.7.8

On Thu, Feb 09, 2012 at 11:43:36AM -0800, Tom Grennan wrote:

Use the "!" prefix to ignore tags of the given pattern.
This has precedence over other matching patterns.
For example,

...

static int match_pattern(const char **patterns, const char *ref)
{
+	int ret;
+
	/* no pattern means match everything */
	if (!*patterns)
		return 1;
-	for (; *patterns; patterns++)
-		if (!fnmatch(*patterns, ref, 0))
-			return 1;
-	return 0;
+	for (ret = 0; *patterns; patterns++)
+		if (**patterns == '!') {
+		    if (!fnmatch(*patterns+1, ref, 0))
+			    return 0;
+		} else if (!fnmatch(*patterns, ref, 0))
+			ret = 1;
+	return ret;
}

Correction, match_pattern() needs to be as follows to support all these cases,
  $ git tag -l
  $ git tag -l \!*-rc?
  $ git tag -l \!*-rc? v1.7.8*
  $ git tag -l v1.7.8* \!*-rc?
  $ git tag -l v1.7.8*

-- 
TomG

diff --git a/builtin/tag.c b/builtin/tag.c
index 31f02e8..e99be5c 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -32,13 +32,16 @@ struct tag_filter {
 
 static int match_pattern(const char **patterns, const char *ref)
 {
-	/* no pattern means match everything */
-	if (!*patterns)
-		return 1;
+	int had_match_pattern = 0, had_match = 0;
+
 	for (; *patterns; patterns++)
-		if (!fnmatch(*patterns, ref, 0))
-			return 1;
-	return 0;
+		if (**patterns != '!') {
+			had_match_pattern = 1;
+			if (!fnmatch(*patterns, ref, 0))
+				had_match = 1;
+		} else if (!fnmatch(*patterns+1, ref, 0))
+			return 0;
+	return had_match_pattern ? had_match : 1;
 }
 
 static int in_commit_list(const struct commit_list *want, struct commit *c)

On Fri, Feb 10, 2012 at 2:43 AM, Tom Grennan [off-list ref] wrote:

Please see the following patch which filters the tag list of "!" prefaced
patterns.  If this is deemed desirable and correct, I'll resubmit with updated
documentation and unit tests.

git-branch, git-tag and git-for-each-ref are in the same family. I
think it's good to that all three commands share things, like this
pattern matching.

About the '!' for exclusion, maybe it's better to move from fnmatch()
as matching machinery to pathspec. Then when git learns negative
pathspec [1], we have this feature for free.

[1] http://thread.gmane.org/gmane.comp.version-control.git/189645/focus=190072
-- 
Duy

On Fri, Feb 10, 2012 at 01:34:26PM +0700, Nguyen Thai Ngoc Duy wrote:

On Fri, Feb 10, 2012 at 2:43 AM, Tom Grennan [off-list ref] wrote:

quoted

Please see the following patch which filters the tag list of "!" prefaced
patterns.  If this is deemed desirable and correct, I'll resubmit with updated
documentation and unit tests.

git-branch, git-tag and git-for-each-ref are in the same family. I
think it's good to that all three commands share things, like this
pattern matching.

Yes, git-branch and git-tag could now use a common match_patterns() but
git-for-each-ref needs some rearranging; as will: git-describe,
git-replace, git-ls-remote, git-name-rev, and git-show-branch.

If we pursue this, it may be best to first add match_patterns() to ./refs.[ch]
then incrementally modify these builtin commands to use it.
  

About the '!' for exclusion, maybe it's better to move from fnmatch()
as matching machinery to pathspec. Then when git learns negative
pathspec [1], we have this feature for free.

[1] http://thread.gmane.org/gmane.comp.version-control.git/189645/focus=190072

I have to study this more. I'm not sure that --exclude has precedence
over matches. It also looks like this would require a lot more change to
the above.

Thanks,
TomG

On Fri, 10 Feb 2012 10:55:16 -0800, Tom Grennan wrote:

On Fri, Feb 10, 2012 at 01:34:26PM +0700, Nguyen Thai Ngoc Duy wrote:

quoted

On Fri, Feb 10, 2012 at 2:43 AM, Tom Grennan [off-list ref] wrote:

quoted

Please see the following patch which filters the tag list of "!" prefaced
patterns.  If this is deemed desirable and correct, I'll resubmit with updated
documentation and unit tests.

git-branch, git-tag and git-for-each-ref are in the same family. I
think it's good to that all three commands share things, like this
pattern matching.

Yes, git-branch and git-tag could now use a common match_patterns() but
git-for-each-ref needs some rearranging; as will: git-describe,
git-replace, git-ls-remote, git-name-rev, and git-show-branch.

If we pursue this, it may be best to first add match_patterns() to ./refs.[ch]
then incrementally modify these builtin commands to use it.

The following series implements !<pattern> with: git-tag, git-branch, and
git-for-each-ref.

This still requires Documentation and unit test updates but I think these are
close to functionally complete.

quoted

About the '!' for exclusion, maybe it's better to move from fnmatch()
as matching machinery to pathspec. Then when git learns negative
pathspec [1], we have this feature for free.

[1] http://thread.gmane.org/gmane.comp.version-control.git/189645/focus=190072

After looking at this some more, I don't understand the value of replacing
libc:fnmatch().  Or are you just referring to '--exclude' instead of
[!]<pattern> argument parsing?

---

Tom Grennan (4):
  refs: add common refname_match_patterns()
  tag: use refs.c:refname_match_patterns()
  branch: use refs.c:refname_match_patterns()
  for-each-ref: use refs.c:refname_match_patterns()

 Documentation/git-tag.txt |   10 ++++++----
 builtin/branch.c          |   16 ++--------------
 builtin/for-each-ref.c    |   23 +++--------------------
 builtin/tag.c             |   15 ++-------------
 refs.c                    |   14 ++++++++++++++
 refs.h                    |    8 ++++++++
 6 files changed, 35 insertions(+), 51 deletions(-)

-- 
1.7.8

Signed-off-by: Tom Grennan <redacted>
---
 builtin/for-each-ref.c |   23 +++--------------------
 1 files changed, 3 insertions(+), 20 deletions(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index b01d76a..2c9cc47 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c

@@ -781,25 +781,8 @@ static int grab_single_ref(const char *refname, const unsigned char *sha1, int f
 	struct refinfo *ref;
 	int cnt;
 
-	if (*cb->grab_pattern) {
-		const char **pattern;
-		int namelen = strlen(refname);
-		for (pattern = cb->grab_pattern; *pattern; pattern++) {
-			const char *p = *pattern;
-			int plen = strlen(p);
-
-			if ((plen <= namelen) &&
-			    !strncmp(refname, p, plen) &&
-			    (refname[plen] == '\0' ||
-			     refname[plen] == '/' ||
-			     p[plen-1] == '/'))
-				break;
-			if (!fnmatch(p, refname, FNM_PATHNAME))
-				break;
-		}
-		if (!*pattern)
-			return 0;
-	}
+	if (!refname_match_patterns(cb->grab_pattern, refname))
+		return 0;
 
 	/*
 	 * We do not open the object yet; sort may only need refname

@@ -974,7 +957,7 @@ static int opt_parse_sort(const struct option *opt, const char *arg, int unset)
 }
 
 static char const * const for_each_ref_usage[] = {
-	"git for-each-ref [options] [<pattern>]",
+	"git for-each-ref [options] [[!]<pattern>...]",
 	NULL
 };
 

-- 
1.7.8

Signed-off-by: Tom Grennan <redacted>
---
 builtin/branch.c |   16 ++--------------
 1 files changed, 2 insertions(+), 14 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 7095718..7dfc693 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c

@@ -266,18 +266,6 @@ struct append_ref_cb {
 	int ret;
 };
 
-static int match_patterns(const char **pattern, const char *refname)
-{
-	if (!*pattern)
-		return 1; /* no pattern always matches */
-	while (*pattern) {
-		if (!fnmatch(*pattern, refname, 0))
-			return 1;
-		pattern++;
-	}
-	return 0;
-}
-
 static int append_ref(const char *refname, const unsigned char *sha1, int flags, void *cb_data)
 {
 	struct append_ref_cb *cb = (struct append_ref_cb *)(cb_data);

@@ -312,7 +300,7 @@ static int append_ref(const char *refname, const unsigned char *sha1, int flags,
 	if ((kind & ref_list->kinds) == 0)
 		return 0;
 
-	if (!match_patterns(cb->pattern, refname))
+	if (!refname_match_patterns(cb->pattern, refname))
 		return 0;
 
 	commit = NULL;

@@ -542,7 +530,7 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
 	qsort(ref_list.list, ref_list.index, sizeof(struct ref_item), ref_cmp);
 
 	detached = (detached && (kinds & REF_LOCAL_BRANCH));
-	if (detached && match_patterns(pattern, "HEAD"))
+	if (detached && refname_match_patterns(pattern, "HEAD"))
 		show_detached(&ref_list);
 
 	for (i = 0; i < ref_list.index; i++) {

-- 
1.7.8

This will exclude tags matching patterns prefaced with the '!'
character.  This has precedence over other matching patterns.
For example,

  $ git tag -l \!*-rc? v1.7.8*
  v1.7.8
  v1.7.8.1
  v1.7.8.2
  v1.7.8.3
  v1.7.8.4
  $ git tag -l v1.7.8* \!*-rc?
  v1.7.8
  v1.7.8.1
  v1.7.8.2
  v1.7.8.3
  v1.7.8.4

This is equivalent to,

  $ git tag -l v1.7.8* | grep -v '\-rc.'

Without a matching pattern, filter all tags with the "!" patterns,
  $ ./git-tag -l \!*-rc?
  gitgui-0.10.0
  gitgui-0.10.1
  gitgui-0.10.2
  ...
  v1.7.8.3
  v1.7.8.4
  v1.7.9

That is equivalent to,

  $ git tag -l | grep -v '\-rc.'

Signed-off-by: Tom Grennan <redacted>
---
 Documentation/git-tag.txt |   10 ++++++----
 builtin/tag.c             |   15 ++-------------
 2 files changed, 8 insertions(+), 17 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 53ff5f6..56ea2fa 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt

@@ -12,7 +12,7 @@ SYNOPSIS
 'git tag' [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]
 	<tagname> [<commit> | <object>]
 'git tag' -d <tagname>...
-'git tag' [-n[<num>]] -l [--contains <commit>] [<pattern>...]
+'git tag' [-n[<num>]] -l [--contains <commit>] [[!]<pattern>...]
 'git tag' -v <tagname>...
 
 DESCRIPTION

@@ -75,13 +75,15 @@ OPTIONS
 	If no number is given to `-n`, only the first line is printed.
 	If the tag is not annotated, the commit message is displayed instead.
 
--l <pattern>::
---list <pattern>::
+-l [!]<pattern>::
+--list [!]<pattern>::
 	List tags with names that match the given pattern (or all if no
 	pattern is given).  Running "git tag" without arguments also
 	lists all tags. The pattern is a shell wildcard (i.e., matched
 	using fnmatch(3)).  Multiple patterns may be given; if any of
-	them matches, the tag is shown.
+	them matches, the tag is shown.  If the pattern is prefaced with
+	the '!' character, all tags matching the pattern are filtered
+	from the list.
 
 --contains <commit>::
 	Only list tags which contain the specified commit.

diff --git a/builtin/tag.c b/builtin/tag.c
index 31f02e8..7f99424 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c

@@ -19,7 +19,7 @@
 static const char * const git_tag_usage[] = {
 	"git tag [-a|-s|-u <key-id>] [-f] [-m <msg>|-F <file>] <tagname> [<head>]",
 	"git tag -d <tagname>...",
-	"git tag -l [-n[<num>]] [<pattern>...]",
+	"git tag -l [-n[<num>]] [[!]<pattern>...]",
 	"git tag -v <tagname>...",
 	NULL
 };

@@ -30,17 +30,6 @@ struct tag_filter {
 	struct commit_list *with_commit;
 };
 
-static int match_pattern(const char **patterns, const char *ref)
-{
-	/* no pattern means match everything */
-	if (!*patterns)
-		return 1;
-	for (; *patterns; patterns++)
-		if (!fnmatch(*patterns, ref, 0))
-			return 1;
-	return 0;
-}
-
 static int in_commit_list(const struct commit_list *want, struct commit *c)
 {
 	for (; want; want = want->next)

@@ -88,7 +77,7 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 {
 	struct tag_filter *filter = cb_data;
 
-	if (match_pattern(filter->patterns, refname)) {
+	if (refname_match_patterns(filter->patterns, refname)) {
 		int i;
 		unsigned long size;
 		enum object_type type;

-- 
1.7.8

Signed-off-by: Tom Grennan <redacted>
---
 refs.c |   14 ++++++++++++++
 refs.h |    8 ++++++++
 2 files changed, 22 insertions(+), 0 deletions(-)

diff --git a/refs.c b/refs.c
index b8843bb..b42eb4a 100644
--- a/refs.c
+++ b/refs.c

@@ -1057,6 +1057,20 @@ int refname_match(const char *abbrev_name, const char *full_name, const char **r
 	return 0;
 }
 
+int refname_match_patterns(const char **patterns, const char *refname)
+{
+	int given_match_pattern = 0, had_match = 0;
+
+	for (; *patterns; patterns++)
+		if (**patterns != '!') {
+			given_match_pattern = 1;
+			if (!fnmatch(*patterns, refname, 0))
+				had_match = 1;
+		} else if (!fnmatch(*patterns+1, refname, 0))
+			return 0;
+	return given_match_pattern ? had_match : 1;
+}
+
 static struct ref_lock *verify_lock(struct ref_lock *lock,
 	const unsigned char *old_sha1, int mustexist)
 {

diff --git a/refs.h b/refs.h
index 00ba1e2..13015ba 100644
--- a/refs.h
+++ b/refs.h

@@ -152,4 +152,12 @@ int update_ref(const char *action, const char *refname,
 		const unsigned char *sha1, const unsigned char *oldval,
 		int flags, enum action_on_err onerr);
 
+/**
+ * Returns:
+ *   1 with NULL patterns
+ *   0 if refname fnmatch()es any ! prefaced pattern
+ *   1 if refname fnmatch()es any pattern
+ */
+extern int refname_match_patterns(const char **patterns, const char *refname);
+
 #endif /* REFS_H */

-- 
1.7.8

On 02/11/2012 03:16 AM, Tom Grennan wrote:

This comment is unclear and incomplete.

1. What does "NULL patterns" mean?  Your code fails if patterns==NULL,
so I guess you mean "1 if there are no patterns in the list".

2. Since the three conditions are not mutually exclusive, you should say
how they are connected.  I believe that you want something like "A
otherwise B otherwise C".

3. You haven't specified what happens if refname matches neither a
!-prefixed pattern nor a non-!-prefixed pattern.  Does this behavior
depend on which types of patterns were present in the list?

I see that you have described the behavior more completely in the commit
message for patch 2/4, but the commit message is not enough: this
behavior should be described precisely in both code comments (when the
function is defined) and in the user documentation (when the
functionality is added to a command).

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

On Sat, Feb 11, 2012 at 08:12:54AM +0100, Michael Haggerty wrote:

On 02/11/2012 03:16 AM, Tom Grennan wrote:

quoted

diff --git a/refs.h b/refs.h
index 00ba1e2..13015ba 100644
--- a/refs.h
+++ b/refs.h

@@ -152,4 +152,12 @@ int update_ref(const char *action, const char *refname,
 		const unsigned char *sha1, const unsigned char *oldval,
 		int flags, enum action_on_err onerr);
 
+/**
+ * Returns:
+ *   1 with NULL patterns
+ *   0 if refname fnmatch()es any ! prefaced pattern
+ *   1 if refname fnmatch()es any pattern
+ */
+extern int refname_match_patterns(const char **patterns, const char *refname);
+
 #endif /* REFS_H */

This comment is unclear and incomplete.

1. What does "NULL patterns" mean?  Your code fails if patterns==NULL,
so I guess you mean "1 if there are no patterns in the list".

2. Since the three conditions are not mutually exclusive, you should say
how they are connected.  I believe that you want something like "A
otherwise B otherwise C".

3. You haven't specified what happens if refname matches neither a
!-prefixed pattern nor a non-!-prefixed pattern.  Does this behavior
depend on which types of patterns were present in the list?

I see that you have described the behavior more completely in the commit
message for patch 2/4, but the commit message is not enough: this
behavior should be described precisely in both code comments (when the
function is defined) and in the user documentation (when the
functionality is added to a command).

Yes, I didn't explicitly state that the precedence is the order written
and in correctly described the first case. How about?

/**
 * Returns in highest to lowest precedence:
 *   1 with an empty patterns list
 *   0 if refname fnmatch()es any ^ prefaced pattern
 *   1 if refname fnmatch()es any other pattern
 *   0 otherwise
 */

Thanks,
TomG

On 02/11/2012 08:17 PM, Tom Grennan wrote:

Yes, I didn't explicitly state that the precedence is the order written
and in correctly described the first case. How about?

/**
 * Returns in highest to lowest precedence:
 *   1 with an empty patterns list
 *   0 if refname fnmatch()es any ^ prefaced pattern
 *   1 if refname fnmatch()es any other pattern
 *   0 otherwise
 */

Much better; thanks.

Please note that this choice of semantics limits its power.  For
example, if the rule were instead (like with gitattributes(5)) "if more
than one pattern matches a refname, a later pattern overrides an earlier
pattern", then one could do things like

    refs/remotes/*/* !refs/remotes/gitster/* refs/remotes/gitster/master

to include specific references within a hierarchy that is otherwise
excluded.

However, since rev-list apparently uses a rule more like the one that
you are proposing, it might be better to be consistent than to choose a
different convention.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

On Mon, Feb 13, 2012 at 06:00:40AM +0100, Michael Haggerty wrote:

On 02/11/2012 08:17 PM, Tom Grennan wrote:

quoted

Yes, I didn't explicitly state that the precedence is the order written
and in correctly described the first case. How about?

/**
 * Returns in highest to lowest precedence:
 *   1 with an empty patterns list
 *   0 if refname fnmatch()es any ^ prefaced pattern
 *   1 if refname fnmatch()es any other pattern
 *   0 otherwise
 */

Much better; thanks.

Please note that this choice of semantics limits its power.  For
example, if the rule were instead (like with gitattributes(5)) "if more
than one pattern matches a refname, a later pattern overrides an earlier
pattern", then one could do things like

   refs/remotes/*/* !refs/remotes/gitster/* refs/remotes/gitster/master

to include specific references within a hierarchy that is otherwise
excluded.

However, since rev-list apparently uses a rule more like the one that
you are proposing, it might be better to be consistent than to choose a
different convention.

Hmm, I think it's important to have same respective result in each of
these case's,

  $ git tag -l | grep v1.7.8.*
  $ git tag -l v1.7.8*

  $ git tag -l | grep -v .*-rc*
  $ git tag -l ^*-rc*

  $ git tag -l v1.7.8* | grep -v .*-rc*
  $ git tag -l v1.7.8* ^*-rc*
  $ git tag -l ^*-rc* v1.7.8*

What I propose is somewhat analogous to gitignore's double negative,
      *	An optional prefix !  which negates the pattern; any matching
	file excluded by a previous pattern will become included again. If
	a negated pattern matches, this will override lower precedence
	patterns sources.

I still prefer "^" to "!" b/c A) it doesn't cause the noted regressions;
and B) doesn't need command quoting.  I'd accept the counter proposals
of --exclude or --with[out][-TYPE] but frankly, that's more
code/documentation churn ("less code is always better"[TM]) and worse,
more crap to type on the command line:

  $ git --with-tags v1.7.8* --without-tags '*-rc*' tag -l v1.7.8* 
or
  $ git tag -l --exclude '*-rc*' v1.7.8*
vs.
  $ git tag -l v1.7.8* ^*-rc*

-- 
TomG