RFC patchgit-tag: Add --regex option

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(-)

Jake Goulding <goulding@vivisimo.com> writes:

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

Junio C Hamano wrote:

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

Jake Goulding <goulding@vivisimo.com> writes:

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.

Junio C Hamano wrote:

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

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

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.

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.

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

Jake Goulding <goulding@vivisimo.com> writes:

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

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.

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);
        }