{"thread":{"id":"45358","subject":"[RFC PATCH] Move SHA-1 implementation selection into a header file","startedAt":"2017-03-11T22:28:34Z","lastAt":"2017-03-15T19:50:21Z","messageCount":16,"participants":["brian m. carlson","Jeff King","Junio C Hamano","Jonathan Nieder","Ramsay Jones"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"313838","messageId":"20170311222818.518541-1-sandals@crustytoothpaste.net","threadId":"45358","inReplyTo":null,"subject":"[RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-03-11T22:28:18Z","receivedAt":"2017-03-11T22:28:34Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Many developers use functionality in their editors that allows for quick\nsyntax checks, including warning about questionable constructs.  This\nfunctionality allows rapid development with fewer errors.  However, such\nfunctionality generally does not allow the specification of\nproject-specific defines or command-line options.\n\nSince the SHA1_HEADER include is not defined in such a case, developers\nsee spurious errors when using these tools.  Furthermore, while using a\nmacro as the argument to #include is permitted by C11, it isn't\npermitted by C89 and C99, and there are known implementations which\nreject it.\n\nInstead of using SHA1_HEADER, create a hash.h header and use #if\nand #elif to select the desired header.  Have the Makefile pass an\nappropriate option to help the header select the right implementation to\nuse.\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n---\n\nFixing this makes my development using the vim-ale plugin much nicer.  I\ndon't care which implementation is selected by default, as long as\n*some* implementation is selected by default.\n\nI called this \"hash.h\" instead of \"sha1.h\" to allow for future hash\nfunction extensions.  I was worried that some OS might define a hash.h\nheader as well, but the use of quotation marks instead of angle brackets\nshould cause it to look in the current directory first.\n\nI also picked \"SHA1_*\" instead of \"*_SHA1\" as it makes it easier to find\nall the constants.\n\n Makefile |  8 ++++----\n cache.h  |  2 +-\n hash.h   | 14 ++++++++++++++\n 3 files changed, 19 insertions(+), 5 deletions(-)\n create mode 100644 hash.h\n\ndiff --git a/Makefile b/Makefile\nindex ed68700acb..244eb6a0f2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1384,19 +1384,19 @@ ifdef APPLE_COMMON_CRYPTO\n endif\n \n ifdef BLK_SHA1\n-\tSHA1_HEADER = \"block-sha1/sha1.h\"\n \tLIB_OBJS += block-sha1/sha1.o\n+\tBASIC_CFLAGS += -DSHA1_BLK\n else\n ifdef PPC_SHA1\n-\tSHA1_HEADER = \"ppc/sha1.h\"\n \tLIB_OBJS += ppc/sha1.o ppc/sha1ppc.o\n+\tBASIC_CFLAGS += -DSHA1_PPC\n else\n ifdef APPLE_COMMON_CRYPTO\n \tCOMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL\n-\tSHA1_HEADER = <CommonCrypto/CommonDigest.h>\n+\tBASIC_CFLAGS += -DSHA1_APPLE\n else\n-\tSHA1_HEADER = <openssl/sha.h>\n \tEXTLIBS += $(LIB_4_CRYPTO)\n+\tBASIC_CFLAGS += -DSHA1_OPENSSL\n endif\n endif\n endif\ndiff --git a/cache.h b/cache.h\nindex 283ab8fb40..6a9afb8561 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -10,8 +10,8 @@\n #include \"trace.h\"\n #include \"string-list.h\"\n #include \"pack-revindex.h\"\n+#include \"hash.h\"\n \n-#include SHA1_HEADER\n #ifndef platform_SHA_CTX\n /*\n  * platform's underlying implementation of SHA-1; could be OpenSSL,\ndiff --git a/hash.h b/hash.h\nnew file mode 100644\nindex 0000000000..7c6b52835c\n--- /dev/null\n+++ b/hash.h\n@@ -0,0 +1,14 @@\n+#ifndef HASH_H\n+#define HASH_H\n+\n+#if defined(SHA1_BLK)\n+#include \"block-sha1/sha1.h\"\n+#elif defined(SHA1_PPC)\n+#include \"ppc/sha1.h\"\n+#elif defined(SHA1_APPLE)\n+#include <CommonCrypto/CommonDigest.h>\n+#else /* SHA1_OPENSSL */\n+#include <openssl/sha.h>\n+#endif\n+\n+#endif\n"},{"id":"313858","messageId":"20170312130149.czir5hcbosqlmkhb@sigill.intra.peff.net","threadId":"45358","inReplyTo":"20170311222818.518541-1-sandals@crustytoothpaste.net","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-12T13:01:49Z","receivedAt":"2017-03-12T13:02:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Mar 11, 2017 at 10:28:18PM +0000, brian m. carlson wrote:\n\n> Many developers use functionality in their editors that allows for quick\n> syntax checks, including warning about questionable constructs.  This\n> functionality allows rapid development with fewer errors.  However, such\n> functionality generally does not allow the specification of\n> project-specific defines or command-line options.\n> \n> Since the SHA1_HEADER include is not defined in such a case, developers\n> see spurious errors when using these tools.  Furthermore, while using a\n> macro as the argument to #include is permitted by C11, it isn't\n> permitted by C89 and C99, and there are known implementations which\n> reject it.\n> \n> Instead of using SHA1_HEADER, create a hash.h header and use #if\n> and #elif to select the desired header.  Have the Makefile pass an\n> appropriate option to help the header select the right implementation to\n> use.\n\nThis has bit me once or twice in the past[1], too, so I'm happy to see\nsomebody tackling it.\n\nYour solution does mean that your tool, whatever it is, that runs\nwithout the same CFLAGS as the Makefile may get a _different_ sha1\nimplementation, though. That's probably good enough for some purposes\nbut perhaps not for others.\n\nThe \"most correct\" solution, I think, would be to stop using \"-D\" flags\nin favor of actually generating hash.h. Something like the patch\nbelow[2] (though I think it there are some rough edges with the\ndependencies).\n\nOf course the sha1 header is just one of many such defines. It's the one\nthat is most visible because the result is syntactically valid without\nit, but anything you compile without the Makefile's CFLAGS may be subtly\ndifferent than what the Makefile would produce. So arguably the Makefile\nshould be writing out a build-options.h with all of the values, and that\nshould get pulled in by git-compat-util.h.\n\nI don't know if we want to go down that rabbit hole or not. I offer it\nmerely as an alternative. I'm OK with your patch as-is if you don't want\nto dump any more time into it.\n\n-Peff\n\n[1] I think I hit the problem when trying to debug some internal part of\n    git and writing a one-off \"foo.c\" that calls the code I want. You\n    can't compile it with \"gcc foo.c\".\n\n[2] Here's what a patch for the generated-header might look like:\n\ndiff --git a/Makefile b/Makefile\nindex 9f0eae428..0d65d50e9 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -690,6 +690,7 @@ XDIFF_LIB = xdiff/lib.a\n VCSSVN_LIB = vcs-svn/lib.a\n \n GENERATED_H += common-cmds.h\n+GENERATED_H += hash.h\n \n LIB_H = $(shell $(FIND) . \\\n \t-name .git -prune -o \\\n@@ -1639,8 +1640,7 @@ PERLLIB_EXTRA_SQ = $(subst ','\\'',$(PERLLIB_EXTRA))\n # from the dependency list, that would make each entry appear twice.\n LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)\n \n-BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \\\n-\t$(COMPAT_CFLAGS)\n+BASIC_CFLAGS += $(COMPAT_CFLAGS)\n LIB_OBJS += $(COMPAT_OBJS)\n \n # Quote for C\n@@ -1781,7 +1781,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)\n \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \\\n \t\t$(filter %.o,$^) $(LIBS)\n \n-help.sp help.s help.o: common-cmds.h\n+help.sp help.s help.o: common-cmds.h hash.h\n \n builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX\n builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \\\n@@ -1805,6 +1805,10 @@ common-cmds.h: generate-cmdlist.sh command-list.txt\n common-cmds.h: $(wildcard Documentation/git-*.txt)\n \t$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ && mv $@+ $@\n \n+hash.h:\n+\t$(QUIET_GEN)echo '#include $(SHA1_HEADER)' >$@+ && \\\n+\t{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; }\n+\n SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\\\n \t$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\\\n \t$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV)\ndiff --git a/cache.h b/cache.h\nindex 0a14141fc..345104849 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -11,7 +11,7 @@\n #include \"string-list.h\"\n #include \"pack-revindex.h\"\n \n-#include SHA1_HEADER\n+#include \"hash.h\"\n #ifndef platform_SHA_CTX\n /*\n  * platform's underlying implementation of SHA-1; could be OpenSSL,\n"},{"id":"313861","messageId":"20170312165119.3eokqse4uxdet6yd@genre.crustytoothpaste.net","threadId":"45358","inReplyTo":"20170312130149.czir5hcbosqlmkhb@sigill.intra.peff.net","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-03-12T16:51:19Z","receivedAt":"2017-03-12T16:51:33Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Mar 12, 2017 at 09:01:49AM -0400, Jeff King wrote:\n> On Sat, Mar 11, 2017 at 10:28:18PM +0000, brian m. carlson wrote:\n> \n> > Many developers use functionality in their editors that allows for quick\n> > syntax checks, including warning about questionable constructs.  This\n> > functionality allows rapid development with fewer errors.  However, such\n> > functionality generally does not allow the specification of\n> > project-specific defines or command-line options.\n> > \n> > Since the SHA1_HEADER include is not defined in such a case, developers\n> > see spurious errors when using these tools.  Furthermore, while using a\n> > macro as the argument to #include is permitted by C11, it isn't\n> > permitted by C89 and C99, and there are known implementations which\n> > reject it.\n> > \n> > Instead of using SHA1_HEADER, create a hash.h header and use #if\n> > and #elif to select the desired header.  Have the Makefile pass an\n> > appropriate option to help the header select the right implementation to\n> > use.\n> \n> This has bit me once or twice in the past[1], too, so I'm happy to see\n> somebody tackling it.\n> \n> Your solution does mean that your tool, whatever it is, that runs\n> without the same CFLAGS as the Makefile may get a _different_ sha1\n> implementation, though. That's probably good enough for some purposes\n> but perhaps not for others.\n\nYeah, my goal was basically to pass -fsyntax-only, not to produce useful\nobject files.  My patch does basically require that the user have\nOpenSSL installed, but I do, so it doesn't matter.\n\nI considered after the fact that I might just do something like:\n\n  #ifdef SHA1_HEADER\n  #include SHA1_HEADER\n  #else\n  #include \"block-sha1/sha1.h\"\n  #endif\n\nThat would be the smallest change, but probably not the best.\n\n> The \"most correct\" solution, I think, would be to stop using \"-D\" flags\n> in favor of actually generating hash.h. Something like the patch\n> below[2] (though I think it there are some rough edges with the\n> dependencies).\n\nI agree that's a better solution.  I was concerned about avoiding ending\nup rebuilding everything when we regenerated the file, but it looks like\nyou've avoided that with cmp.\n\n> Of course the sha1 header is just one of many such defines. It's the one\n> that is most visible because the result is syntactically valid without\n> it, but anything you compile without the Makefile's CFLAGS may be subtly\n> different than what the Makefile would produce. So arguably the Makefile\n> should be writing out a build-options.h with all of the values, and that\n> should get pulled in by git-compat-util.h.\n> \n> I don't know if we want to go down that rabbit hole or not. I offer it\n> merely as an alternative. I'm OK with your patch as-is if you don't want\n> to dump any more time into it.\n\nI'll take this patch for now and fix it up with the comment I mentioned\nbelow.  If someone wants to improve the situation down the line, then\nthey can pick that up.\n\nI assume I can apply your sign-off to the resulting patch?\n\n> -Peff\n> \n> [1] I think I hit the problem when trying to debug some internal part of\n>     git and writing a one-off \"foo.c\" that calls the code I want. You\n>     can't compile it with \"gcc foo.c\".\n> \n> [2] Here's what a patch for the generated-header might look like:\n> \n> diff --git a/Makefile b/Makefile\n> index 9f0eae428..0d65d50e9 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -690,6 +690,7 @@ XDIFF_LIB = xdiff/lib.a\n>  VCSSVN_LIB = vcs-svn/lib.a\n>  \n>  GENERATED_H += common-cmds.h\n> +GENERATED_H += hash.h\n>  \n>  LIB_H = $(shell $(FIND) . \\\n>  \t-name .git -prune -o \\\n> @@ -1639,8 +1640,7 @@ PERLLIB_EXTRA_SQ = $(subst ','\\'',$(PERLLIB_EXTRA))\n>  # from the dependency list, that would make each entry appear twice.\n>  LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)\n>  \n> -BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \\\n> -\t$(COMPAT_CFLAGS)\n> +BASIC_CFLAGS += $(COMPAT_CFLAGS)\n>  LIB_OBJS += $(COMPAT_OBJS)\n>  \n>  # Quote for C\n> @@ -1781,7 +1781,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)\n>  \t$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \\\n>  \t\t$(filter %.o,$^) $(LIBS)\n>  \n> -help.sp help.s help.o: common-cmds.h\n> +help.sp help.s help.o: common-cmds.h hash.h\n>  \n>  builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX\n>  builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \\\n> @@ -1805,6 +1805,10 @@ common-cmds.h: generate-cmdlist.sh command-list.txt\n>  common-cmds.h: $(wildcard Documentation/git-*.txt)\n>  \t$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ && mv $@+ $@\n>  \n> +hash.h:\n> +\t$(QUIET_GEN)echo '#include $(SHA1_HEADER)' >$@+ && \\\n> +\t{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; }\n\nI think here we'd want to also \"rm -f $@+\", so that we don't leave an\nextra file behind if we're up-to-date (which is the common case), much\nlike we do for GIT-BUILD-OPTIONS.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"313863","messageId":"xmqqfuiiv0s7.fsf@gitster.mtv.corp.google.com","threadId":"45358","inReplyTo":"20170312130149.czir5hcbosqlmkhb@sigill.intra.peff.net","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-12T17:54:16Z","receivedAt":"2017-03-12T17:54:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> diff --git a/Makefile b/Makefile\n> index 9f0eae428..0d65d50e9 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -690,6 +690,7 @@ XDIFF_LIB = xdiff/lib.a\n>  VCSSVN_LIB = vcs-svn/lib.a\n>  \n>  GENERATED_H += common-cmds.h\n> +GENERATED_H += hash.h\n> ...\n> -BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \\\n> -\t$(COMPAT_CFLAGS)\n> +BASIC_CFLAGS += $(COMPAT_CFLAGS)\n>   ...\n> +hash.h:\n> +\t$(QUIET_GEN)echo '#include $(SHA1_HEADER)' >$@+ && \\\n> +\t{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; }\n> +\n\nThat looks sensible.  We do not have to adjust the code to update\nGIT-BUILD-OPTIONS and friends in this patch because the current way\nto force rebuilding when SHA1_HEADER changes is by letting\nGIT-CFLAGS file notice the change, and the new \"hash.h\" itself will\nlet the $(MAKE) notice if a different hash implementation is chosen\nfor the build.\n\n\n"},{"id":"313868","messageId":"20170312201237.wrstmhk5saiy52m4@sigill.intra.peff.net","threadId":"45358","inReplyTo":"20170312165119.3eokqse4uxdet6yd@genre.crustytoothpaste.net","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-12T20:12:37Z","receivedAt":"2017-03-12T20:12:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Mar 12, 2017 at 04:51:19PM +0000, brian m. carlson wrote:\n\n> Yeah, my goal was basically to pass -fsyntax-only, not to produce useful\n> object files.  My patch does basically require that the user have\n> OpenSSL installed, but I do, so it doesn't matter.\n> \n> I considered after the fact that I might just do something like:\n> \n>   #ifdef SHA1_HEADER\n>   #include SHA1_HEADER\n>   #else\n>   #include \"block-sha1/sha1.h\"\n>   #endif\n> \n> That would be the smallest change, but probably not the best.\n\nYeah, if there is going to be a fallback it probably ought to be\nthe internal one.\n\n> > Of course the sha1 header is just one of many such defines. It's the one\n> > that is most visible because the result is syntactically valid without\n> > it, but anything you compile without the Makefile's CFLAGS may be subtly\n> > different than what the Makefile would produce. So arguably the Makefile\n> > should be writing out a build-options.h with all of the values, and that\n> > should get pulled in by git-compat-util.h.\n> > \n> > I don't know if we want to go down that rabbit hole or not. I offer it\n> > merely as an alternative. I'm OK with your patch as-is if you don't want\n> > to dump any more time into it.\n> \n> I'll take this patch for now and fix it up with the comment I mentioned\n> below.  If someone wants to improve the situation down the line, then\n> they can pick that up.\n> \n> I assume I can apply your sign-off to the resulting patch?\n\nYes, it is OK to add my sign-off. The two things I was concerned about\nwith my patch were:\n\n  1. It does introduce an extra shell invocation on every \"make\", even\n     if the file does not need to be rebuilt (though it is just one of\n     many; GIT-BUILD-OPTIONS, etc).\n\n  2. I wasn't sure if the dependencies were quite right. I _thought_ we\n     should pick it up as an auto-dependency, but I don't think that\n     works because we do our header dependencies as a side effect of the\n     compile.\n\n     So \"make\" didn't actually know to build hash.h until I made it a\n     dependency of help.o. Which feels weird and hacky. It's really a\n     dependency of _anything_ that includes cache.h.\n\nSo you may want to dig into that second one to make sure the result is\nsane, not racy, etc.\n\n> > +hash.h:\n> > +\t$(QUIET_GEN)echo '#include $(SHA1_HEADER)' >$@+ && \\\n> > +\t{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; }\n> \n> I think here we'd want to also \"rm -f $@+\", so that we don't leave an\n> extra file behind if we're up-to-date (which is the common case), much\n> like we do for GIT-BUILD-OPTIONS.\n\nYeah, I agree that would be an improvement.\n\n-Peff\n"},{"id":"314048","messageId":"20170314184126.GJ26789@aiede.mtv.corp.google.com","threadId":"45358","inReplyTo":"20170311222818.518541-1-sandals@crustytoothpaste.net","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-14T18:41:26Z","receivedAt":"2017-03-14T18:41:33Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nbrian m. carlson wrote:\n\n[...]\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -10,8 +10,8 @@\n>  #include \"trace.h\"\n>  #include \"string-list.h\"\n>  #include \"pack-revindex.h\"\n> +#include \"hash.h\"\n>  \n> -#include SHA1_HEADER\n\nFor what it's worth, the bazel build tool doesn't like this\n'#include SHA1_HEADER' either.  Your fix looks like a straightforward\nfix and we never encouraged directly customizing SHA1_HEADER.\n\nThe other approaches discussed may also work but they don't add\nanything for my application (nor yours, I'd think).  Conditional\n#includes are a pretty normal thing so I am fine with this more\nstraightforward change.  So\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThat said, if someone is excited about one of the other approaches\nthen I don't object to them.\n\nThanks,\nJonathan\n"},{"id":"314055","messageId":"20170314201424.vccij5z2ortq4a4o@sigill.intra.peff.net","threadId":"45358","inReplyTo":"20170314184126.GJ26789@aiede.mtv.corp.google.com","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-14T20:14:24Z","receivedAt":"2017-03-14T20:14:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 14, 2017 at 11:41:26AM -0700, Jonathan Nieder wrote:\n\n> brian m. carlson wrote:\n> \n> [...]\n> > --- a/cache.h\n> > +++ b/cache.h\n> > @@ -10,8 +10,8 @@\n> >  #include \"trace.h\"\n> >  #include \"string-list.h\"\n> >  #include \"pack-revindex.h\"\n> > +#include \"hash.h\"\n> >  \n> > -#include SHA1_HEADER\n> \n> For what it's worth, the bazel build tool doesn't like this\n> '#include SHA1_HEADER' either.  Your fix looks like a straightforward\n> fix and we never encouraged directly customizing SHA1_HEADER.\n\nHmm. I don't know how you're using bazel with git, but if it is doing\nsomething like generating header dependencies, would that mean that it\npotentially picks up the wrong dependency with brian's patch?\n\n> The other approaches discussed may also work but they don't add\n> anything for my application (nor yours, I'd think).  Conditional\n> #includes are a pretty normal thing so I am fine with this more\n> straightforward change.  So\n> \n> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n> \n> That said, if someone is excited about one of the other approaches\n> then I don't object to them.\n\nMy biggest complaint with the initial patch is that it repeats the\nif-else chain of each type (once in the Makefile and once in hash.h). I\ncan live with having to touch two parts of the code (it's not like we\nexpect a huge number of alternative sha1 implementations), but I worried\nthat we were replicating the \"which one is primary\" logic in two places\n(i.e., the Makefile prefers BLK_SHA1 above others, and I expect that\nwhen we add USE_SHA1DC it may become the primary).\n\nBut I think it's probably OK in practice. The if-else inside hash.h is\nchecking the -D defines we set in the Makefile, and the Makefile logic\nwould set only one such define. So hash.h would have to follow whatever\nthe Makefile picked (unless you do something idiotic like \"CFLAGS +=\n-DBLK_SHA1\" yourself, but then you deserve every bad thing that comes\nyour way).\n\nSo I'm OK with brian's patch as the simplest thing that covers\nnon-compilation use cases. It should probably default to BLK_SHA1 rather\nthan OpenSSL, as discussed earlier.\n\n-Peff\n"},{"id":"314058","messageId":"xmqq1stzio5b.fsf@gitster.mtv.corp.google.com","threadId":"45358","inReplyTo":"20170314201424.vccij5z2ortq4a4o@sigill.intra.peff.net","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-14T20:44:48Z","receivedAt":"2017-03-14T20:44:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"OK, then I'll queue this.  The selection still goes to BASIC_CFLAGS\nso the dependencies for re-compilation should be right, I'd think.\n\n-- >8 --\nFrom: \"brian m. carlson\" <sandals@crustytoothpaste.net>\nDate: Sat, 11 Mar 2017 22:28:18 +0000\nSubject: [PATCH] hash.h: move SHA-1 implementation selection into a header file\n\nMany developers use functionality in their editors that allows for quick\nsyntax checks, including warning about questionable constructs.  This\nfunctionality allows rapid development with fewer errors.  However, such\nfunctionality generally does not allow the specification of\nproject-specific defines or command-line options.\n\nSince the SHA1_HEADER include is not defined in such a case, developers\nsee spurious errors when using these tools.  Furthermore, while using a\nmacro as the argument to #include is permitted by C11, it isn't\npermitted by C89 and C99, and there are known implementations which\nreject it.\n\nInstead of using SHA1_HEADER, create a hash.h header and use #if\nand #elif to select the desired header.  Have the Makefile pass an\nappropriate option to help the header select the right implementation to\nuse.\n\n[jc: make BLK_SHA1 the fallback default as discussed on list,\ne.g. <20170314201424.vccij5z2ortq4a4o@sigill.intra.peff.net>; also\nremove SHA1_HEADER and SHA1_HEADER_SQ that are no longer used].\n\nSigned-off-by: brian m. carlson <sandals@crustytoothpaste.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Makefile | 12 +++++-------\n cache.h  |  2 +-\n hash.h   | 14 ++++++++++++++\n 3 files changed, 20 insertions(+), 8 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 8e4081e061..25c21f08b1 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1387,19 +1387,19 @@ ifdef APPLE_COMMON_CRYPTO\n endif\n \n ifdef BLK_SHA1\n-\tSHA1_HEADER = \"block-sha1/sha1.h\"\n \tLIB_OBJS += block-sha1/sha1.o\n+\tBASIC_CFLAGS += -DSHA1_BLK\n else\n ifdef PPC_SHA1\n-\tSHA1_HEADER = \"ppc/sha1.h\"\n \tLIB_OBJS += ppc/sha1.o ppc/sha1ppc.o\n+\tBASIC_CFLAGS += -DSHA1_PPC\n else\n ifdef APPLE_COMMON_CRYPTO\n \tCOMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL\n-\tSHA1_HEADER = <CommonCrypto/CommonDigest.h>\n+\tBASIC_CFLAGS += -DSHA1_APPLE\n else\n-\tSHA1_HEADER = <openssl/sha.h>\n \tEXTLIBS += $(LIB_4_CRYPTO)\n+\tBASIC_CFLAGS += -DSHA1_OPENSSL\n endif\n endif\n endif\n@@ -1591,7 +1591,6 @@ endif\n \n # Shell quote (do not use $(call) to accommodate ancient setups);\n \n-SHA1_HEADER_SQ = $(subst ','\\'',$(SHA1_HEADER))\n ETC_GITCONFIG_SQ = $(subst ','\\'',$(ETC_GITCONFIG))\n ETC_GITATTRIBUTES_SQ = $(subst ','\\'',$(ETC_GITATTRIBUTES))\n \n@@ -1624,8 +1623,7 @@ PERLLIB_EXTRA_SQ = $(subst ','\\'',$(PERLLIB_EXTRA))\n # from the dependency list, that would make each entry appear twice.\n LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)\n \n-BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \\\n-\t$(COMPAT_CFLAGS)\n+BASIC_CFLAGS += $(COMPAT_CFLAGS)\n LIB_OBJS += $(COMPAT_OBJS)\n \n # Quote for C\ndiff --git a/cache.h b/cache.h\nindex 61fc86e6d7..f98c95bf2a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -10,8 +10,8 @@\n #include \"trace.h\"\n #include \"string-list.h\"\n #include \"pack-revindex.h\"\n+#include \"hash.h\"\n \n-#include SHA1_HEADER\n #ifndef platform_SHA_CTX\n /*\n  * platform's underlying implementation of SHA-1; could be OpenSSL,\ndiff --git a/hash.h b/hash.h\nnew file mode 100644\nindex 0000000000..f0d9ddd0c2\n--- /dev/null\n+++ b/hash.h\n@@ -0,0 +1,14 @@\n+#ifndef HASH_H\n+#define HASH_H\n+\n+#if defined(SHA1_PPC)\n+#include \"ppc/sha1.h\"\n+#elif defined(SHA1_APPLE)\n+#include <CommonCrypto/CommonDigest.h>\n+#elif defined(SHA1_OPENSSL)\n+#include <openssl/sha.h>\n+#else /* SHA1_BLK */\n+#include \"block-sha1/sha1.h\"\n+#endif\n+\n+#endif\n"},{"id":"314061","messageId":"20170314212612.webhzphavvislt4u@sigill.intra.peff.net","threadId":"45358","inReplyTo":"xmqq1stzio5b.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-03-14T21:26:12Z","receivedAt":"2017-03-14T21:26:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 14, 2017 at 01:44:48PM -0700, Junio C Hamano wrote:\n\n> OK, then I'll queue this.  The selection still goes to BASIC_CFLAGS\n> so the dependencies for re-compilation should be right, I'd think.\n\nYeah, I think that part should be fine.\n\n> -- >8 --\n> From: \"brian m. carlson\" <sandals@crustytoothpaste.net>\n> Date: Sat, 11 Mar 2017 22:28:18 +0000\n> Subject: [PATCH] hash.h: move SHA-1 implementation selection into a header file\n\nLooks good to me.\n\nReviewed-by: Jeff King <peff@peff.net>\n\n-Peff\n"},{"id":"314085","messageId":"20170314215003.GK26789@aiede.mtv.corp.google.com","threadId":"45358","inReplyTo":"xmqq1stzio5b.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-14T21:50:03Z","receivedAt":"2017-03-14T21:50:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Junio C Hamano wrote:\n\n> [jc: make BLK_SHA1 the fallback default as discussed on list,\n> e.g. <20170314201424.vccij5z2ortq4a4o@sigill.intra.peff.net>; also\n> remove SHA1_HEADER and SHA1_HEADER_SQ that are no longer used].\n>\n> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>\n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks for taking care of it.\n"},{"id":"314086","messageId":"20170314215605.GL26789@aiede.mtv.corp.google.com","threadId":"45358","inReplyTo":"20170314201424.vccij5z2ortq4a4o@sigill.intra.peff.net","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-03-14T21:56:06Z","receivedAt":"2017-03-14T21:56:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Tue, Mar 14, 2017 at 11:41:26AM -0700, Jonathan Nieder wrote:\n>> brian m. carlson wrote:\n\n>>> --- a/cache.h\n>>> +++ b/cache.h\n>>> @@ -10,8 +10,8 @@\n>>>  #include \"trace.h\"\n>>>  #include \"string-list.h\"\n>>>  #include \"pack-revindex.h\"\n>>> +#include \"hash.h\"\n>>>\n>>> -#include SHA1_HEADER\n>>\n>> For what it's worth, the bazel build tool doesn't like this\n>> '#include SHA1_HEADER' either.  Your fix looks like a straightforward\n>> fix and we never encouraged directly customizing SHA1_HEADER.\n>\n> Hmm. I don't know how you're using bazel with git, but if it is doing\n> something like generating header dependencies, would that mean that it\n> potentially picks up the wrong dependency with brian's patch?\n\nI believe it picks up all options as dependencies, which is good\nenough for me.\n\nI have a custom BUILD file to build git with bazel.  I like the\nreliable dependencies it generates (unless you do heavy contortions,\nfiles aren't available to the build commands unless the dependency is\ndeclared) and fast, parallel build with simple progress output.  But\nkeeping it up to date with every patch that changes the Makefile is\nnot something I would wish on the git project.\n\nOne of these days I'd like to try making a tool to automatically\ngenerate the BUILD file, like contrib/buildsystems generates a\nVisual C project.\n\nRegards,\nJonathan\n"},{"id":"314109","messageId":"1fce97df-7eb8-79d5-b96b-2086746bf19d@ramsayjones.plus.com","threadId":"45358","inReplyTo":"xmqq1stzio5b.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-03-14T23:42:20Z","receivedAt":"2017-03-14T23:42:29Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 14/03/17 20:44, Junio C Hamano wrote:\n> OK, then I'll queue this.  The selection still goes to BASIC_CFLAGS\n> so the dependencies for re-compilation should be right, I'd think.\n> \n> -- >8 --\n> From: \"brian m. carlson\" <sandals@crustytoothpaste.net>\n> Date: Sat, 11 Mar 2017 22:28:18 +0000\n> Subject: [PATCH] hash.h: move SHA-1 implementation selection into a header file\n> \n> Many developers use functionality in their editors that allows for quick\n> syntax checks, including warning about questionable constructs.  This\n> functionality allows rapid development with fewer errors.  However, such\n> functionality generally does not allow the specification of\n> project-specific defines or command-line options.\n> \n> Since the SHA1_HEADER include is not defined in such a case, developers\n> see spurious errors when using these tools.  Furthermore, while using a\n> macro as the argument to #include is permitted by C11, it isn't\n> permitted by C89 and C99, and there are known implementations which\n> reject it.\n\nC99 certainly allows a macro argument to #include (see, 6.10.2-4; there\nis also an example in 6.10.2-8).\n\nI can't remember if it's allowed in C89/C90 (I think it is). I only\nhave immediate access to the C99 and C11 standards (and I can't be\nbothered to search), so I can't say for sure.\n\nATB,\nRamsay Jones\n\n\n"},{"id":"314110","messageId":"20170314234659.wamzgpqu2aikghci@genre.crustytoothpaste.net","threadId":"45358","inReplyTo":"1fce97df-7eb8-79d5-b96b-2086746bf19d@ramsayjones.plus.com","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-03-14T23:46:59Z","receivedAt":"2017-03-14T23:47:11Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Tue, Mar 14, 2017 at 11:42:20PM +0000, Ramsay Jones wrote:\n> \n> \n> On 14/03/17 20:44, Junio C Hamano wrote:\n> > OK, then I'll queue this.  The selection still goes to BASIC_CFLAGS\n> > so the dependencies for re-compilation should be right, I'd think.\n> > \n> > -- >8 --\n> > From: \"brian m. carlson\" <sandals@crustytoothpaste.net>\n> > Date: Sat, 11 Mar 2017 22:28:18 +0000\n> > Subject: [PATCH] hash.h: move SHA-1 implementation selection into a header file\n> > \n> > Many developers use functionality in their editors that allows for quick\n> > syntax checks, including warning about questionable constructs.  This\n> > functionality allows rapid development with fewer errors.  However, such\n> > functionality generally does not allow the specification of\n> > project-specific defines or command-line options.\n> > \n> > Since the SHA1_HEADER include is not defined in such a case, developers\n> > see spurious errors when using these tools.  Furthermore, while using a\n> > macro as the argument to #include is permitted by C11, it isn't\n> > permitted by C89 and C99, and there are known implementations which\n> > reject it.\n> \n> C99 certainly allows a macro argument to #include (see, 6.10.2-4; there\n> is also an example in 6.10.2-8).\n> \n> I can't remember if it's allowed in C89/C90 (I think it is). I only\n> have immediate access to the C99 and C11 standards (and I can't be\n> bothered to search), so I can't say for sure.\n\nYou're right.  I only have access to N1124 (the C99 final draft), but it\ndoes allow that.  I could have sworn it was new in C11.  I'm pretty\ncertain it isn't allowed in C89, but I don't have access to that\nstandard.\n\nI know there have been reasonably standards-conforming compilers that\nhave rejected it in the past, but I can't remember which ones (I think\nthey were for proprietary Unices).\n\nJunio, do you want to amend the commit message before you merge it?\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\n+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"314113","messageId":"dd285cb9-c7c7-e945-1fa2-7eb58d83ea06@ramsayjones.plus.com","threadId":"45358","inReplyTo":"20170314234659.wamzgpqu2aikghci@genre.crustytoothpaste.net","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-03-15T00:15:39Z","receivedAt":"2017-03-15T00:15:46Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 14/03/17 23:46, brian m. carlson wrote:\n> On Tue, Mar 14, 2017 at 11:42:20PM +0000, Ramsay Jones wrote:\n>>\n>>\n>> On 14/03/17 20:44, Junio C Hamano wrote:\n>>> OK, then I'll queue this.  The selection still goes to BASIC_CFLAGS\n>>> so the dependencies for re-compilation should be right, I'd think.\n>>>\n>>> -- >8 --\n>>> From: \"brian m. carlson\" <sandals@crustytoothpaste.net>\n>>> Date: Sat, 11 Mar 2017 22:28:18 +0000\n>>> Subject: [PATCH] hash.h: move SHA-1 implementation selection into a header file\n>>>\n>>> Many developers use functionality in their editors that allows for quick\n>>> syntax checks, including warning about questionable constructs.  This\n>>> functionality allows rapid development with fewer errors.  However, such\n>>> functionality generally does not allow the specification of\n>>> project-specific defines or command-line options.\n>>>\n>>> Since the SHA1_HEADER include is not defined in such a case, developers\n>>> see spurious errors when using these tools.  Furthermore, while using a\n>>> macro as the argument to #include is permitted by C11, it isn't\n>>> permitted by C89 and C99, and there are known implementations which\n>>> reject it.\n>>\n>> C99 certainly allows a macro argument to #include (see, 6.10.2-4; there\n>> is also an example in 6.10.2-8).\n>>\n>> I can't remember if it's allowed in C89/C90 (I think it is). I only\n>> have immediate access to the C99 and C11 standards (and I can't be\n>> bothered to search), so I can't say for sure.\n> \n> You're right.  I only have access to N1124 (the C99 final draft), but it\n> does allow that.  I could have sworn it was new in C11.  I'm pretty\n> certain it isn't allowed in C89, but I don't have access to that\n> standard.\n\nMy copies of Harbison and Steele (Third and Fifth editions) claim that\nStandard C supports it also (by which they mean C89/C90).\n\n> I know there have been reasonably standards-conforming compilers that\n> have rejected it in the past, but I can't remember which ones (I think\n> they were for proprietary Unices).\n\nYes, I think that happened to me on Irix, if I recall correctly.\n\n> Junio, do you want to amend the commit message before you merge it?\n\nYes, please! ;-)\n\nATB,\nRamsay Jones\n\n\n"},{"id":"314139","messageId":"xmqqvarafs7j.fsf@gitster.mtv.corp.google.com","threadId":"45358","inReplyTo":"dd285cb9-c7c7-e945-1fa2-7eb58d83ea06@ramsayjones.plus.com","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-15T15:57:36Z","receivedAt":"2017-03-15T15:59:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n\n> On 14/03/17 23:46, brian m. carlson wrote:\n>>>>\n>>>> Since the SHA1_HEADER include is not defined in such a case, developers\n>>>> see spurious errors when using these tools.  Furthermore, while using a\n>>>> macro as the argument to #include is permitted by C11, it isn't\n>>>> permitted by C89 and C99, and there are known implementations which\n>>>> reject it.\n>>>\n>\n>> Junio, do you want to amend the commit message before you merge it?\n>\n> Yes, please! ;-)\n\nIf only you were a few hours quicker.\n\nLet me see how bad the fallout is to revert the merge to 'next' and\nmerge an amended version in.\n\nI _think_ the whole \"Furthermore\" sentence can go, since nobody\ncomplained since cef661fc (\"Add support for alternate SHA1 library\nimplementations.\", 2005-04-21) started using the Makefile construct.\n\nThanks.\n"},{"id":"314161","messageId":"512b2a2b-237b-1e73-5a85-1a19a796aa2b@ramsayjones.plus.com","threadId":"45358","inReplyTo":"xmqqvarafs7j.fsf@gitster.mtv.corp.google.com","subject":"Re: [RFC PATCH] Move SHA-1 implementation selection into a header file","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsayjones.plus.com","sentAt":"2017-03-15T19:48:56Z","receivedAt":"2017-03-15T19:50:21Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"\n\nOn 15/03/17 15:57, Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:\n> \n>> On 14/03/17 23:46, brian m. carlson wrote:\n>>>>>\n>>>>> Since the SHA1_HEADER include is not defined in such a case, developers\n>>>>> see spurious errors when using these tools.  Furthermore, while using a\n>>>>> macro as the argument to #include is permitted by C11, it isn't\n>>>>> permitted by C89 and C99, and there are known implementations which\n>>>>> reject it.\n>>>>\n>>\n>>> Junio, do you want to amend the commit message before you merge it?\n>>\n>> Yes, please! ;-)\n> \n> If only you were a few hours quicker.\n\nOops, sorry. When I wrote that I didn't know it was already in 'next'.\n\n[I tend not to reply to emails as soon as I read them (because it\noften gets me into trouble!), but wait until I've read everything\nin my inbox. Unfortunately, I get so much email, it can be hours\nlater before I respond ... (I keep saying that I will unsubscribe\nfrom some mailing lists ;-) ).]\n\n> Let me see how bad the fallout is to revert the merge to 'next' and\n> merge an amended version in.\n\nHmm, I didn't intend to add to your workload! Is it worth the hassle?\nIn the great scheme of things, it's not a major issue. I dunno.\n\n> I _think_ the whole \"Furthermore\" sentence can go, since nobody\n> complained since cef661fc (\"Add support for alternate SHA1 library\n> implementations.\", 2005-04-21) started using the Makefile construct.\n\nYep.\n\n[BTW, I haven't finished reading everything in my inbox yet, hopefully\nI won't get into trouble. :P ]\n\nATB,\nRamsay Jones\n\n\n"}]}