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

From: David Aguilar <davvid@gmail.com>
Date: May 17, 2013, 08:21 UTC
Message-ID: <CAJDDKr726C5BZwufz5o7JWZiP3pKce4g=AZvEbj8qzk8dOqzQA@mail.gmail.com>
In-Reply-To: <CAPig+cTHh7iEY0+rReQ2LC94CsX-_aKdMLiVnL0ZF-FtKL6DaQ@mail.gmail.com>

On Thu, May 16, 2013 at 11:18 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:

Show 46 quoted lines

> 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.
>
> [1]: /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.8.sdk/usr/include/CommonCrypto/CommonDigest.h

This is a nice justification for taking v5 of this series over v6.

Sorry for all the churn in this series, Junio. I wrote v5 so I certainly felt it was a good idea at the time, and I feel bad for not having waited longer before sending out v6 (which is what was eventually queued in "pu").

Do you have advice on how we should proceed? :sigh: sorry for wasting so much maintainer time on this series already. If you need any resends or anything please let me know. This time I'll wait for a strong opinion before firing off patches.

My opinion: yeah, v5's ([NO_]APPLE_COMMON_CRYPTO) was probably where we should have stopped painting. Hindsight is 20/20. Luckily it never left "pu". -- David