{"thread":{"id":"33783","subject":"[PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8","startedAt":"2013-05-11T02:44:42Z","lastAt":"2013-05-11T09:50:36Z","messageCount":8,"participants":["David Aguilar","Jonathan Nieder","Jeff King"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"216958","messageId":"1368240282-89581-1-git-send-email-davvid@gmail.com","threadId":"33783","inReplyTo":null,"subject":"[PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-05-11T02:44:42Z","receivedAt":"2013-05-11T02:44:42Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Mac OS X Mountain Lion prints warnings when building git:\n\n\twarning: 'SHA1_Init' is deprecated\n\t(declared at /usr/include/openssl/sha.h:121)\n\nSilence the warnings by using the Common Digest SHA-1\nfunctions for SHA1_Init(), SHA1_Update(), and SHA1_Final().\n\nAdd a COMMON_DIGEST_SHA1 knob to the Makefile to allow\nchoosing this implementation and define it by default on Darwin.\n\nSigned-off-by: David Aguilar \n---\nThe previous version of this patch (somehow?) eliminated the SHA1\nwarnings, but it was not redefining SHA1_HEADER to CommonDigest.h.\nThis version goes all the way.\n\nSome warnings still exist around the HMAC functions (and others)\nused by e.g. imap.c, which should be dealt with separately.\n\n Makefile | 7 +++++++\n cache.h | 7 +++++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/Makefile b/Makefile\nindex 0f931a2..d8a45b4 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1054,6 +1054,7 @@ ifeq ($(uname_S),Darwin)\n \t\t\tBASIC_LDFLAGS += -L/opt/local/lib\n \t\tendif\n \tendif\n+\tCOMMON_DIGEST_SHA1 = YesPlease\n \tPTHREAD_LIBS =\n endif\n \n@@ -1388,10 +1389,16 @@ ifdef PPC_SHA1\n \tLIB_OBJS += ppc/sha1.o ppc/sha1ppc.o\n \tLIB_H += ppc/sha1.h\n else\n+ifdef COMMON_DIGEST_SHA1\n+\tBASIC_CFLAGS += -DCOMMON_DIGEST_FOR_SHA1=1\n+\tSHA1_HEADER = \n+\tEXTLIBS += $(LIB_4_CRYPTO)\n+else\n \tSHA1_HEADER = \n \tEXTLIBS += $(LIB_4_CRYPTO)\n endif\n endif\n+endif\n ifdef NO_PERL_MAKEMAKER\n \texport NO_PERL_MAKEMAKER\n endif\ndiff --git a/cache.h b/cache.h\nindex 94ca1ac..e2b24c6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -10,11 +10,18 @@\n \n #include SHA1_HEADER\n #ifndef git_SHA_CTX\n+#ifdef COMMON_DIGEST_FOR_SHA1\n+#define git_SHA_CTX\tCC_SHA1_CTX\n+#define git_SHA1_Init\tCC_SHA1_Init\n+#define git_SHA1_Update\tCC_SHA1_Update\n+#define git_SHA1_Final\tCC_SHA1_Final\n+#else\n #define git_SHA_CTX\tSHA_CTX\n #define git_SHA1_Init\tSHA1_Init\n #define git_SHA1_Update\tSHA1_Update\n #define git_SHA1_Final\tSHA1_Final\n #endif\n+#endif\n \n #include \n typedef struct git_zstream {\n-- \n1.8.3.rc1.45.g88f80b8.dirty\n"},{"id":"216964","messageId":"20130511062336.GD3394@elie","threadId":"33783","inReplyTo":"1368240282-89581-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-05-11T06:23:36Z","receivedAt":"2013-05-11T06:23:36Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nDavid Aguilar wrote:\n\n> Mac OS X Mountain Lion prints warnings when building git:\n>\n> \twarning: 'SHA1_Init' is deprecated\n> \t(declared at /usr/include/openssl/sha.h:121)\n>\n> Silence the warnings by using the Common Digest SHA-1\n> functions for SHA1_Init(), SHA1_Update(), and SHA1_Final().\n\nThanks.\n\nDoes this perform better or worse than just setting\nBLK_SHA1=YesPlease? I'd naively think it could go either way: on one\nhand adding another library dependency can slow down startup, and on\nthe other hand the implementation may or may not be optimized better\nthan the generic block-sha1/ implementation.\n\nCurious,\nJonathan\n"},{"id":"216969","messageId":"CAJDDKr6QkeSk32kNhU_QuvrZSGUEokcEbN7Aq3PcOhvaA_hFgg@mail.gmail.com","threadId":"33783","inReplyTo":"20130511062336.GD3394@elie","subject":"Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-05-11T07:11:05Z","receivedAt":"2013-05-11T07:11:05Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, May 10, 2013 at 11:23 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> David Aguilar wrote:\n>\n>> Mac OS X Mountain Lion prints warnings when building git:\n>>\n>> warning: 'SHA1_Init' is deprecated\n>> (declared at /usr/include/openssl/sha.h:121)\n>>\n>> Silence the warnings by using the Common Digest SHA-1\n>> functions for SHA1_Init(), SHA1_Update(), and SHA1_Final().\n>\n> Thanks.\n>\n> Does this perform better or worse than just setting\n> BLK_SHA1=YesPlease? I'd naively think it could go either way: on one\n> hand adding another library dependency can slow down startup, and on\n> the other hand the implementation may or may not be optimized better\n> than the generic block-sha1/ implementation.\n\nPretty much identical.\n\nHere are the timings (I should probably read t/perf/README and get\nbetter numbers):\n\nBest of ten\n$ time git rev-list --all --objects >/dev/null\n\n# master\ngit rev-list --all --objects > /dev/null 5.16s user 0.11s system 99%\ncpu 5.277 total\n\n# BLK_SHA1\ngit rev-list --all --objects > /dev/null 5.17s user 0.11s system 99%\ncpu 5.299 total\n\n# CommonDigest\ngit rev-list --all --objects > /dev/null 5.18s user 0.11s system 99%\ncpu 5.301 total\n\nThe library startup cost is identical to master since at the end of\nthe day it's still libcrypto.\n\nFWIW the way it's done in this patch still allows defining BLK_SHA1,\nwhich is parity with how the Makefile behaves on Linux.\n\nRe-roll?\n--\nDavid\n"},{"id":"216972","messageId":"20130511082203.GA953@sigill.intra.peff.net","threadId":"33783","inReplyTo":"CAJDDKr6QkeSk32kNhU_QuvrZSGUEokcEbN7Aq3PcOhvaA_hFgg@mail.gmail.com","subject":"Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-11T08:22:03Z","receivedAt":"2013-05-11T08:22:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 11, 2013 at 12:11:05AM -0700, David Aguilar wrote:\n\n> > Does this perform better or worse than just setting\n> > BLK_SHA1=YesPlease? I'd naively think it could go either way: on one\n> > hand adding another library dependency can slow down startup, and on\n> > the other hand the implementation may or may not be optimized better\n> > than the generic block-sha1/ implementation.\n> \n> Pretty much identical.\n> \n> Here are the timings (I should probably read t/perf/README and get\n> better numbers):\n> \n> Best of ten\n> $ time git rev-list --all --objects >/dev/null\n> [...]\n\nI'm not sure that's a great test of sha1 performance. It will hash the\ncommit and tree objects it loads during the traversal, but that time is\nalmost certainly dwarfed by zlib inflation and by lookup_object.\n\nAdding \"--verify-objects\" would sha1 the blobs, too, which might be more\nreasonable (or running \"git fsck\"). Something like \"git add\" on a large\nblob would also be a good test.\n\n-Peff\n"},{"id":"216976","messageId":"CAJDDKr7-sMrX=2Aek1LY8knM2jFPs933eLj3iZdTa68YYv-6Og@mail.gmail.com","threadId":"33783","inReplyTo":"20130511082203.GA953@sigill.intra.peff.net","subject":"Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-05-11T08:38:32Z","receivedAt":"2013-05-11T08:38:32Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sat, May 11, 2013 at 1:22 AM, Jeff King wrote:\n> On Sat, May 11, 2013 at 12:11:05AM -0700, David Aguilar wrote:\n>\n>> > Does this perform better or worse than just setting\n>> > BLK_SHA1=YesPlease? I'd naively think it could go either way: on one\n>> > hand adding another library dependency can slow down startup, and on\n>> > the other hand the implementation may or may not be optimized better\n>> > than the generic block-sha1/ implementation.\n>>\n>> Pretty much identical.\n>>\n>> Here are the timings (I should probably read t/perf/README and get\n>> better numbers):\n>>\n>> Best of ten\n>> $ time git rev-list --all --objects >/dev/null\n>> [...]\n>\n> I'm not sure that's a great test of sha1 performance. It will hash the\n> commit and tree objects it loads during the traversal, but that time is\n> almost certainly dwarfed by zlib inflation and by lookup_object.\n>\n> Adding \"--verify-objects\" would sha1 the blobs, too, which might be more\n> reasonable (or running \"git fsck\"). Something like \"git add\" on a large\n> blob would also be a good test.\n\nThanks. Here are the numbers with --verify-objects:\n\n$ time git rev-list --all --objects --verify-objects >/dev/null\n\n# CommonCrypto 32.24s user 4.65s system 99% cpu 37.098 total\n# master 33.00s user 4.68s system 99% cpu 37.852 total\n# BLK_SHA1 54.17s user 4.67s system 99% cpu 58.928 total\n\nDoing BLK_SHA1 seems like less of a good idea now, so I think my\nlatest re-roll might be the way to go...\n--\nDavid\n"},{"id":"216977","messageId":"20130511084543.GA1905@sigill.intra.peff.net","threadId":"33783","inReplyTo":"CAJDDKr7-sMrX=2Aek1LY8knM2jFPs933eLj3iZdTa68YYv-6Og@mail.gmail.com","subject":"Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-11T08:45:43Z","receivedAt":"2013-05-11T08:45:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 11, 2013 at 01:38:32AM -0700, David Aguilar wrote:\n\n> > Adding \"--verify-objects\" would sha1 the blobs, too, which might be more\n> > reasonable (or running \"git fsck\"). Something like \"git add\" on a large\n> > blob would also be a good test.\n> \n> Thanks. Here are the numbers with --verify-objects:\n> \n> $ time git rev-list --all --objects --verify-objects >/dev/null\n> \n> # CommonCrypto 32.24s user 4.65s system 99% cpu 37.098 total\n> # master 33.00s user 4.68s system 99% cpu 37.852 total\n> # BLK_SHA1 54.17s user 4.67s system 99% cpu 58.928 total\n> \n> Doing BLK_SHA1 seems like less of a good idea now, so I think my\n> latest re-roll might be the way to go...\n\nWow, that's really terrible. What level of optimization do you compile\nwith? With the other implementations, you are calling into\n(presumably) optimized library code, but with BLK_SHA1 you are getting\nwhatever you just compiled.\n\nHere are three timings that show how big a difference that can make:\n\n openssl, -O0 0m21.152s\n BLK_SHA1, -O0 0m31.920s\n BLK_SHA1, -O3 0m19.652s\n\nSo it is a win over openssl with optimizations on, but a pretty big loss\notherwise.\n\n-Peff\n"},{"id":"216979","messageId":"CAJDDKr4DoEUw=4O_m-91Hsg4SRZzkn6yJPf_nBsPsEv0Weibrw@mail.gmail.com","threadId":"33783","inReplyTo":"20130511084543.GA1905@sigill.intra.peff.net","subject":"Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-05-11T09:17:10Z","receivedAt":"2013-05-11T09:17:10Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sat, May 11, 2013 at 1:45 AM, Jeff King wrote:\n> On Sat, May 11, 2013 at 01:38:32AM -0700, David Aguilar wrote:\n>\n>> > Adding \"--verify-objects\" would sha1 the blobs, too, which might be more\n>> > reasonable (or running \"git fsck\"). Something like \"git add\" on a large\n>> > blob would also be a good test.\n>>\n>> Thanks. Here are the numbers with --verify-objects:\n>>\n>> $ time git rev-list --all --objects --verify-objects >/dev/null\n>>\n>> # CommonCrypto 32.24s user 4.65s system 99% cpu 37.098 total\n>> # master 33.00s user 4.68s system 99% cpu 37.852 total\n>> # BLK_SHA1 54.17s user 4.67s system 99% cpu 58.928 total\n>>\n>> Doing BLK_SHA1 seems like less of a good idea now, so I think my\n>> latest re-roll might be the way to go...\n>\n> Wow, that's really terrible. What level of optimization do you compile\n> with? With the other implementations, you are calling into\n> (presumably) optimized library code, but with BLK_SHA1 you are getting\n> whatever you just compiled.\n>\n> Here are three timings that show how big a difference that can make:\n>\n> openssl, -O0 0m21.152s\n> BLK_SHA1, -O0 0m31.920s\n> BLK_SHA1, -O3 0m19.652s\n>\n> So it is a win over openssl with optimizations on, but a pretty big loss\n> otherwise.\n\nGood catch. I had a config.mak without any -O flags in CFLAGS.\nHere are the timings with -O3. We're back to parity.\n\n$ time git rev-list --all --objects --verify-objects >/dev/null\n\n# CommonCrypto 28.95s user 4.62s system 99% cpu 33.630 total\n# master 29.81s user 4.70s system 99% cpu 34.760 total\n# BLK_SHA1 29.80s user 4.62s system 99% cpu 34.505 total\n\nIf BLK_SHA1 were the default on all platforms then I wouldn't have\nbothered with the SHA-1 patch. With this patch it makes it like Linux\nin that Git can choose between the built-in functions and the external\nlibrary.\n\nThat's why I moved this patch to 3/3.. it could go either way.\n--\nDavid\n"},{"id":"216983","messageId":"20130511095035.GA17326@sigill.intra.peff.net","threadId":"33783","inReplyTo":"CAJDDKr4DoEUw=4O_m-91Hsg4SRZzkn6yJPf_nBsPsEv0Weibrw@mail.gmail.com","subject":"Re: [PATCH v2] cache.h: eliminate SHA-1 deprecation warnings on OS X 10.8","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-05-11T09:50:36Z","receivedAt":"2013-05-11T09:50:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 11, 2013 at 02:17:10AM -0700, David Aguilar wrote:\n\n> Good catch. I had a config.mak without any -O flags in CFLAGS.\n> Here are the timings with -O3. We're back to parity.\n> \n> $ time git rev-list --all --objects --verify-objects >/dev/null\n> \n> # CommonCrypto 28.95s user 4.62s system 99% cpu 33.630 total\n> # master 29.81s user 4.70s system 99% cpu 34.760 total\n> # BLK_SHA1 29.80s user 4.62s system 99% cpu 34.505 total\n> \n> If BLK_SHA1 were the default on all platforms then I wouldn't have\n> bothered with the SHA-1 patch. With this patch it makes it like Linux\n> in that Git can choose between the built-in functions and the external\n> library.\n\nWith those numbers above, it seems like CommonCrypto is still a\nreasonable choice. I don't know anything about availability or other OS\nX specific issues, but assuming there are no issues there, your patch\nmakes sense to me. Thanks for double-checking the timings.\n\n-Peff\n"}]}