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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2012, 06:33 UTC
Message-ID: <7vobsrbcny.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1329874130-16818-2-git-send-email-tmgrennan@gmail.com>

Tom Grennan <tmgrennan@gmail.com> writes:

Show 10 quoted lines

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

Show 23 quoted lines

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

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

 - 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?

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

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

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.

Thanks.