Re: [PATCH 1/2] rev-parse --glob

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 20, 2010, 10:17 UTC
Message-ID: <7vska13wt0.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1263980906-11058-1-git-send-email-ilari.liusvaara@elisanet.fi>

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:

Show 12 quoted lines

> Add --glob=<glob-pattern> option to rev-parse and everything that
> accepts its options. This option matches all refs that match given
> shell glob pattern (complete with some DWIM logic).
>
> Example:
>
> 'git log --branches --not --glob=remotes/origin'
>
> To show what you have that origin doesn't.
>
> Signed-off-by: Ilari Liusvaara <ilari.liusvaara@elisanet.fi>
> ---

Looks fairly competently done. Does this mean you've abandoned the --namespace series? I don't mind reverting them from 'next'.

Show 6 quoted lines

> +static int filter_refs(const char* ref, const unsigned char *sha, int flags,
> +	void* data)
> +{
> +	struct ref_filter *filter = (struct ref_filter*)data;
> +	if (fnmatch(filter->pattern, ref, 0))
> +		return 0;

In many places (not just refs) we use "If leading path component matches, or fnmatch() says Ok" (e.g. builtin-for-each-ref.c, builtin_grep.c), and in some other places we only use fnmatch(). I think it is better to pick one convention, and adjust the other one to match, and my gut feeling is that "leading path or fnmatch()", while theologically might be messier, would be more useful in practice. And that was why I did my "like this" patch http://article.gmane.org/gmane.comp.version-control.git/137492 that way.

My longer term desire is to consolidate these fnmatch() users to call a single helper function, and then perhaps replace fnmatch() in that helper function with something more powerful. Although I do not personally care too much, people often wanted to have "path/**/leaf-that-is-deep" matcher.

I don't want you to go there just yet with this series, but at least following the same style everywhere would pave the way to consolidating them.

Show 16 quoted lines

> @@ -674,6 +691,35 @@ int for_each_replace_ref(each_ref_fn fn, void *cb_data)
>  	return do_for_each_ref("refs/replace/", fn, 13, 0, cb_data);
>  }
>  
> +int for_each_glob_ref(each_ref_fn fn, const char *pattern, void *cb_data)
> +{
> +	struct strbuf real_pattern = STRBUF_INIT;
> +	struct ref_filter filter;
> +	const char *has_glob_specials;
> +	int ret;
> +
> +	if (prefixcmp(pattern, "refs/"))
> +		strbuf_addstr(&real_pattern, "refs/");
> +	strbuf_addstr(&real_pattern, pattern);
> +
> +	has_glob_specials = strpbrk(pattern, "?*[");

Would the need for this code hopefully disappear if we take the "leading path or fnmatch()" check? If we still need this, we probably would want to add a has_glob_special() helper function not as a hardcoded srpbrk(), but in terms of is_glob_special() in git-compat-util.h.

Thanks.