From: Jeff King <peff@peff.net>
Date: Fri, 22 Mar 2013 22:24:39 GMT
Subject: Re: [regression?] trailing slash required in .gitattributes
Message-ID: <20130322222438.GA13207@sigill.intra.peff.net>
In-Reply-To: <20130319181042.GA14295@sigill.intra.peff.net>

On Tue, Mar 19, 2013 at 02:10:42PM -0400, Jeff King wrote:

> > The issue bisects to 94bc671 (Add directory pattern matching to
> > attributes, 2012-12-08). That commit actually tests not only that
> > "subdir/" matches, but also that just "subdir" does not match.
> [...]
> So I think the regression is accidental. And we would want tests like
> this on top (which currently fail):
> [...]

I'm having trouble figuring out the right solution for this.

The problem is in path_matches, which used to receive just the unadorned
pathname, and now receives "path/" for directories. It now looks like
this:

> static int path_matches(const char *pathname, int pathlen,
> 			const char *basename,
> 			const struct pattern *pat,
> 			const char *base, int baselen)
> {
> 	const char *pattern = pat->pattern;
> 	int prefix = pat->nowildcardlen;
> 
> 	if ((pat->flags & EXC_FLAG_MUSTBEDIR) &&
> 	    ((!pathlen) || (pathname[pathlen-1] != '/')))
> 		return 0;

This first stanza checks that a pattern like "foo/" must be matched by a
real directory. Which is fine; that's the point of adding the "/" to the
pattern.

> 	if (pat->flags & EXC_FLAG_NODIR) {
> 		return match_basename(basename,
> 				      pathlen - (basename - pathname),
> 				      pattern, prefix,
> 				      pat->patternlen, pat->flags);
> 	}
> 	return match_pathname(pathname, pathlen,
> 			      base, baselen,
> 			      pattern, prefix, pat->patternlen, pat->flags);
> }

But then here we'll end up feeding "foo/" to be compared with "foo",
which we don't want. For a pattern "foo", we want to match _either_
"foo/" or "foo". So you'd think something like:

  if (pathlen && pathname[pathlen-1] == '/')
          pathlen--;

would work. But it seems that match_basename, despite taking the length
of all of the strings we pass it, will happily use NUL-terminated
functions like strcmp or fnmatch. Converting the former to check lengths
should be pretty straightforward. But there is no version of fnmatch
that does what we want. I wonder if we using wildmatch can get around
this limitation.

-Peff