Re: [PATCH] imap-send: use Apple's Security framework for base64 encoding

From: David Aguilar <davvid@gmail.com>
Date: Jul 29, 2013, 07:23 UTC
Message-ID: <CAJDDKr7=Y=zRHv_NRS64tXN=bw8hCZ503oVP+96TAkSzYbV9ug@mail.gmail.com>
In-Reply-To: <20130729033533.GB2890@elie.Belkin>

On Sun, Jul 28, 2013 at 8:35 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Show 35 quoted lines

> Hi,
>
> David Aguilar wrote:
>
>> --- a/imap-send.c
>> +++ b/imap-send.c
>> @@ -22,14 +22,11 @@
>>   *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA  02111-1307  USA
>>   */
>>
>> -#include "cache.h"
>> -#include "exec_cmd.h"
>> -#include "run-command.h"
>> -#include "prompt.h"
>>  #ifdef NO_OPENSSL
>>  typedef void *SSL;
>>  #else
>>  #ifdef APPLE_COMMON_CRYPTO
>> +/* git-compat-util.h overwrites ctype.h; this must be included first */
>>  #include <CommonCrypto/CommonHMAC.h>
>
> Thanks for your work on this.
>
> Currently each translation unit of git includes git-compat-util.h or a
> header like cache.h that includes git-compat-util.h before doing
> anything else, since otherwise feature test macros are not set before
> the first system header is included.
>
> The above (CommonCrypto needing to be included before some of the
> definitions from git-compat-util.h) suggests to me that CommonCrypto
> should just be included directly from git-compat-util.h in some
> appropriate place.  That way any other header that needs CommonCrypto
> routines only has to include git-compat-util.h first as usual and
> doesn't have to worry about the order of other #includes.  Could that
> work?

imap-send is currently the only user of this stuff; I believe this would pull in these library dependencies for all builtins.

If we don't mind a new file, something like git-compat-crypto.h could be a nice place to tuck all these #ifdefs away. If we had another command that needed these then it'd be a easier to justify a new file.

A test-crypto command in the test suite could be written to verify that the implementations work as advertised. That counts as "another command", albeit an internal one. I don't believe imap-send has any test coverage so pulling this stuff out would help make it more testable, which is a net win. Libifying might be a little premature, though.

Thoughts?

-- 
David