Re: [PATCHv3 1/5] refs: add match_pattern()

[Tom Grennan <tmgrennan@gmail.com>]

On Tue, Feb 21, 2012 at 10:33:05PM -0800, Junio C Hamano wrote:
>Tom Grennan <tmgrennan@gmail.com> writes:
>
>> +static int match_path(const char *name, const char *pattern, int nlen)
>> +{
>> +	int plen = strlen(pattern);
>> +
>> +	return ((plen <= nlen) &&
>> +		!strncmp(name, pattern, plen) &&
>> +		(name[plen] == '\0' ||
>> +		 name[plen] == '/' ||
>> +		 pattern[plen-1] == '/'));
>> +}
>
>This is a counterpart to the tail match found in ls-remote, so we would
>want to call it with a name that makes it clear this is a leading path
>match not just "path" match.  Perhaps match_leading_path() or something.

OK

>> +int match_pattern(const char *name, const char **match,
>> +		  struct string_list *exclude, int flags)
>> +{
>> +	int nlen = strlen(name);
>> +
>> +	if (exclude) {
>> +		struct string_list_item *x;
>> +		for_each_string_list_item(x, exclude) {
>> +			if (!fnmatch(x->string, name, 0))
>> +				return 0;
>> +		}
>> +	}
>> +	if (!match || !*match)
>> +		return 1;
>> +	for (; *match; match++) {
>> +		if (flags == FNM_PATHNAME)
>> +			if (match_path(name, *match, nlen))
>> +				return 1;
>> +		if (!fnmatch(*match, name, flags))
>> +			return 1;
>> +	}
>> +	return 0;
>> +}
>
>As an API for a consolidated and generic function, the design needs a bit
>more improving, I would think.
>
> - The name match_pattern() was OK for a static function inside a single
>   file, but it is way too vague for a global function. This is to match
>   refnames, so I suspect there should at least be a string "ref_"
>   somewhere in its name.

OK

> - You pass "flags" argument, so that later we _could_ enhance the
>   implementation to cover needs for new callers, but alas, it uses its
>   full bits to express only one "do we do FNM_PATHNAME or not?" bit of
>   information, so essentially "flags" does not give us any expandability.

I agree.

> - Is it a sane assumption that a caller that asks FNM_PATHNAME will
>   always want match_path() semantics, too?  Aren't these two logically
>   independent?

Yes, these should be ligically independent although the current use has
combined them.

> - Is it a sane assumption that a caller that gives an exclude list will
>   want neither FNM_PATHNAME semantics nor match_path() semantics?

I'm not sure.  I tried using FNM_PATHNAME with both exclusion and match
patterns of git-for-each-ref but I couldn't get it to do something like
this:
	git for-each-ref ... --exclude '*HEAD' refs/remotes/

I don't remember if this worked,
	git for-each-ref ... --exclude HEAD refs/remotes/

Now I see how an implicit TRAILING match would be useful,
	git for-each-ref ... --exclude /HEAD refs/remotes/

Where git-for-each-ref uses this flag:
	REF_MATCH_LEADING | REF_MATCH_TRAILING | REF_MATCH_FNM_PATH

I'll experiment with this more. 

> - Positive patterns are passed in "const char **match", and negative ones
>   are in "struct string_list *". Doesn't the inconsistency strike you as
>   strange?

Yes, I tried to minimize change but the conversion of argv's to
string_list's won't add that much.

>Perhaps like...
>
>#define REF_MATCH_LEADING       01
>#define REF_MATCH_TRAILING      02
>#define REF_MATCH_FNM_PATH      04
>
>static int match_one(const char *name, size_t namelen, const char *pattern,
>		unsigned flags)
>{
>       	if ((flags & REF_MATCH_LEADING) &&
>            match_leading_path(name, pattern, namelen))
>		return 1;
>       	if ((flags & REF_MATCH_TRAILING) &&
>            match_trailing_path(name, pattern, namelen))
>		return 1;
>	if (!fnmatch(pattern, name, 
>		     (flags & REF_MATCH_FNM_PATH) ? FNM_PATHNAME : 0))
>		return 1;
>	return 0;
>}
>
>int ref_match_pattern(const char *name,
>		const char **pattern, const char **exclude, unsigned flags)
>{
>	size_t namelen = strlen(name);
>        if (exclude) {
>		while (*exclude) {
>			if (match_one(name, namelen, *exclude, flags))
>				return 0;
>			exclude++;
>		}
>	}
>        if (!pattern || !*pattern)
>        	return 1;
>	while (*pattern) {
>		if (match_one(name, namelen, *pattern, flags))
>			return 1;
>		pattern++;
>	}
>        return 0;
>}
>
>and then the caller could do something like
>
>	ref_match_pattern("refs/heads/master",
>        		  ["maste?", NULL],
>                          ["refs/heads/", NULL],
>                          (REF_MATCH_FNM_PATH|REF_MATCH_LEADING));
>
>Note that the above "ref_match_pattern()" gives the same "flags" for the
>call to match_one() for elements in both positive and negative array and
>it is very deliberate.  See review comment to [3/5] for the reasoning.

OK, I think that I understand, but please confirm, you'd expect no output in
the above example, right?

-- 
TomG