All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
@ 2013-05-15  7:11 David Aguilar
  2013-05-15  7:11 ` [PATCH v5 2/2] imap-send: eliminate HMAC " David Aguilar
  2013-05-15 17:56 ` [PATCH v5 1/2] cache.h: eliminate SHA-1 " Torsten Bögershausen
  0 siblings, 2 replies; 13+ messages in thread
From: David Aguilar @ 2013-05-15  7:11 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jonathan Nieder, Eric Sunshine

Mac OS X 10.8 Mountain Lion prints warnings when building git:

	warning: 'SHA1_Init' is deprecated
	(declared at /usr/include/openssl/sha.h:121)

Silence the warnings by using the CommonCrytpo SHA-1
functions for SHA1_Init(), SHA1_Update(), and SHA1_Final().

COMMON_DIGEST_FOR_OPENSSL is defined to enable the OpenSSL
compatibility macros in CommonDigest.h.

Add a NO_APPLE_COMMON_CRYPTO option to the Makefile to allow
users to opt out of using this library.  When defined, Git will
use OpenSSL instead.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
Both of these are replacement patches "pu".

Changes from last time:

It now uses a single APPLE_COMMON_CRYPTO definition.
Users can now opt-out by setting NO_APPLE_COMMON_CRYPTO.

 Makefile | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Makefile b/Makefile
index f698c1a..8309c41 100644
--- a/Makefile
+++ b/Makefile
@@ -137,6 +137,10 @@ all::
 # specify your own (or DarwinPort's) include directories and
 # library directories by defining CFLAGS and LDFLAGS appropriately.
 #
+# Define NO_APPLE_COMMON_CRYPTO if you are building on Darwin/Mac OS X
+# and do not want to use Apple's CommonCrypto library.  This allows you
+# to provide your own OpenSSL library, for example from MacPorts.
+#
 # Define BLK_SHA1 environment variable to make use of the bundled
 # optimized C SHA1 routine.
 #
@@ -1054,6 +1058,9 @@ ifeq ($(uname_S),Darwin)
 			BASIC_LDFLAGS += -L/opt/local/lib
 		endif
 	endif
+	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>
+else
 	SHA1_HEADER = <openssl/sha.h>
 	EXTLIBS += $(LIB_4_CRYPTO)
 endif
 endif
+endif
+
 ifdef NO_PERL_MAKEMAKER
 	export NO_PERL_MAKEMAKER
 endif
-- 
1.8.3.rc2.2.gbc955d1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH v5 2/2] imap-send: eliminate HMAC deprecation warnings on Mac OS X
  2013-05-15  7:11 [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X David Aguilar
@ 2013-05-15  7:11 ` David Aguilar
  2013-05-15 17:56 ` [PATCH v5 1/2] cache.h: eliminate SHA-1 " Torsten Bögershausen
  1 sibling, 0 replies; 13+ messages in thread
From: David Aguilar @ 2013-05-15  7:11 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jonathan Nieder, Eric Sunshine

Mac OS X 10.8 Mountain Lion warns that HMAC_Init() and friends
are deprecated.  Detect the COMMON_CRYPTO_FOR_OPENSSL definition
and use CommonCrypto's HMAC functions to eliminate the warnings.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
Changes since last time:

This version re-uses the existing COMMON_CRYPTO_FOR_OPENSSL define
instead of tweaking the Makefile to add a new one, so it's simpler.

My previous patch had Jonathan's reviewed-by tag, but he hasn't
reviewed this exact patch, so I removed it.  The C macros are unchanged.

 imap-send.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/imap-send.c b/imap-send.c
index d9bcfb4..96012b1 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -29,8 +29,18 @@
 #ifdef NO_OPENSSL
 typedef void *SSL;
 #else
+#ifdef COMMON_DIGEST_FOR_OPENSSL
+#include <CommonCrypto/CommonHMAC.h>
+#define HMAC_CTX CCHmacContext
+#define HMAC_Init(hmac, key, len, algo) CCHmacInit(hmac, algo, key, len)
+#define HMAC_Update CCHmacUpdate
+#define HMAC_Final(hmac, hash, ptr) CCHmacFinal(hmac, hash)
+#define HMAC_CTX_cleanup
+#define EVP_md5() kCCHmacAlgMD5
+#else
 #include <openssl/evp.h>
 #include <openssl/hmac.h>
+#endif
 #include <openssl/x509v3.h>
 #endif
 
-- 
1.8.3.rc2.2.gbc955d1

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
  2013-05-15  7:11 [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X David Aguilar
  2013-05-15  7:11 ` [PATCH v5 2/2] imap-send: eliminate HMAC " David Aguilar
@ 2013-05-15 17:56 ` Torsten Bögershausen
  2013-05-17  6:18   ` Eric Sunshine
  1 sibling, 1 reply; 13+ messages in thread
From: Torsten Bögershausen @ 2013-05-15 17:56 UTC (permalink / raw)
  To: David Aguilar; +Cc: Junio C Hamano, git, Jonathan Nieder, Eric Sunshine

On 2013-05-15 09.11, David Aguilar wrote:
> Mac OS X 10.8 Mountain Lion prints warnings when building git:
> 
> 	warning: 'SHA1_Init' is deprecated
> 	(declared at /usr/include/openssl/sha.h:121)
> 
> Silence the warnings by using the CommonCrytpo SHA-1
> functions for SHA1_Init(), SHA1_Update(), and SHA1_Final().
> 
> COMMON_DIGEST_FOR_OPENSSL is defined to enable the OpenSSL
> compatibility macros in CommonDigest.h.
> 
> Add a NO_APPLE_COMMON_CRYPTO option to the Makefile to allow
> users to opt out of using this library.  When defined, Git will
> use OpenSSL instead.
> 
> Helped-by: Eric Sunshine <sunshine@sunshineco.com>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> Both of these are replacement patches "pu".
> 
> Changes from last time:
> 
> It now uses a single APPLE_COMMON_CRYPTO definition.
> Users can now opt-out by setting NO_APPLE_COMMON_CRYPTO.
> 
>  Makefile | 13 +++++++++++++
>  1 file changed, 13 insertions(+)
> 
> diff --git a/Makefile b/Makefile
> index f698c1a..8309c41 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -137,6 +137,10 @@ all::
>  # specify your own (or DarwinPort's) include directories and
>  # library directories by defining CFLAGS and LDFLAGS appropriately.
>  #
> +# Define NO_APPLE_COMMON_CRYPTO if you are building on Darwin/Mac OS X
> +# and do not want to use Apple's CommonCrypto library.  This allows you
> +# to provide your own OpenSSL library, for example from MacPorts.
> +#
>  # Define BLK_SHA1 environment variable to make use of the bundled
>  # optimized C SHA1 routine.
>  #
> @@ -1054,6 +1058,9 @@ ifeq ($(uname_S),Darwin)
>  			BASIC_LDFLAGS += -L/opt/local/lib
>  		endif
>  	endif
> +	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

/Torsten

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
  2013-05-15 17:56 ` [PATCH v5 1/2] cache.h: eliminate SHA-1 " Torsten Bögershausen
@ 2013-05-17  6:18   ` Eric Sunshine
  2013-05-17  8:21     ` David Aguilar
  0 siblings, 1 reply; 13+ messages in thread
From: Eric Sunshine @ 2013-05-17  6:18 UTC (permalink / raw)
  To: Torsten Bögershausen
  Cc: David Aguilar, Junio C Hamano, Git List, Jonathan Nieder

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

-- ES

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
  2013-05-17  6:18   ` Eric Sunshine
@ 2013-05-17  8:21     ` David Aguilar
  2013-05-17 16:53       ` Junio C Hamano
  2013-05-17 17:40       ` Eric Sunshine
  0 siblings, 2 replies; 13+ messages in thread
From: David Aguilar @ 2013-05-17  8:21 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Torsten Bögershausen, Junio C Hamano, Git List, Jonathan Nieder

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.
>
> [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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
  2013-05-17  8:21     ` David Aguilar
@ 2013-05-17 16:53       ` Junio C Hamano
  2013-05-17 17:20         ` David Aguilar
  2013-05-17 17:29         ` Eric Sunshine
  2013-05-17 17:40       ` Eric Sunshine
  1 sibling, 2 replies; 13+ messages in thread
From: Junio C Hamano @ 2013-05-17 16:53 UTC (permalink / raw)
  To: David Aguilar
  Cc: Eric Sunshine, Torsten Bögershausen, Git List, Jonathan Nieder

David Aguilar <davvid@gmail.com> writes:

> 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".

I could do this easily:

    $ git checkout da/darwin ;# b72ac20a6f73b
    $ git format-patch --stdout -2 |
      sed -e 's|COMMON_DIGEST_FOR_OPENSSL|APPLE_COMMON_CRYPTO|g' >P.mbox
    $ git checkout HEAD^^ ;# 29de20504e
    $ git am P.mbox
    $ git diff da/darwin HEAD ;# sanity check
    $ git log da/darwin.. ;# sanity check
    $ git branch -f da/darwin

if you nicely ask ;-)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
  2013-05-17 16:53       ` Junio C Hamano
@ 2013-05-17 17:20         ` David Aguilar
  2013-05-17 17:29         ` Eric Sunshine
  1 sibling, 0 replies; 13+ messages in thread
From: David Aguilar @ 2013-05-17 17:20 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Eric Sunshine, Torsten Bögershausen, Git List, Jonathan Nieder

On Fri, May 17, 2013 at 9:53 AM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> 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".
>
> I could do this easily:
>
>     $ git checkout da/darwin ;# b72ac20a6f73b
>     $ git format-patch --stdout -2 |
>       sed -e 's|COMMON_DIGEST_FOR_OPENSSL|APPLE_COMMON_CRYPTO|g' >P.mbox
>     $ git checkout HEAD^^ ;# 29de20504e
>     $ git am P.mbox
>     $ git diff da/darwin HEAD ;# sanity check
>     $ git log da/darwin.. ;# sanity check
>     $ git branch -f da/darwin
>
> if you nicely ask ;-)

gitster please ;-)

--
David

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
  2013-05-17 16:53       ` Junio C Hamano
  2013-05-17 17:20         ` David Aguilar
@ 2013-05-17 17:29         ` Eric Sunshine
  2013-05-17 17:57           ` Junio C Hamano
  1 sibling, 1 reply; 13+ messages in thread
From: Eric Sunshine @ 2013-05-17 17:29 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: David Aguilar, Torsten Bögershausen, Git List, Jonathan Nieder

On Fri, May 17, 2013 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> 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".
>
> I could do this easily:
>
>     $ git checkout da/darwin ;# b72ac20a6f73b
>     $ git format-patch --stdout -2 |
>       sed -e 's|COMMON_DIGEST_FOR_OPENSSL|APPLE_COMMON_CRYPTO|g' >P.mbox
>     $ git checkout HEAD^^ ;# 29de20504e
>     $ git am P.mbox
>     $ git diff da/darwin HEAD ;# sanity check
>     $ git log da/darwin.. ;# sanity check
>     $ git branch -f da/darwin

That would lose the one legitimate COMMON_DIGEST_FOR_OPENSSL which
needs to be defined before <CommonCrypto/CommonDigest.h> is included.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
  2013-05-17  8:21     ` David Aguilar
  2013-05-17 16:53       ` Junio C Hamano
@ 2013-05-17 17:40       ` Eric Sunshine
  1 sibling, 0 replies; 13+ messages in thread
From: Eric Sunshine @ 2013-05-17 17:40 UTC (permalink / raw)
  To: David Aguilar
  Cc: Torsten Bögershausen, Junio C Hamano, Git List, Jonathan Nieder

On Fri, May 17, 2013 at 4:21 AM, David Aguilar <davvid@gmail.com> wrote:
> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
  2013-05-17 17:29         ` Eric Sunshine
@ 2013-05-17 17:57           ` Junio C Hamano
  2013-05-18  0:38             ` David Aguilar
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2013-05-17 17:57 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: David Aguilar, Torsten Bögershausen, Git List, Jonathan Nieder

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Fri, May 17, 2013 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> David Aguilar <davvid@gmail.com> writes:
>>
>>> 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".
>>
>> I could do this easily:
>>
>>     $ git checkout da/darwin ;# b72ac20a6f73b
>>     $ git format-patch --stdout -2 |
>>       sed -e 's|COMMON_DIGEST_FOR_OPENSSL|APPLE_COMMON_CRYPTO|g' >P.mbox
>>     $ git checkout HEAD^^ ;# 29de20504e
>>     $ git am P.mbox
>>     $ git diff da/darwin HEAD ;# sanity check
>>     $ git log da/darwin.. ;# sanity check
>>     $ git branch -f da/darwin
>
> That would lose the one legitimate COMMON_DIGEST_FOR_OPENSSL which
> needs to be defined before <CommonCrypto/CommonDigest.h> is included.

It probably is a good catch, but I'll stop reading patches and start
today's integration cycle (and will tag 1.8.3-rc3).

At this point, I think the best would be for you to reroll these two
patches, then after David reviews it and I'd re-queue the result
with

	... original commit message ...

        Reorginized use of Makefile variables and C preprosessor
        macros with help by Eric Sunshine.

	Signed-off-by: David
        Signed-off-by: Eric
        Signed-off-by: Me

or something.  Can you two help that?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
  2013-05-17 17:57           ` Junio C Hamano
@ 2013-05-18  0:38             ` David Aguilar
  2013-05-19  6:26               ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: David Aguilar @ 2013-05-18  0:38 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Eric Sunshine, Torsten Bögershausen, Git List, Jonathan Nieder

On Fri, May 17, 2013 at 10:57 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> On Fri, May 17, 2013 at 12:53 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> David Aguilar <davvid@gmail.com> writes:
>>>
>>>> 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".
>>>
>>> I could do this easily:
>>>
>>>     $ git checkout da/darwin ;# b72ac20a6f73b
>>>     $ git format-patch --stdout -2 |
>>>       sed -e 's|COMMON_DIGEST_FOR_OPENSSL|APPLE_COMMON_CRYPTO|g' >P.mbox
>>>     $ git checkout HEAD^^ ;# 29de20504e
>>>     $ git am P.mbox
>>>     $ git diff da/darwin HEAD ;# sanity check
>>>     $ git log da/darwin.. ;# sanity check
>>>     $ git branch -f da/darwin
>>
>> That would lose the one legitimate COMMON_DIGEST_FOR_OPENSSL which
>> needs to be defined before <CommonCrypto/CommonDigest.h> is included.
>
> It probably is a good catch, but I'll stop reading patches and start
> today's integration cycle (and will tag 1.8.3-rc3).
>
> At this point, I think the best would be for you to reroll these two
> patches, then after David reviews it and I'd re-queue the result
> with
>
>         ... original commit message ...
>
>         Reorginized use of Makefile variables and C preprosessor
>         macros with help by Eric Sunshine.
>
>         Signed-off-by: David
>         Signed-off-by: Eric
>         Signed-off-by: Me
>
> or something.  Can you two help that?

Thanks Eric and Junio.  I looked over the patches and they look good.

Signed-off-by: David Aguilar <davvid@gmail.com>

Cheers,
--
David

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
  2013-05-18  0:38             ` David Aguilar
@ 2013-05-19  6:26               ` Junio C Hamano
  2013-05-19 21:51                 ` David Aguilar
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2013-05-19  6:26 UTC (permalink / raw)
  To: David Aguilar
  Cc: Eric Sunshine, Torsten Bögershausen, Git List, Jonathan Nieder

David Aguilar <davvid@gmail.com> writes:

> Thanks Eric and Junio.  I looked over the patches and they look good.

Are you sure about that?  It seemed to me that it was breaking
everybody that is not on MacOS X --- did I misread the patch?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X
  2013-05-19  6:26               ` Junio C Hamano
@ 2013-05-19 21:51                 ` David Aguilar
  0 siblings, 0 replies; 13+ messages in thread
From: David Aguilar @ 2013-05-19 21:51 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Eric Sunshine, Torsten Bögershausen, Git List, Jonathan Nieder

On Sat, May 18, 2013 at 11:26 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> Thanks Eric and Junio.  I looked over the patches and they look good.
>
> Are you sure about that?  It seemed to me that it was breaking
> everybody that is not on MacOS X --- did I misread the patch?

Gah, correct.  I've now tested v8 which Eric just sent out.  It worked
fine for me, with and without NO_APPLE_COMMON_CRYPTO.

Thanks,
--
David

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2013-05-19 21:51 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-05-15  7:11 [PATCH v5 1/2] cache.h: eliminate SHA-1 deprecation warnings on Mac OS X David Aguilar
2013-05-15  7:11 ` [PATCH v5 2/2] imap-send: eliminate HMAC " David Aguilar
2013-05-15 17:56 ` [PATCH v5 1/2] cache.h: eliminate SHA-1 " Torsten Bögershausen
2013-05-17  6:18   ` Eric Sunshine
2013-05-17  8:21     ` David Aguilar
2013-05-17 16:53       ` Junio C Hamano
2013-05-17 17:20         ` David Aguilar
2013-05-17 17:29         ` Eric Sunshine
2013-05-17 17:57           ` Junio C Hamano
2013-05-18  0:38             ` David Aguilar
2013-05-19  6:26               ` Junio C Hamano
2013-05-19 21:51                 ` David Aguilar
2013-05-17 17:40       ` Eric Sunshine

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.