From: Joshua Jensen <jjensen@workspacewhiz.com>
Date: Sun, 03 Oct 2010 09:07:33 GMT
Subject: Re: [PATCH v2 1/6] Add string comparison functions that respect the ignore_case variable.
Message-ID: <4CA847D5.4000903@workspacewhiz.com>
In-Reply-To: <AANLkTikU7D5dWAc-04cVUnjPPrC7rjaqjPe_j3rEvn0u@mail.gmail.com>

  ----- Original Message -----
From: Ævar Arnfjörð Bjarmason
Date: 10/3/2010 2:30 AM
> On Sun, Oct 3, 2010 at 04:32, Joshua Jensen<jjensen@workspacewhiz.com>  wrote
>> +int fnmatch_casefold(const char *pattern, const char *string, int flags)
>> +{
>> +       char lowerPatternBuf[MAX_PATH];
>> +       char lowerStringBuf[MAX_PATH];
>> +       char* lowerPattern;
>> +       char* lowerString;
>> +       size_t patternLen;
>> +       size_t stringLen;
>> +       char* out;
>> +       int ret;
>> +
>> +       /*
>> +        * Use the provided stack buffer, if possible.  If the string is too
>> +        * large, allocate buffer space.
>> +        */
>> +       patternLen = strlen(pattern);
>> +       if (patternLen + 1>  sizeof(lowerPatternBuf))
>> +               lowerPattern = xmalloc(patternLen + 1);
>> +       else
>> +               lowerPattern = lowerPatternBuf;
>> +
>> +       stringLen = strlen(string);
>> +       if (stringLen + 1>  sizeof(lowerStringBuf))
>> +               lowerString = xmalloc(stringLen + 1);
>> +       else
>> +               lowerString = lowerStringBuf;
>> +
>> +       /* Make the pattern and string lowercase to pass to fnmatch. */
>> +       for (out = lowerPattern; *pattern; ++out, ++pattern)
>> +               *out = tolower(*pattern);
>> +       *out = 0;
>> +
>> +       for (out = lowerString; *string; ++out, ++string)
>> +               *out = tolower(*string);
>> +       *out = 0;
>> +
>> +       ret = fnmatch(lowerPattern, lowerString, flags);
>> +
>> +       /* Free the pattern or string if it was allocated. */
>> +       if (lowerPattern != lowerPatternBuf)
>> +               free(lowerPattern);
>> +       if (lowerString != lowerStringBuf)
>> +               free(lowerString);
>> +       return ret;
>> +}
>> +
>> +int fnmatch_icase(const char *pattern, const char *string, int flags)
>> +{
>> +       return ignore_case ? fnmatch_casefold(pattern, string, flags) : fnmatch(pattern, string, flags);
>> +}
>
> I liked v1 of this patch better, although it obviously had portability
> issues. But I think it would be better to handle this with:
>
>      #ifndef FNM_CASEFOLD
>      int fnmatch_casefold(const char *pattern, const char *string, int flags)
>      {
>          ...
>      }
>      #endf
>
>      int fnmatch_icase(const char *pattern, const char *string, int flags)
>      {
>      #ifndef FNM_CASEFOLD
>             return ignore_case ? fnmatch_casefold(pattern, string,
> flags) : fnmatch(pattern, string, flags);
>      #else
>              return fnmatch(pattern, string, flags | (ignore_case ?
> FNM_CASEFOLD : 0));
>      #endif
>      }
>
> Or simply use fnmatch(..., FNM_CASEFOLD) everywhere and include
> compat/fnmatch/* on platforms like Solaris that don't have the GNU
> extension.
The real problem with compat/fnmatch is determining which random 
platforms need that support and updating the makefile accordingly.  
Further, the compat/fnmatch/* code would need to be rejigged somewhat, 
so there is no possible conflict (now or in the future) with the 
provided symbols.  We discussed this as a potential problem developers 
would need to be aware of if the system fnmatch.h (or whatever it is 
called) gets #included.

Anyway, what you describe above creates two code paths.  I would imagine 
that would be harder to debug; that is, on some platforms, it uses 
fnmatch_casefold and on others, it hands it off to fnmatch(..., 
FNM_CASEFOLD).

In any case, I'd like to find a solution to get this series working for 
everyone.  I've been out of commission for a month (deploying Git to 80+ 
programmers at an organization, by the way), but I'm back now and can 
work this until it is complete.

Josh