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

On Tue, Feb 21, 2012 at 10:33:05PM -0800, Junio C Hamano wrote:

Tom Grennan [off-list ref] writes:

quoted

+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

quoted

+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