[RFC PATCH 1/1] git-tag: Add --regex option

[RFC PATCH 1/1] git-tag: Add --regex option

[Jake Goulding]

Allows the tag pattern to be expressed as a regular expression.

Signed-off-by: Jake Goulding <goulding@vivisimo.com>
---
 Documentation/git-tag.txt |    5 ++++-
 builtin-tag.c             |   34 ++++++++++++++++++++++++++++++----
 t/t7004-tag.sh            |   26 ++++++++++++++++++++++++++
 3 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 533d18b..5c08a45 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>]
 	<name> [<commit> | <object>]
 'git tag' -d <name>...
-'git tag' [-n[<num>]] -l [--contains <commit>] [<pattern>]
+'git tag' [-n[<num>]] -l [--contains <commit>] [--regex] [<pattern>]
 'git tag' -v <name>...
 
 DESCRIPTION
@@ -71,6 +71,9 @@ OPTIONS
 --contains <commit>::
 	Only list tags which contain the specified commit.
 
+--regex::
+	Treat the pattern as a regular expression, instead of a shell wildcard pattern.
+
 -m <msg>::
 	Use the given tag message (instead of prompting).
 	If multiple `-m` options are given, their values are
diff --git a/builtin-tag.c b/builtin-tag.c
index 01e7374..dad4e53 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -27,6 +27,8 @@ struct tag_filter {
 	const char *pattern;
 	int lines;
 	struct commit_list *with_commit;
+	int use_regex;
+	regex_t tag_regex;
 };
 
 #define PGP_SIGNATURE "-----BEGIN PGP SIGNATURE-----"
@@ -35,8 +37,14 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 			  int flag, void *cb_data)
 {
 	struct tag_filter *filter = cb_data;
+	int matched = 0;
 
-	if (!fnmatch(filter->pattern, refname, 0)) {
+	if (filter->use_regex)
+		matched = !regexec(&filter->tag_regex, refname, 0, NULL, 0);
+	else
+		matched = !fnmatch(filter->pattern, refname, 0);
+
+	if (matched) {
 		int i;
 		unsigned long size;
 		enum object_type type;
@@ -91,7 +99,7 @@ static int show_reference(const char *refname, const unsigned char *sha1,
 }
 
 static int list_tags(const char *pattern, int lines,
-			struct commit_list *with_commit)
+			struct commit_list *with_commit, int use_regex)
 {
 	struct tag_filter filter;
 
@@ -101,9 +109,24 @@ static int list_tags(const char *pattern, int lines,
 	filter.pattern = pattern;
 	filter.lines = lines;
 	filter.with_commit = with_commit;
+	filter.use_regex = use_regex;
+	if (use_regex) {
+		int regex_errcode;
+
+		regex_errcode = regcomp(&filter.tag_regex, pattern, 0);
+		if (regex_errcode) {
+			char regex_error[1024];
+
+			regerror(regex_errcode, &filter.tag_regex, regex_error, 1024);
+			return error("Invalid tag pattern \"%s\": %s\n", pattern, regex_error);
+		}
+	}
 
 	for_each_tag_ref(show_reference, (void *) &filter);
 
+	if (use_regex)
+		regfree(&filter.tag_regex);
+
 	return 0;
 }
 
@@ -370,7 +393,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	struct ref_lock *lock;
 
 	int annotate = 0, sign = 0, force = 0, lines = -1,
-		list = 0, delete = 0, verify = 0;
+		list = 0, delete = 0, verify = 0, use_regex = 0;
 	const char *msgfile = NULL, *keyid = NULL;
 	struct msg_arg msg = { 0, STRBUF_INIT };
 	struct commit_list *with_commit = NULL;
@@ -400,6 +423,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 			PARSE_OPT_LASTARG_DEFAULT,
 			parse_opt_with_commit, (intptr_t)"HEAD",
 		},
+		OPT_BOOLEAN(0, "regex", &use_regex, "use regex matching"),
 		OPT_END()
 	};
 
@@ -425,11 +449,13 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 		usage_with_options(git_tag_usage, options);
 	if (list)
 		return list_tags(argv[0], lines == -1 ? 0 : lines,
-				 with_commit);
+				 with_commit, use_regex);
 	if (lines != -1)
 		die("-n option is only allowed with -l.");
 	if (with_commit)
 		die("--contains option is only allowed with -l.");
+	if (use_regex)
+		die("--regex option is only allowed with -l.");
 	if (delete)
 		return for_each_tag_name(argv, delete_tag);
 	if (verify)
diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
index 69501e2..a93de89 100755
--- a/t/t7004-tag.sh
+++ b/t/t7004-tag.sh
@@ -256,6 +256,32 @@ test_expect_success \
 	test_cmp expect actual
 '
 
+# regex pattern matching:
+
+test_expect_success 'listing a tag using a matching regex pattern should succeed' \
+	'git tag -l --regex "^aa1$"'
+
+test_expect_success 'listing a tag using a nonmatching regex pattern should succeed' \
+	'git tag -l --regex "^xxxxxxxxxxxxx$"'
+
+test_expect_success \
+	'listing a tag using a matching regex pattern should output that tag' \
+	'test `git tag -l --regex "^aa1$"` = aa1'
+
+test_expect_success \
+	'listing tags using a matching regex pattern should output those tags' \
+	'test `git tag -l --regex "^v1.[[:digit:]]$"` = "v1.0"'
+
+cat > expected <<EOF
+v1.0.1
+v1.1.3
+EOF
+
+test_expect_success \
+	'listing a tag using a matching regex pattern should output that tag' \
+	'git tag -l --regex "^v1.[[:digit:]].[[:digit:]]$" > actual &&
+         test_cmp expected actual'
+
 # creating and verifying lightweight tags:
 
 test_expect_success \
-- 
1.6.0.6

Re: [RFC PATCH 1/1] git-tag: Add --regex option

[Junio C Hamano]

Jake Goulding <goulding@vivisimo.com> writes:

> Allows the tag pattern to be expressed as a regular expression.

We use shell globs for refname throughout the system (not just tags).  Why
is this a good thing, other than "because we can"?

Re: [RFC PATCH 1/1] git-tag: Add --regex option

[Jake Goulding]

Junio C Hamano wrote:
> Jake Goulding <goulding@vivisimo.com> writes:
> 
>> Allows the tag pattern to be expressed as a regular expression.
> 
> We use shell globs for refname throughout the system (not just tags).  Why
> is this a good thing, other than "because we can"?
> 

I'll give the particular use-case that we are using it for:

In preparation for a release, we have a nightly tagging/building
process. We start by tagging something as 1.0.0-build1. We then do that
series for a while, then decide it is time to shift to a more thorough
QA cycle. We branch a QA branch, then start tagging at 1.0-0-rc1.
Eventually, a rc passes all QA tests and we tag that rc again as 1.0-0.

Thus, our tags look like something of the form:

1.0.0-build1
1.0.0-rc1
1.0.0

As we fix bugs, a hook automatically adds the commit hash is as a
comment to the appropriate bugzilla bug.

We whipped up a dinky little web application that takes a hash and a
branch, and shows which tags contain that particular hash (which is the
reason for my previous commit for --contains support in git tag). We
hacked bugzilla to match on git hashes, and provide a link to this webapp.

I wanted to be able to limit the search space to (builds, rcs,
releases), but globs don't allow that amount of flexibility.

Is that a complete enough description for a rational use-case?

-Jake

Re: [RFC PATCH 1/1] git-tag: Add --regex option

[Junio C Hamano]

Jake Goulding <goulding@vivisimo.com> writes:

> Junio C Hamano wrote:
>> Jake Goulding <goulding@vivisimo.com> writes:
>> 
>>> Allows the tag pattern to be expressed as a regular expression.
>> 
>> We use shell globs for refname throughout the system (not just tags).  Why
>> is this a good thing, other than "because we can"?
>
> I'll give the particular use-case that we are using it for:
>
> In preparation for a release, we have a nightly tagging/building
> process. We start by tagging something as 1.0.0-build1. We then do that
> series for a while, then decide it is time to shift to a more thorough
> QA cycle. We branch a QA branch, then start tagging at 1.0-0-rc1.
> Eventually, a rc passes all QA tests and we tag that rc again as 1.0-0.
>
> Thus, our tags look like something of the form:
>
> 1.0.0-build1
> 1.0.0-rc1
> 1.0.0
>
> As we fix bugs, a hook automatically adds the commit hash is as a
> comment to the appropriate bugzilla bug.
>
> We whipped up a dinky little web application that takes a hash and a
> branch, and shows which tags contain that particular hash (which is the
> reason for my previous commit for --contains support in git tag). We
> hacked bugzilla to match on git hashes, and provide a link to this webapp.
>
> I wanted to be able to limit the search space to (builds, rcs,
> releases), but globs don't allow that amount of flexibility.
>
> Is that a complete enough description for a rational use-case?

It certainly describes what you are trying to use it for much better than
a patch that does not say anything other than "because we can".  A patch
marked as RFC could have been written with such an explanation from the
beginning to save everybody's time.

I still do not like the patch, and it is not entirely your fault.

Imagine we are not discussing git nor tags, but doing something similar on
the filesystem (e.g. you have files that store release-notes for key
versions) and would want to have a way to limit the filename arguments you
give to the commands, e.g. "wc -l 1.0.0*".  You most likely would not
patch your shell to accept "set regexpglob; wc -l 1\.0\.0.*".  Instead you
would arrange your naming convention to make it easy to limit to what you
are interested in by globbing.

In that sense, the above explanation does not change the fact at all that
your patch is "because we can".

But that is minor.

I liked the "git-tag --contains" patch, because the ref filters git-branch
has are really ref filters, not branch filters, and should not belong only
to git-branch.  But now that you revealed that you are using the Porcelain
for scripting, it made me realize that these features should be accessible
from the plumbing, perhaps for-each-ref, and your little web application
should be using that, not "git-branch" nor "git-tag".

Currently it cannot, because these useful ref filters are implemented
first at the Porcelain level.  Because the plumbing _is_ meant for people
writing scripts, that is the issue we should be fixing first.

I would have liked the patch if it were a series to refactor the code to
filter refs based on their relationships from "git branch" (one of which
you did with the --contains patch, but --merged, --no-merged should be
addressed, too, I think), and make them available to for-each-ref, branch
and tag.  If done right, adding regexp support or other fancier filtering
can be done by enhancing the shared ref filtering logic once and both
Porcelain and plumbing will benefit.

Adding --regexp only to "git-tag" is going backwards, as you would have to
then sideport that to "git-branch", and the new feature is still not
available to the plumbing.  Seeing such a patch from somebody who improved
the world by freeing --contains from the grip of "git-branch" makes me
moderately unhappy.  It shows that you did not understand why your earlier
patch was a good thing to begin with.

Re: [RFC PATCH 1/1] git-tag: Add --regex option

[Jake Goulding]

Junio C Hamano wrote:
> Jake Goulding <goulding@vivisimo.com> writes:
> 
>> Is that a complete enough description for a rational use-case?
> 
> It certainly describes what you are trying to use it for much better than
> a patch that does not say anything other than "because we can".  A patch
> marked as RFC could have been written with such an explanation from the
> beginning to save everybody's time.

My apologies - my previous patch, as you pointed out, was more
self-evident, and I felt this one was too.

> Imagine we are not discussing git nor tags, but doing something similar on
> the filesystem (e.g. you have files that store release-notes for key
> versions) and would want to have a way to limit the filename arguments you
> give to the commands, e.g. "wc -l 1.0.0*".  You most likely would not
> patch your shell to accept "set regexpglob; wc -l 1\.0\.0.*".  Instead you
> would arrange your naming convention to make it easy to limit to what you
> are interested in by globbing.

This is true. In this case, I would instead post-process the globbed
list with my regex, then pass the munged list to wc. I thought about
doing something similar here, where I would do the list, munge it, then
perform the contains test. Since I had just messed with git tag,
however, I felt like messing some more. :-) I

> I liked the "git-tag --contains" patch, because the ref filters git-branch
> has are really ref filters, not branch filters, and should not belong only
> to git-branch.  But now that you revealed that you are using the Porcelain
> for scripting, it made me realize that these features should be accessible
> from the plumbing, perhaps for-each-ref, and your little web application
> should be using that, not "git-branch" nor "git-tag".

This is a good point. I originally started by using for-each-ref, then
realized that it did not have contains. Someone on IRC suggested I look
at the functionality in git branch, and copy over to git tag. That is
how I arrived at the previous patch series.

> Currently it cannot, because these useful ref filters are implemented
> first at the Porcelain level.  Because the plumbing _is_ meant for people
> writing scripts, that is the issue we should be fixing first.

This is something that I constantly forget, and it happened again in
this case. I get so used to using the porcelain in day-to-day use, it is
the most prevalent thing in my head when I go to write simple scripts.

> I would have liked the patch if it were a series to refactor the code to
> filter refs based on their relationships from "git branch" (one of which
> you did with the --contains patch, but --merged, --no-merged should be
> addressed, too, I think), and make them available to for-each-ref, branch
> and tag.  If done right, adding regexp support or other fancier filtering
> can be done by enhancing the shared ref filtering logic once and both
> Porcelain and plumbing will benefit.

> Adding --regexp only to "git-tag" is going backwards, as you would have to
> then sideport that to "git-branch", and the new feature is still not
> available to the plumbing.  Seeing such a patch from somebody who improved
> the world by freeing --contains from the grip of "git-branch" makes me
> moderately unhappy.  It shows that you did not understand why your earlier
> patch was a good thing to begin with.

Well, in my defense, it still is a good thing, just not as good as it
could have been. I will admit that I do not have a total grasp of the
git code, but I figured (correctly) that someone would be kind enough to
point it out when I show my ignorance ;-).

I agree that it feels dumb to add this type of functionality to git tag
and not git branch (and not to the plumbing).

I'm sure that adding the regex support to for-each-ref would be nearly
as trivial as it was for git tag, and I will happily do that instead of
this patch.

What I am not as clear about is how I can then use that functionality in
git tag/branch. The main code I messed with in git tag calls
for_each_tag_ref (and for_each_ref in branch). Would it be appropriate
to add a struct reference_filter to these functions?

Really, that is what I am trying to accomplish - to have finer-grained
filtering on the references that are being enumerated.

If that seems fine, then I will work on refactoring that code. If not,
let me know. :-)

-Jake

Re: [RFC PATCH 1/1] git-tag: Add --regex option

[Junio C Hamano]

Jake Goulding <goulding@vivisimo.com> writes:

> Junio C Hamano wrote:
>> Jake Goulding <goulding@vivisimo.com> writes:
>> 
>>> Is that a complete enough description for a rational use-case?
>> 
>> It certainly describes what you are trying to use it for much better than
>> a patch that does not say anything other than "because we can".  A patch
>> marked as RFC could have been written with such an explanation from the
>> beginning to save everybody's time.
>
> My apologies - my previous patch, as you pointed out, was more
> self-evident, and I felt this one was too.

There is no need to apologize.  If a patch without RFC does not have
sufficient justification, I'd just reject it and that would be that, but
an RFC patch is for people to comment on, and it would be a way for you to
get more feedback to give sufficient background material.  It was just a
suggestion to help _you_, not request to help _me_.

>> Currently it cannot, because these useful ref filters are implemented
>> first at the Porcelain level.  Because the plumbing _is_ meant for people
>> writing scripts, that is the issue we should be fixing first.
> ...
> Well, in my defense, it still is a good thing, just not as good as it
> could have been.

Please don't take my "Sad" too seriously.  As I said upfront, it is not
your fault.  I was sad mostly because the existing code was not structured
that way before you started touching it.

> What I am not as clear about is how I can then use that functionality in
> git tag/branch. The main code I messed with in git tag calls
> for_each_tag_ref (and for_each_ref in branch). Would it be appropriate
> to add a struct reference_filter to these functions?

Yeah, if we realize that for_each_*_ref() are all based on for_each_ref()
and about implementing trivial filtering on the result from the latter,
it might be a reasonable approach to do something like this.

	/*
         * generic filter description.  base is typically "refs/heads/"
         * and things like that to cheaply filter the ref.
         *
         * filter_one callback can return 0 to skip, or positive to call
         * the user function supplied to for_each_ref_filtered(), one by
         * one.
         *
         * when filtering many refs at once is more efficient,
         * filter_group can be specified. for_each_ref_filtered() function
         * first collects all the eligible refs into a ref-list, and calls
         * the filter_group callback.  The callback is expected to modify
         * the given ref_list in-place to omit the ones it does not want
         * the user callback to be called. for_each_ref_filtered() will
         * then call the user callback for each of the refs left in the
         * ref_list.
         *
         * when using filter_group, callback data can be placed in
         * cb_data.  when using filter_one callback, the field is used to
         * hold the callback data for the user callback function.
         */
	struct ref_filter {
		char *base;
                each_ref_fn filter_one;
                int (*filter_group)(struct ref_list **, struct ref_filter *);
		void *cb_data;
		each_ref_fn user_fn;
        };

	static int collect_refs(const char *name,
        			const unsigned char *sha1,
                                int flags, void *cb_data)
        {
		struct ref_list **tail = cb_data;
                struct ref_list *entry;
                entry = xmalloc(sizeof(*entry) + strlen(name) + 1);
                strcpy(entry->name, name);
                entry->flag = flag;
                memcpy(entry->sha1, sha1, 20);
                hashclr(entry->peeled);
		*tail = entry;
                entry->next = NULL;
                **tail = &entry->next;
                return 0;
	}

	static int filter_ref(const char *name,
        		      const unsigned char *sha1,
                              int flags, void *cb_data)
        {
		struct ref_filter *filter = cb_data;
		int ret = filter->filter_one(name, sha1, flags, cb_data);
		if (ret <= 0)
                	return ret;
		return filter->user_fn(name, sha1, flags, filter->cb_data);
        }

        int for_each_ref_filtered(each_ref_fn fn,
        			  struct ref_filter *filter,
                                  void *cb_data)
	{
		char *base = filter->base;

		if (!filter->filter_one && !filter->filter_group)
			return for_each_ref(fn, base, cb_data);
                if (filter->filter_group) {
			struct ref_list *collect = NULL;
                        struct ref_list **tail = &collect;
			for_each_ref(collect_refs, base, tail);
			filter->filter_group(&collect_refs, filter->cb_data);
                        while (collect) {
				struct ref_list *entry = collect;
                                retval = do_one_ref(base, fn, 0, entry);
                                if (retval) {
                                	free_ref_list(collect);
                                        return retval;
				}
				collect = entry->next;
                                free(entry);
                        }
                        return 0;
                }
		filter->user_fn = fn;
                return for_each_ref(filter_ref, base, filter);
        }