[PATCH 0/3] Fixes for OS X

[PATCH 0/3] Fixes for OS X

[Brian Gernhardt]

A few changes recently broke my build on Mac 10.8, possibly because I have a
more strict set of warnings/errors enabled.  The first two handle minor
problems with the use of APPLE_COMMON_CRYPTO, which was expanded for use in
imap-send but had a couple of problems.

The last is likely due to curl version skew between Dave Borowitz and myself.
(see 912b2ac).

There are a few notes on the patches indicating where I was less than sure
about my solutions.

Brian Gernhardt (3):
  Makefile: Fix APPLE_COMMON_CRYPTO with BLK_SHA1
  OS X: Fix redeclaration of die warning
  t5551: Remove header from curl cookie file

 Makefile              |  4 +++-
 git-compat-util.h     | 20 ++++++++++----------
 t/t5551-http-fetch.sh |  6 ++----
 3 files changed, 15 insertions(+), 15 deletions(-)

-- 
1.8.4.rc1.384.g0976a17.dirty

[PATCH 1/3] Makefile: Fix APPLE_COMMON_CRYPTO with BLK_SHA1

[Brian Gernhardt]

It used to be that APPLE_COMMON_CRYPTO did nothing when BLK_SHA1 was
set.  But APPLE_COMMON_CRYPTO is now used for more than just SHA1 (see
3ef2bca) so make sure that the appropriate libraries are always set.

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---
 Makefile | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 82f2e22..7051956 100644
--- a/Makefile
+++ b/Makefile
@@ -1182,6 +1182,9 @@ ifdef NEEDS_SSL_WITH_CRYPTO
 else
 	LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto
 endif
+ifdef APPLE_COMMON_CRYPTO
+	LIB_4_CRYPTO += -framework Security -framework CoreFoundation
+endif
 endif
 ifdef NEEDS_LIBICONV
 	ifdef ICONVDIR
@@ -1413,7 +1416,6 @@ ifdef PPC_SHA1
 	LIB_H += ppc/sha1.h
 else
 ifdef APPLE_COMMON_CRYPTO
-	LIB_4_CRYPTO += -framework Security -framework CoreFoundation
 	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
 	SHA1_HEADER = <CommonCrypto/CommonDigest.h>
 else
-- 
1.8.4.rc1.384.g0976a17.dirty

Re: [PATCH 1/3] Makefile: Fix APPLE_COMMON_CRYPTO with BLK_SHA1

[Jeremy Huddleston Sequoia]

[-- Attachment #1: Type: text/plain, Size: 1184 bytes --]

Thanks Brian,

Reviewed-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>

On Aug 5, 2013, at 8:59, Brian Gernhardt <brian@gernhardtsoftware.com> wrote:

> It used to be that APPLE_COMMON_CRYPTO did nothing when BLK_SHA1 was
> set.  But APPLE_COMMON_CRYPTO is now used for more than just SHA1 (see
> 3ef2bca) so make sure that the appropriate libraries are always set.
> 
> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
> ---
> Makefile | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 82f2e22..7051956 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1182,6 +1182,9 @@ ifdef NEEDS_SSL_WITH_CRYPTO
> else
> 	LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto
> endif
> +ifdef APPLE_COMMON_CRYPTO
> +	LIB_4_CRYPTO += -framework Security -framework CoreFoundation
> +endif
> endif
> ifdef NEEDS_LIBICONV
> 	ifdef ICONVDIR
> @@ -1413,7 +1416,6 @@ ifdef PPC_SHA1
> 	LIB_H += ppc/sha1.h
> else
> ifdef APPLE_COMMON_CRYPTO
> -	LIB_4_CRYPTO += -framework Security -framework CoreFoundation
> 	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
> 	SHA1_HEADER = <CommonCrypto/CommonDigest.h>
> else
> -- 
> 1.8.4.rc1.384.g0976a17.dirty
> 


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4136 bytes --]

Re: [PATCH 1/3] Makefile: Fix APPLE_COMMON_CRYPTO with BLK_SHA1

[Junio C Hamano]

Brian Gernhardt <brian@gernhardtsoftware.com> writes:

> It used to be that APPLE_COMMON_CRYPTO did nothing when BLK_SHA1 was
> set.  But APPLE_COMMON_CRYPTO is now used for more than just SHA1 (see
> 3ef2bca) so make sure that the appropriate libraries are always set.
>
> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
> ---
>  Makefile | 4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Makefile b/Makefile
> index 82f2e22..7051956 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1182,6 +1182,9 @@ ifdef NEEDS_SSL_WITH_CRYPTO
>  else
>  	LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto
>  endif
> +ifdef APPLE_COMMON_CRYPTO
> +	LIB_4_CRYPTO += -framework Security -framework CoreFoundation
> +endif
>  endif
>  ifdef NEEDS_LIBICONV
>  	ifdef ICONVDIR
> @@ -1413,7 +1416,6 @@ ifdef PPC_SHA1
>  	LIB_H += ppc/sha1.h
>  else
>  ifdef APPLE_COMMON_CRYPTO
> -	LIB_4_CRYPTO += -framework Security -framework CoreFoundation
>  	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
>  	SHA1_HEADER = <CommonCrypto/CommonDigest.h>
>  else

Hmph.

So the people previously tested this must have built imap-send
without blk-sha1, which not just linked with these libs but also
included the <CommonCrypto/CommonDigest.h> header file and defined
the -DCOMMON_DIGEST_FOR_OPENSSL preprocessor macro.  Building with
blk-sha1 would not have worked for them.

Now we always link with these libraries, even when building with
blk-sha1.  Do the COMPAT_CFLAGS and SHA1_HEADER pieces only needed
when using the SHA1 digest implementation from CommonCrypto and
nothing imap-send uses?

Re: [PATCH 1/3] Makefile: Fix APPLE_COMMON_CRYPTO with BLK_SHA1

[David Aguilar]

On Mon, Aug 5, 2013 at 10:52 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Brian Gernhardt <brian@gernhardtsoftware.com> writes:
>
>> It used to be that APPLE_COMMON_CRYPTO did nothing when BLK_SHA1 was
>> set.  But APPLE_COMMON_CRYPTO is now used for more than just SHA1 (see
>> 3ef2bca) so make sure that the appropriate libraries are always set.
>>
>> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
>> ---
>>  Makefile | 4 +++-
>>  1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Makefile b/Makefile
>> index 82f2e22..7051956 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -1182,6 +1182,9 @@ ifdef NEEDS_SSL_WITH_CRYPTO
>>  else
>>       LIB_4_CRYPTO = $(OPENSSL_LINK) -lcrypto
>>  endif
>> +ifdef APPLE_COMMON_CRYPTO
>> +     LIB_4_CRYPTO += -framework Security -framework CoreFoundation
>> +endif
>>  endif
>>  ifdef NEEDS_LIBICONV
>>       ifdef ICONVDIR
>> @@ -1413,7 +1416,6 @@ ifdef PPC_SHA1
>>       LIB_H += ppc/sha1.h
>>  else
>>  ifdef APPLE_COMMON_CRYPTO
>> -     LIB_4_CRYPTO += -framework Security -framework CoreFoundation
>>       COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
>>       SHA1_HEADER = <CommonCrypto/CommonDigest.h>
>>  else
>
> Hmph.
>
> So the people previously tested this must have built imap-send
> without blk-sha1, which not just linked with these libs but also
> included the <CommonCrypto/CommonDigest.h> header file and defined
> the -DCOMMON_DIGEST_FOR_OPENSSL preprocessor macro.  Building with
> blk-sha1 would not have worked for them.
> Now we always link with these libraries, even when building with
> blk-sha1.  Do the COMPAT_CFLAGS and SHA1_HEADER pieces only needed
> when using the SHA1 digest implementation from CommonCrypto and
> nothing imap-send uses?

LIB_4_CRYPTO is used by imap-send only, and these libraries are needed
for the base64 git_CC_EVP_(Encode|Decode), so unconditionally adding
these libraries there is correct.

COMPAT_CFLAGS and SHA1_HEADER enable the common crypto SHA1 only.
BLK_SHA1 provides its own SHA1 so they're not needed there.

I tested the tip of da/darwin (pu) w/ and w/out BLK_SHA1.

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

Re: [PATCH 1/3] Makefile: Fix APPLE_COMMON_CRYPTO with BLK_SHA1

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> I tested the tip of da/darwin (pu) w/ and w/out BLK_SHA1.
>
> Tested-by: David Aguilar <davvid@gmail.com>

Thanks.

[PATCH 2/3] OS X: Fix redeclaration of die warning

[Brian Gernhardt]

compat/apple-common-crypto.h uses die() in one of its macros, but was
included in git-compat-util.h before the definition of die.

Fix by simply moving the relevant block after the die/error/warning
declarations.

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---

 Not sure if this is the best place to move it to, but it's the earliest it can
 be in the file without causing errors.  (Namely that clang has to guess what
 die() means in apple-common-crypto.h and guesses differently than the actual
 definition.)

 git-compat-util.h | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index af5f6bb..d60e28d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -129,16 +129,6 @@
 #include <poll.h>
 #endif
 
-#ifndef NO_OPENSSL
-#ifdef APPLE_COMMON_CRYPTO
-#include "compat/apple-common-crypto.h"
-#else
-#include <openssl/evp.h>
-#include <openssl/hmac.h>
-#endif /* APPLE_COMMON_CRYPTO */
-#include <openssl/x509v3.h>
-#endif /* NO_OPENSSL */
-
 #if defined(__MINGW32__)
 /* pull in Windows compatibility stuff */
 #include "compat/mingw.h"
@@ -340,6 +330,16 @@ extern NORETURN void die_errno(const char *err, ...) __attribute__((format (prin
 extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
 extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
+#ifndef NO_OPENSSL
+#ifdef APPLE_COMMON_CRYPTO
+#include "compat/apple-common-crypto.h"
+#else
+#include <openssl/evp.h>
+#include <openssl/hmac.h>
+#endif /* APPLE_COMMON_CRYPTO */
+#include <openssl/x509v3.h>
+#endif /* NO_OPENSSL */
+
 /*
  * Let callers be aware of the constant return value; this can help
  * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,
-- 
1.8.4.rc1.384.g0976a17.dirty

Re: [PATCH 2/3] OS X: Fix redeclaration of die warning

[Jeremy Huddleston Sequoia]

[-- Attachment #1: Type: text/plain, Size: 2152 bytes --]

Thanks Brian,

Reviewed-by: Jeremy Huddleston Sequoia <jeremyhu@apple.com>

On Aug 5, 2013, at 8:59, Brian Gernhardt <brian@gernhardtsoftware.com> wrote:

> compat/apple-common-crypto.h uses die() in one of its macros, but was
> included in git-compat-util.h before the definition of die.
> 
> Fix by simply moving the relevant block after the die/error/warning
> declarations.
> 
> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
> ---
> 
> Not sure if this is the best place to move it to, but it's the earliest it can
> be in the file without causing errors.  (Namely that clang has to guess what
> die() means in apple-common-crypto.h and guesses differently than the actual
> definition.)
> 
> git-compat-util.h | 20 ++++++++++----------
> 1 file changed, 10 insertions(+), 10 deletions(-)
> 
> diff --git a/git-compat-util.h b/git-compat-util.h
> index af5f6bb..d60e28d 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -129,16 +129,6 @@
> #include <poll.h>
> #endif
> 
> -#ifndef NO_OPENSSL
> -#ifdef APPLE_COMMON_CRYPTO
> -#include "compat/apple-common-crypto.h"
> -#else
> -#include <openssl/evp.h>
> -#include <openssl/hmac.h>
> -#endif /* APPLE_COMMON_CRYPTO */
> -#include <openssl/x509v3.h>
> -#endif /* NO_OPENSSL */
> -
> #if defined(__MINGW32__)
> /* pull in Windows compatibility stuff */
> #include "compat/mingw.h"
> @@ -340,6 +330,16 @@ extern NORETURN void die_errno(const char *err, ...) __attribute__((format (prin
> extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
> extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
> 
> +#ifndef NO_OPENSSL
> +#ifdef APPLE_COMMON_CRYPTO
> +#include "compat/apple-common-crypto.h"
> +#else
> +#include <openssl/evp.h>
> +#include <openssl/hmac.h>
> +#endif /* APPLE_COMMON_CRYPTO */
> +#include <openssl/x509v3.h>
> +#endif /* NO_OPENSSL */
> +
> /*
>  * Let callers be aware of the constant return value; this can help
>  * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,
> -- 
> 1.8.4.rc1.384.g0976a17.dirty
> 


[-- Attachment #2: smime.p7s --]
[-- Type: application/pkcs7-signature, Size: 4136 bytes --]

Re: [PATCH 2/3] OS X: Fix redeclaration of die warning

[Junio C Hamano]

Brian Gernhardt <brian@gernhardtsoftware.com> writes:

> compat/apple-common-crypto.h uses die() in one of its macros, but was
> included in git-compat-util.h before the definition of die.
>
> Fix by simply moving the relevant block after the die/error/warning
> declarations.

Puzzled.  What needs fixing???

Ahh, that one is not just making #define macros, but defining static
inline functions.

I wonder if they need to be static inlines to be duplicated at each
call sites in the first place.  Wouldn't it be better to create a
compat/something.c file to be linked with?

> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
> ---
>
>  Not sure if this is the best place to move it to, but it's the earliest it can
>  be in the file without causing errors.  (Namely that clang has to guess what
>  die() means in apple-common-crypto.h and guesses differently than the actual
>  definition.)
>
>  git-compat-util.h | 20 ++++++++++----------
>  1 file changed, 10 insertions(+), 10 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index af5f6bb..d60e28d 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -129,16 +129,6 @@
>  #include <poll.h>
>  #endif
>  
> -#ifndef NO_OPENSSL
> -#ifdef APPLE_COMMON_CRYPTO
> -#include "compat/apple-common-crypto.h"
> -#else
> -#include <openssl/evp.h>
> -#include <openssl/hmac.h>
> -#endif /* APPLE_COMMON_CRYPTO */
> -#include <openssl/x509v3.h>
> -#endif /* NO_OPENSSL */
> -
>  #if defined(__MINGW32__)
>  /* pull in Windows compatibility stuff */
>  #include "compat/mingw.h"
> @@ -340,6 +330,16 @@ extern NORETURN void die_errno(const char *err, ...) __attribute__((format (prin
>  extern int error(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  extern void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
>  
> +#ifndef NO_OPENSSL
> +#ifdef APPLE_COMMON_CRYPTO
> +#include "compat/apple-common-crypto.h"
> +#else
> +#include <openssl/evp.h>
> +#include <openssl/hmac.h>
> +#endif /* APPLE_COMMON_CRYPTO */
> +#include <openssl/x509v3.h>
> +#endif /* NO_OPENSSL */
> +
>  /*
>   * Let callers be aware of the constant return value; this can help
>   * gcc with -Wuninitialized analysis. We restrict this trick to gcc, though,

Re: [PATCH 2/3] OS X: Fix redeclaration of die warning

[David Aguilar]

On Mon, Aug 5, 2013 at 11:00 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Brian Gernhardt <brian@gernhardtsoftware.com> writes:
>
>> compat/apple-common-crypto.h uses die() in one of its macros, but was
>> included in git-compat-util.h before the definition of die.
>>
>> Fix by simply moving the relevant block after the die/error/warning
>> declarations.
>
> Puzzled.  What needs fixing???
>
> Ahh, that one is not just making #define macros, but defining static
> inline functions.
>
> I wonder if they need to be static inlines to be duplicated at each
> call sites in the first place.  Wouldn't it be better to create a
> compat/something.c file to be linked with?

IMO it's not worth it right now because there's only a single call
site (imap-send).  The moment another call site is introduced then
compat/apple-common-crypto.c would be the natural home for it.
-- 
David

[PATCH 3/3] t5551: Remove header from curl cookie file

[Brian Gernhardt]

The URL included in the header appears to vary from curl version to
curl version.  Since we only care about the final few lines, only test
them.  However, make sure the blank line after the header is still
included to make sure there are no extra cookie lines.

Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
---

 I suppose a sed invocation to strip out the URL or comments might be better,
 but this seemed simpler.

 t/t5551-http-fetch.sh | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 287d22b..8196af1 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -191,9 +191,6 @@ cat >cookies.txt <<EOF
 127.0.0.1	FALSE	/smart_cookies/	FALSE	0	othername	othervalue
 EOF
 cat >expect_cookies.txt <<EOF
-# Netscape HTTP Cookie File
-# http://curl.haxx.se/docs/http-cookies.html
-# This file was generated by libcurl! Edit at your own risk.
 
 127.0.0.1	FALSE	/smart_cookies/	FALSE	0	othername	othervalue
 127.0.0.1	FALSE	/smart_cookies/repo.git/info/	FALSE	0	name	value
@@ -202,7 +199,8 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set
 	git config http.cookiefile cookies.txt &&
 	git config http.savecookies true &&
 	git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
-	test_cmp expect_cookies.txt cookies.txt
+	tail -3 cookies.txt > cookies_tail.txt
+	test_cmp expect_cookies.txt cookies_tail.txt
 '
 
 test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
-- 
1.8.4.rc1.384.g0976a17.dirty

Re: [PATCH 3/3] t5551: Remove header from curl cookie file

[Dave Borowitz]

On Mon, Aug 5, 2013 at 8:59 AM, Brian Gernhardt
<brian@gernhardtsoftware.com> wrote:
>
> The URL included in the header appears to vary from curl version to
> curl version.  Since we only care about the final few lines, only test
> them.  However, make sure the blank line after the header is still
> included to make sure there are no extra cookie lines.
>
> Signed-off-by: Brian Gernhardt <brian@gernhardtsoftware.com>
> ---
>
>  I suppose a sed invocation to strip out the URL or comments might be better,
>  but this seemed simpler.
>
>  t/t5551-http-fetch.sh | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
> index 287d22b..8196af1 100755
> --- a/t/t5551-http-fetch.sh
> +++ b/t/t5551-http-fetch.sh
> @@ -191,9 +191,6 @@ cat >cookies.txt <<EOF
>  127.0.0.1      FALSE   /smart_cookies/ FALSE   0       othername       othervalue
>  EOF
>  cat >expect_cookies.txt <<EOF
> -# Netscape HTTP Cookie File
> -# http://curl.haxx.se/docs/http-cookies.html
> -# This file was generated by libcurl! Edit at your own risk.
>
>  127.0.0.1      FALSE   /smart_cookies/ FALSE   0       othername       othervalue
>  127.0.0.1      FALSE   /smart_cookies/repo.git/info/   FALSE   0       name    value
> @@ -202,7 +199,8 @@ test_expect_success 'cookies stored in http.cookiefile when http.savecookies set
>         git config http.cookiefile cookies.txt &&
>         git config http.savecookies true &&
>         git ls-remote $HTTPD_URL/smart_cookies/repo.git master &&
> -       test_cmp expect_cookies.txt cookies.txt
> +       tail -3 cookies.txt > cookies_tail.txt

Would it make more sense to ignore comments entirely? I.e. instead of
taking the tail, pipe through sed 's/#.*//'.

Thanks for catching this by the way; you can probably guess that I
only checked with a single curl version.

>
> +       test_cmp expect_cookies.txt cookies_tail.txt
>  '
>
>  test -n "$GIT_TEST_LONG" && test_set_prereq EXPENSIVE
> --
> 1.8.4.rc1.384.g0976a17.dirty
>