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

[Junio C Hamano <gitster@pobox.com>]

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

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

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

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