On Thu, Jul 16, 2009 at 07:15:26AM -0600, Joshua Jensen wrote:
> >Should we actually be converting the filesystem names into a canonical
> >format as they are read? IIRC, Linus posted some patches a few weeks ago
> >about "git path" versus "filesystem path", but I didn't actually look
> >too closely.
> I'm game for whatever. Git actually has a lot of places where it
> doesn't pay attention to core.ignorecase, and having a standard and
> correct method of comparing filenames would make it easier to handle
> core.ignorecase=true in a more global fashion.
Like I said, I'm not sure what the status of that is, so probably
something simple like your patch makes sense in the interim (unless we
hear from somebody more clueful).
> >If your patch is the right route, it might be nice to collapse the
> >comparison into its own function. You end up cutting and pasting a lot
> >of the related conditionals and returns (like above, where 2 lines
> >become 9), so it might make sense to do something like:
> >
> > int filename_cmp(const char *a, const char *b, int ignore_case)
> > {
> > return ignore_case ? strcasecmp(a, b) : strcmp(a, b);
> > }
> >
> >and then just s/strcmp/filename_cmp/ at the appropriate callsites.
> I started off with this method, but it required two functions, one
> with the strcmp() and one for strncmp(). In fact, in other places in
> the code, Git uses memcmp() for comparison. Is that, then, three
> filename comparison functions, dependent upon intent? At that point,
> it felt like my change wasn't as self contained anymore, so I then
> wrote what I posted to the list to get feedback.
IMHO, you are better off even with three wrapper functions, just because
they are all very straightforward. Whereas with your patch, I felt like
the innards of complex functions got harder to read because of big
duplicate conditionals. But that's just my two cents.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html