From: Joshua Jensen <jjensen@workspacewhiz.com>
Date: Tue, 21 Jul 2009 15:55:17 GMT
Subject: Re: [PATCH] Added support for core.ignorecase when excluding gitignore entries
Message-ID: <4A65E4E5.3030709@workspacewhiz.com>
In-Reply-To: <20090720153737.GF5347@coredump.intra.peff.net>

----- Original Message -----
From: Jeff King
Date: 7/20/2009 9:37 AM
>>> 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.
>>>       
> 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.
>   
I agree.  I will update the patch soon.

Josh