From: Jeff King <peff@peff.net>
Date: Tue, 26 Mar 2013 21:33:19 GMT
Subject: Re: [PATCH 3/4] attr.c::path_matches(): special case paths that end with a slash
Message-ID: <20130326213319.GA3058@sigill.intra.peff.net>
In-Reply-To: <20130326190558.GC26462@sigill.intra.peff.net>

On Tue, Mar 26, 2013 at 03:05:58PM -0400, Jeff King wrote:

> On Tue, Mar 26, 2013 at 11:39:30AM -0700, Junio C Hamano wrote:
> 
> > A similar adjustment for match_pathname() might be needed, but I
> > didn't look into it.
> 
> I notice that match_pathname takes _two_ lengths for the pattern: the
> nowildcardlen (called "prefix", and the full patternlen). But the first
> thing it does is:
> 
>   if (*pattern == '/') {
>           pattern++;
>           prefix--;
>   }
> 
> which seems obviously wrong, as patternlen should be dropped, too. But
> we do not seem to look at patternlen at all! I think we can drop the
> parameter totally.
> 
> We do seem to use strncmp_icase through the rest of the function,
> though, which should be OK. The one exception is that we call fnmatch at
> the end. Should the allocation hack from the previous patch make its way
> into an "fnmatch_icase_bytes()" function, so we can use it here, too?
> And then when we have a more efficient solution, we can just plug it in
> there.

Hmm. match_pathname does have this:

        /*
         * baselen does not count the trailing slash. base[] may or
         * may not end with a trailing slash though.
         */
        if (pathlen < baselen + 1 ||
            (baselen && pathname[baselen] != '/') ||
            strncmp_icase(pathname, base, baselen))
                return 0;

which seems to imply that the trailing slash is important here, and that
we should not drop it when passing the path to match_pathname. I'm
still trying to figure out exactly what it is that the extra slash check
is for, and whether it might not have the same problem.

-Peff