Re: [PATCH 4/6] dir.c::match_pathname(): pay attention to the length of string parameters

From: Jeff King <peff@peff.net>
Date: Mar 29, 2013, 17:04 UTC
Message-ID: <20130329170459.GB3552@sigill.intra.peff.net>
In-Reply-To: <7vli96p34f.fsf@alter.siamese.dyndns.org>

On Fri, Mar 29, 2013 at 09:44:32AM -0700, Junio C Hamano wrote:

Show 23 quoted lines

> Duy Nguyen <pclouds@gmail.com> writes:
> 
> >> So we would want to do any adjustment to the fix when we merge up to
> >> maint.
> >
> > OK. Then Junio, you may need to resolve the conflict with something
> > like this. Originally match_basename uses fnmatch, not wildmatch. But
> > using wildmatch there too should be fine, now that both
> > match_{base,path}name share fnmatch_icase_mem().
> 
> Thanks.
> 
> The result still smells somewhat funny, though.
> 
> fnmatch_icase_mem() is meant to be a wrapper of fnmatch_icase() for
> counted strings and its matching semantics should be the same as
> fnmatch_icase().
> 
> With the merge-fix, fnmatch_icase_mem() calls into wildmatch(), but
> fnmatch_icase() still calls into fnmatch().
> 
> The latter's flags are meant to be taken from FNM_* family, but the
> former takes flags from WM_* family of bits, no?

Yeah, that does not seem right. If match_pathname has learned to call into wildmatch instead of fnmatch_icase in the interim, then the right resolution is to convert its call to fnmatch_icase_mem to a new wildmatch_mem. Presumably that can be done by either tweaking fnmatch_icase_mem, or, if wildmatch is ready to take counted strings, calling into it with the right options.

> I think you are running with USE_WILDMATCH which may make the
> differences harder to notice, but the name fnmatch_icase_mem() that is
> not in the same family as fnmatch but is from the wildmatch() family
> smells like an accident waiting to happen.

Agreed.

> I tend to think in the longer term it may be a good idea to build with
> USE_WILDMATCH unconditionally (we can lose compat/fnmatch), so in the
> end this may not matter that much

Yeah, I think that is a sane long-term goal.

-Peff