Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 17, 2013, 17:40 UTC
Message-ID: <CAPig+cSY=6abuqtayCvTjrUxBfMLQiZHqLKz26XR7kGTesdYUw@mail.gmail.com>
In-Reply-To: <CAJDDKr726C5BZwufz5o7JWZiP3pKce4g=AZvEbj8qzk8dOqzQA@mail.gmail.com>

On Fri, May 17, 2013 at 4:21 AM, David Aguilar <davvid@gmail.com> wrote:

Show 47 quoted lines

> On Thu, May 16, 2013 at 11:18 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
>> On Wed, May 15, 2013 at 1:56 PM, Torsten Bögershausen <tboegi@web.de> wrote:
>>> On 2013-05-15 09.11, David Aguilar wrote:
>>>> +     ifndef NO_APPLE_COMMON_CRYPTO
>>>> +             APPLE_COMMON_CRYPTO = YesPlease
>>>> +     endif
>>>>       NO_REGEX = YesPlease
>>>>       PTHREAD_LIBS =
>>>>  endif
>>>> @@ -1389,10 +1396,16 @@ ifdef PPC_SHA1
>>>>       LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
>>>>       LIB_H += ppc/sha1.h
>>>>  else
>>>> +ifdef APPLE_COMMON_CRYPTO
>>>> +     BASIC_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
>>>> +     SHA1_HEADER = <CommonCrypto/CommonDigest.h>
>>>
>>> Would it make sense to replace APPLE_COMMON_CRYPTO
>>> with COMMON_DIGEST_FOR_OPENSSL ?
>>>
>>> In the spirit of other Makefile-defines becoming Compiler defines,
>>> a random picked example:
>>> ifdef NO_STRTOULL
>>>         COMPAT_CFLAGS += -DNO_STRTOULL
>>> endif
>>
>> Not necessarily. Unlike NO_STRTOULL and cousins,
>> COMMON_DIGEST_FOR_OPENSSL is not a Git build tweak; it is merely a
>> (public) implementation detail of the Apple header [1] to magically
>> associate OpenSSL digest functions with CommonCrypto counterparts.
>> It's not the only such macro recognized by the Apple headers. For
>> instance, COMMON_DIGEST_FOR_RFC_1321 magically associates legacy MD5
>> digest functions with CommonCrypto counterparts.
>>
>> Further, as Junio noted elsewhere, David is using CommonCrypto for
>> HMAC replacements, not just for digest replacements, so a Makefile
>> knob with DIGEST in its name is not really appropriate. More
>> generally, David would like to find CommonCrypto replacements for all
>> the OpenSSL functionality, so a Makefile knob named after DIGEST is
>> too specific.
>>
>> These considerations motivated the original suggestion for a single
>> Git Makefile knob to enable/disable, as a unit, all CommonCrypto
>> replacements. Such a knob would naturally have COMMON_CRYPTO as part
>> of its name.
>
> This is a nice justification for taking v5 of this series over v6.

You will consider this bike-shedding (I don't), but the above also is good justification for revising your HMAC patch to _not_ rely on COMMON_DIGEST_FOR_OPENSSL, which is an implementation detail of your SHA patch, rather than a proper build knob.

Similar to NO_STRTOULL and cousins, you should have a #define (such as NO_APPLE_COMMON_CRYPTO or NO_COMMON_CRYPTO) which is consulted by your HMAC patch and any future patches you submit to map CommonCrypto counterparts to OpenSSL functions. The fact that you also must #define COMMON_DIGEST_FOR_OPENSSL for the SHA patch is just an implementation detail of that one patch; it is not relevant to the other patches.

-- ES