threads / rfc / 45358

RFC patchMove SHA-1 implementation selection into a header file

Subject: [RFC PATCH] Move SHA-1 implementation selection into a header file

## tl;dr

16 messages between Mar 11, 2017 and Mar 15, 2017. Diffs are folded; open one to read it.

replies: 15people: 5as markdown or json

brian m. carlson· Mar 11, 2017, 22:28 UTC · lore

Many developers use functionality in their editors that allows for quick syntax checks, including warning about questionable constructs. This functionality allows rapid development with fewer errors. However, such functionality generally does not allow the specification of project-specific defines or command-line options.

Since the SHA1_HEADER include is not defined in such a case, developers see spurious errors when using these tools. Furthermore, while using a macro as the argument to #include is permitted by C11, it isn't permitted by C89 and C99, and there are known implementations which reject it.

Instead of using SHA1_HEADER, create a hash.h header and use #if and #elif to select the desired header. Have the Makefile pass an appropriate option to help the header select the right implementation to use.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---

Fixing this makes my development using the vim-ale plugin much nicer. I don't care which implementation is selected by default, as long as *some* implementation is selected by default.

I called this "hash.h" instead of "sha1.h" to allow for future hash function extensions. I was worried that some OS might define a hash.h header as well, but the use of quotation marks instead of angle brackets should cause it to look in the current directory first.

I also picked "SHA1_*" instead of "*_SHA1" as it makes it easier to find all the constants.

 Makefile |  8 ++++----
 cache.h  |  2 +-
 hash.h   | 14 ++++++++++++++
 3 files changed, 19 insertions(+), 5 deletions(-)
 create mode 100644 hash.h
Show changes to 3 files +19 −5

Makefile, cache.h, hash.h

diff --git a/Makefile b/Makefile
index ed68700acb..244eb6a0f2 100644
--- a/Makefile
+++ b/Makefile
@@ -1384,19 +1384,19 @@ ifdef APPLE_COMMON_CRYPTO
 endif
 
 ifdef BLK_SHA1
-	SHA1_HEADER = "block-sha1/sha1.h"
 	LIB_OBJS += block-sha1/sha1.o
+	BASIC_CFLAGS += -DSHA1_BLK
 else
 ifdef PPC_SHA1
-	SHA1_HEADER = "ppc/sha1.h"
 	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
+	BASIC_CFLAGS += -DSHA1_PPC
 else
 ifdef APPLE_COMMON_CRYPTO
 	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
-	SHA1_HEADER = <CommonCrypto/CommonDigest.h>
+	BASIC_CFLAGS += -DSHA1_APPLE
 else
-	SHA1_HEADER = <openssl/sha.h>
 	EXTLIBS += $(LIB_4_CRYPTO)
+	BASIC_CFLAGS += -DSHA1_OPENSSL
 endif
 endif
 endif
diff --git a/cache.h b/cache.h
index 283ab8fb40..6a9afb8561 100644
--- a/cache.h
+++ b/cache.h
@@ -10,8 +10,8 @@
 #include "trace.h"
 #include "string-list.h"
 #include "pack-revindex.h"
+#include "hash.h"
 
-#include SHA1_HEADER
 #ifndef platform_SHA_CTX
 /*
  * platform's underlying implementation of SHA-1; could be OpenSSL,
diff --git a/hash.h b/hash.h
new file mode 100644
index 0000000000..7c6b52835c
--- /dev/null
+++ b/hash.h
@@ -0,0 +1,14 @@
+#ifndef HASH_H
+#define HASH_H
+
+#if defined(SHA1_BLK)
+#include "block-sha1/sha1.h"
+#elif defined(SHA1_PPC)
+#include "ppc/sha1.h"
+#elif defined(SHA1_APPLE)
+#include <CommonCrypto/CommonDigest.h>
+#else /* SHA1_OPENSSL */
+#include <openssl/sha.h>
+#endif
+
+#endif
Jeff King· Mar 12, 2017, 13:01 UTC · re: brian m. carlson · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

On Sat, Mar 11, 2017 at 10:28:18PM +0000, brian m. carlson wrote:
Show 16 quoted lines
> Many developers use functionality in their editors that allows for quick
> syntax checks, including warning about questionable constructs.  This
> functionality allows rapid development with fewer errors.  However, such
> functionality generally does not allow the specification of
> project-specific defines or command-line options.
> 
> Since the SHA1_HEADER include is not defined in such a case, developers
> see spurious errors when using these tools.  Furthermore, while using a
> macro as the argument to #include is permitted by C11, it isn't
> permitted by C89 and C99, and there are known implementations which
> reject it.
> 
> Instead of using SHA1_HEADER, create a hash.h header and use #if
> and #elif to select the desired header.  Have the Makefile pass an
> appropriate option to help the header select the right implementation to
> use.

This has bit me once or twice in the past[1], too, so I'm happy to see somebody tackling it.

Your solution does mean that your tool, whatever it is, that runs without the same CFLAGS as the Makefile may get a _different_ sha1 implementation, though. That's probably good enough for some purposes but perhaps not for others.

The "most correct" solution, I think, would be to stop using "-D" flags in favor of actually generating hash.h. Something like the patch below[2] (though I think it there are some rough edges with the dependencies).

Of course the sha1 header is just one of many such defines. It's the one that is most visible because the result is syntactically valid without it, but anything you compile without the Makefile's CFLAGS may be subtly different than what the Makefile would produce. So arguably the Makefile should be writing out a build-options.h with all of the values, and that should get pulled in by git-compat-util.h.

I don't know if we want to go down that rabbit hole or not. I offer it merely as an alternative. I'm OK with your patch as-is if you don't want to dump any more time into it.

-Peff
[1] I think I hit the problem when trying to debug some internal part of
    git and writing a one-off "foo.c" that calls the code I want. You
    can't compile it with "gcc foo.c".
[2] Here's what a patch for the generated-header might look like:
Show changes to 2 files +8 −4

Makefile, cache.h

diff --git a/Makefile b/Makefile
index 9f0eae428..0d65d50e9 100644
--- a/Makefile
+++ b/Makefile
@@ -690,6 +690,7 @@ XDIFF_LIB = xdiff/lib.a
 VCSSVN_LIB = vcs-svn/lib.a
 
 GENERATED_H += common-cmds.h
+GENERATED_H += hash.h
 
 LIB_H = $(shell $(FIND) . \
 	-name .git -prune -o \
@@ -1639,8 +1640,7 @@ PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
 # from the dependency list, that would make each entry appear twice.
 LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
 
-BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
-	$(COMPAT_CFLAGS)
+BASIC_CFLAGS += $(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
 
 # Quote for C
@@ -1781,7 +1781,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
 		$(filter %.o,$^) $(LIBS)
 
-help.sp help.s help.o: common-cmds.h
+help.sp help.s help.o: common-cmds.h hash.h
 
 builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX
 builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
@@ -1805,6 +1805,10 @@ common-cmds.h: generate-cmdlist.sh command-list.txt
 common-cmds.h: $(wildcard Documentation/git-*.txt)
 	$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ && mv $@+ $@
 
+hash.h:
+	$(QUIET_GEN)echo '#include $(SHA1_HEADER)' >$@+ && \
+	{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; }
+
 SCRIPT_DEFINES = $(SHELL_PATH_SQ):$(DIFF_SQ):$(GIT_VERSION):\
 	$(localedir_SQ):$(NO_CURL):$(USE_GETTEXT_SCHEME):$(SANE_TOOL_PATH_SQ):\
 	$(gitwebdir_SQ):$(PERL_PATH_SQ):$(SANE_TEXT_GREP):$(PAGER_ENV)
diff --git a/cache.h b/cache.h
index 0a14141fc..345104849 100644
--- a/cache.h
+++ b/cache.h
@@ -11,7 +11,7 @@
 #include "string-list.h"
 #include "pack-revindex.h"
 
-#include SHA1_HEADER
+#include "hash.h"
 #ifndef platform_SHA_CTX
 /*
  * platform's underlying implementation of SHA-1; could be OpenSSL,
brian m. carlson· Mar 12, 2017, 16:51 UTC · re: Jeff King · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

On Sun, Mar 12, 2017 at 09:01:49AM -0400, Jeff King wrote:
Show 26 quoted lines
> On Sat, Mar 11, 2017 at 10:28:18PM +0000, brian m. carlson wrote:
> 
> > Many developers use functionality in their editors that allows for quick
> > syntax checks, including warning about questionable constructs.  This
> > functionality allows rapid development with fewer errors.  However, such
> > functionality generally does not allow the specification of
> > project-specific defines or command-line options.
> > 
> > Since the SHA1_HEADER include is not defined in such a case, developers
> > see spurious errors when using these tools.  Furthermore, while using a
> > macro as the argument to #include is permitted by C11, it isn't
> > permitted by C89 and C99, and there are known implementations which
> > reject it.
> > 
> > Instead of using SHA1_HEADER, create a hash.h header and use #if
> > and #elif to select the desired header.  Have the Makefile pass an
> > appropriate option to help the header select the right implementation to
> > use.
> 
> This has bit me once or twice in the past[1], too, so I'm happy to see
> somebody tackling it.
> 
> Your solution does mean that your tool, whatever it is, that runs
> without the same CFLAGS as the Makefile may get a _different_ sha1
> implementation, though. That's probably good enough for some purposes
> but perhaps not for others.

Yeah, my goal was basically to pass -fsyntax-only, not to produce useful object files. My patch does basically require that the user have OpenSSL installed, but I do, so it doesn't matter.

I considered after the fact that I might just do something like:
  #ifdef SHA1_HEADER
  #include SHA1_HEADER
  #else
  #include "block-sha1/sha1.h"
  #endif
That would be the smallest change, but probably not the best.
> The "most correct" solution, I think, would be to stop using "-D" flags
> in favor of actually generating hash.h. Something like the patch
> below[2] (though I think it there are some rough edges with the
> dependencies).

I agree that's a better solution. I was concerned about avoiding ending up rebuilding everything when we regenerated the file, but it looks like you've avoided that with cmp.

Show 10 quoted lines
> Of course the sha1 header is just one of many such defines. It's the one
> that is most visible because the result is syntactically valid without
> it, but anything you compile without the Makefile's CFLAGS may be subtly
> different than what the Makefile would produce. So arguably the Makefile
> should be writing out a build-options.h with all of the values, and that
> should get pulled in by git-compat-util.h.
> 
> I don't know if we want to go down that rabbit hole or not. I offer it
> merely as an alternative. I'm OK with your patch as-is if you don't want
> to dump any more time into it.

I'll take this patch for now and fix it up with the comment I mentioned below. If someone wants to improve the situation down the line, then they can pick that up.

I assume I can apply your sign-off to the resulting patch?
Show 46 quoted lines
> -Peff
> 
> [1] I think I hit the problem when trying to debug some internal part of
>     git and writing a one-off "foo.c" that calls the code I want. You
>     can't compile it with "gcc foo.c".
> 
> [2] Here's what a patch for the generated-header might look like:
> 
> diff --git a/Makefile b/Makefile
> index 9f0eae428..0d65d50e9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -690,6 +690,7 @@ XDIFF_LIB = xdiff/lib.a
>  VCSSVN_LIB = vcs-svn/lib.a
>  
>  GENERATED_H += common-cmds.h
> +GENERATED_H += hash.h
>  
>  LIB_H = $(shell $(FIND) . \
>  	-name .git -prune -o \
> @@ -1639,8 +1640,7 @@ PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
>  # from the dependency list, that would make each entry appear twice.
>  LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
>  
> -BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
> -	$(COMPAT_CFLAGS)
> +BASIC_CFLAGS += $(COMPAT_CFLAGS)
>  LIB_OBJS += $(COMPAT_OBJS)
>  
>  # Quote for C
> @@ -1781,7 +1781,7 @@ git$X: git.o GIT-LDFLAGS $(BUILTIN_OBJS) $(GITLIBS)
>  	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) \
>  		$(filter %.o,$^) $(LIBS)
>  
> -help.sp help.s help.o: common-cmds.h
> +help.sp help.s help.o: common-cmds.h hash.h
>  
>  builtin/help.sp builtin/help.s builtin/help.o: common-cmds.h GIT-PREFIX
>  builtin/help.sp builtin/help.s builtin/help.o: EXTRA_CPPFLAGS = \
> @@ -1805,6 +1805,10 @@ common-cmds.h: generate-cmdlist.sh command-list.txt
>  common-cmds.h: $(wildcard Documentation/git-*.txt)
>  	$(QUIET_GEN)$(SHELL_PATH) ./generate-cmdlist.sh command-list.txt >$@+ && mv $@+ $@
>  
> +hash.h:
> +	$(QUIET_GEN)echo '#include $(SHA1_HEADER)' >$@+ && \
> +	{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; }

I think here we'd want to also "rm -f $@+", so that we don't leave an extra file behind if we're up-to-date (which is the common case), much like we do for GIT-BUILD-OPTIONS.

-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
Jeff King· Mar 12, 2017, 20:12 UTC · re: brian m. carlson · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

On Sun, Mar 12, 2017 at 04:51:19PM +0000, brian m. carlson wrote:
Show 13 quoted lines
> Yeah, my goal was basically to pass -fsyntax-only, not to produce useful
> object files.  My patch does basically require that the user have
> OpenSSL installed, but I do, so it doesn't matter.
> 
> I considered after the fact that I might just do something like:
> 
>   #ifdef SHA1_HEADER
>   #include SHA1_HEADER
>   #else
>   #include "block-sha1/sha1.h"
>   #endif
> 
> That would be the smallest change, but probably not the best.

Yeah, if there is going to be a fallback it probably ought to be the internal one.

Show 16 quoted lines
> > Of course the sha1 header is just one of many such defines. It's the one
> > that is most visible because the result is syntactically valid without
> > it, but anything you compile without the Makefile's CFLAGS may be subtly
> > different than what the Makefile would produce. So arguably the Makefile
> > should be writing out a build-options.h with all of the values, and that
> > should get pulled in by git-compat-util.h.
> > 
> > I don't know if we want to go down that rabbit hole or not. I offer it
> > merely as an alternative. I'm OK with your patch as-is if you don't want
> > to dump any more time into it.
> 
> I'll take this patch for now and fix it up with the comment I mentioned
> below.  If someone wants to improve the situation down the line, then
> they can pick that up.
> 
> I assume I can apply your sign-off to the resulting patch?

Yes, it is OK to add my sign-off. The two things I was concerned about with my patch were:

  1. It does introduce an extra shell invocation on every "make", even
     if the file does not need to be rebuilt (though it is just one of
     many; GIT-BUILD-OPTIONS, etc).
  2. I wasn't sure if the dependencies were quite right. I _thought_ we
     should pick it up as an auto-dependency, but I don't think that
     works because we do our header dependencies as a side effect of the
     compile.
     So "make" didn't actually know to build hash.h until I made it a
     dependency of help.o. Which feels weird and hacky. It's really a
     dependency of _anything_ that includes cache.h.

So you may want to dig into that second one to make sure the result is sane, not racy, etc.

Show 7 quoted lines
> > +hash.h:
> > +	$(QUIET_GEN)echo '#include $(SHA1_HEADER)' >$@+ && \
> > +	{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; }
> 
> I think here we'd want to also "rm -f $@+", so that we don't leave an
> extra file behind if we're up-to-date (which is the common case), much
> like we do for GIT-BUILD-OPTIONS.
Yeah, I agree that would be an improvement.
-Peff
Junio C Hamano· Mar 12, 2017, 17:54 UTC · re: Jeff King · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

Jeff King <peff@peff.net> writes:
Show 18 quoted lines
> diff --git a/Makefile b/Makefile
> index 9f0eae428..0d65d50e9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -690,6 +690,7 @@ XDIFF_LIB = xdiff/lib.a
>  VCSSVN_LIB = vcs-svn/lib.a
>  
>  GENERATED_H += common-cmds.h
> +GENERATED_H += hash.h
> ...
> -BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
> -	$(COMPAT_CFLAGS)
> +BASIC_CFLAGS += $(COMPAT_CFLAGS)
>   ...
> +hash.h:
> +	$(QUIET_GEN)echo '#include $(SHA1_HEADER)' >$@+ && \
> +	{ cmp $@+ $@ >/dev/null 2>/dev/null || mv $@+ $@; }
> +

That looks sensible. We do not have to adjust the code to update GIT-BUILD-OPTIONS and friends in this patch because the current way to force rebuilding when SHA1_HEADER changes is by letting GIT-CFLAGS file notice the change, and the new "hash.h" itself will let the $(MAKE) notice if a different hash implementation is chosen for the build.

Jonathan Nieder· Mar 14, 2017, 18:41 UTC · re: brian m. carlson · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

Hi,
brian m. carlson wrote:
[...]
Show 9 quoted lines
> --- a/cache.h
> +++ b/cache.h
> @@ -10,8 +10,8 @@
>  #include "trace.h"
>  #include "string-list.h"
>  #include "pack-revindex.h"
> +#include "hash.h"
>  
> -#include SHA1_HEADER

For what it's worth, the bazel build tool doesn't like this '#include SHA1_HEADER' either. Your fix looks like a straightforward fix and we never encouraged directly customizing SHA1_HEADER.

The other approaches discussed may also work but they don't add anything for my application (nor yours, I'd think). Conditional #includes are a pretty normal thing so I am fine with this more straightforward change. So

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

That said, if someone is excited about one of the other approaches then I don't object to them.

Thanks, Jonathan

Jeff King· Mar 14, 2017, 20:14 UTC · re: Jonathan Nieder · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

On Tue, Mar 14, 2017 at 11:41:26AM -0700, Jonathan Nieder wrote:
Show 16 quoted lines
> brian m. carlson wrote:
> 
> [...]
> > --- a/cache.h
> > +++ b/cache.h
> > @@ -10,8 +10,8 @@
> >  #include "trace.h"
> >  #include "string-list.h"
> >  #include "pack-revindex.h"
> > +#include "hash.h"
> >  
> > -#include SHA1_HEADER
> 
> For what it's worth, the bazel build tool doesn't like this
> '#include SHA1_HEADER' either.  Your fix looks like a straightforward
> fix and we never encouraged directly customizing SHA1_HEADER.

Hmm. I don't know how you're using bazel with git, but if it is doing something like generating header dependencies, would that mean that it potentially picks up the wrong dependency with brian's patch?

Show 9 quoted lines
> The other approaches discussed may also work but they don't add
> anything for my application (nor yours, I'd think).  Conditional
> #includes are a pretty normal thing so I am fine with this more
> straightforward change.  So
> 
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> 
> That said, if someone is excited about one of the other approaches
> then I don't object to them.

My biggest complaint with the initial patch is that it repeats the if-else chain of each type (once in the Makefile and once in hash.h). I can live with having to touch two parts of the code (it's not like we expect a huge number of alternative sha1 implementations), but I worried that we were replicating the "which one is primary" logic in two places (i.e., the Makefile prefers BLK_SHA1 above others, and I expect that when we add USE_SHA1DC it may become the primary).

But I think it's probably OK in practice. The if-else inside hash.h is checking the -D defines we set in the Makefile, and the Makefile logic would set only one such define. So hash.h would have to follow whatever the Makefile picked (unless you do something idiotic like "CFLAGS += -DBLK_SHA1" yourself, but then you deserve every bad thing that comes your way).

So I'm OK with brian's patch as the simplest thing that covers non-compilation use cases. It should probably default to BLK_SHA1 rather than OpenSSL, as discussed earlier.

-Peff
Junio C Hamano· Mar 14, 2017, 20:44 UTC · re: Jeff King · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

OK, then I'll queue this. The selection still goes to BASIC_CFLAGS so the dependencies for re-compilation should be right, I'd think.

-- >8 --
From: "brian m. carlson" <sandals@crustytoothpaste.net>
Date: Sat, 11 Mar 2017 22:28:18 +0000
Subject: [PATCH] hash.h: move SHA-1 implementation selection into a header file

Many developers use functionality in their editors that allows for quick syntax checks, including warning about questionable constructs. This functionality allows rapid development with fewer errors. However, such functionality generally does not allow the specification of project-specific defines or command-line options.

Since the SHA1_HEADER include is not defined in such a case, developers see spurious errors when using these tools. Furthermore, while using a macro as the argument to #include is permitted by C11, it isn't permitted by C89 and C99, and there are known implementations which reject it.

Instead of using SHA1_HEADER, create a hash.h header and use #if and #elif to select the desired header. Have the Makefile pass an appropriate option to help the header select the right implementation to use.

[jc: make BLK_SHA1 the fallback default as discussed on list, e.g. <20170314201424.vccij5z2ortq4a4o@sigill.intra.peff.net>; also remove SHA1_HEADER and SHA1_HEADER_SQ that are no longer used].

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Makefile | 12 +++++-------
 cache.h  |  2 +-
 hash.h   | 14 ++++++++++++++
 3 files changed, 20 insertions(+), 8 deletions(-)
Show changes to 3 files +20 −8

Makefile, cache.h, hash.h

diff --git a/Makefile b/Makefile
index 8e4081e061..25c21f08b1 100644
--- a/Makefile
+++ b/Makefile
@@ -1387,19 +1387,19 @@ ifdef APPLE_COMMON_CRYPTO
 endif
 
 ifdef BLK_SHA1
-	SHA1_HEADER = "block-sha1/sha1.h"
 	LIB_OBJS += block-sha1/sha1.o
+	BASIC_CFLAGS += -DSHA1_BLK
 else
 ifdef PPC_SHA1
-	SHA1_HEADER = "ppc/sha1.h"
 	LIB_OBJS += ppc/sha1.o ppc/sha1ppc.o
+	BASIC_CFLAGS += -DSHA1_PPC
 else
 ifdef APPLE_COMMON_CRYPTO
 	COMPAT_CFLAGS += -DCOMMON_DIGEST_FOR_OPENSSL
-	SHA1_HEADER = <CommonCrypto/CommonDigest.h>
+	BASIC_CFLAGS += -DSHA1_APPLE
 else
-	SHA1_HEADER = <openssl/sha.h>
 	EXTLIBS += $(LIB_4_CRYPTO)
+	BASIC_CFLAGS += -DSHA1_OPENSSL
 endif
 endif
 endif
@@ -1591,7 +1591,6 @@ endif
 
 # Shell quote (do not use $(call) to accommodate ancient setups);
 
-SHA1_HEADER_SQ = $(subst ','\'',$(SHA1_HEADER))
 ETC_GITCONFIG_SQ = $(subst ','\'',$(ETC_GITCONFIG))
 ETC_GITATTRIBUTES_SQ = $(subst ','\'',$(ETC_GITATTRIBUTES))
 
@@ -1624,8 +1623,7 @@ PERLLIB_EXTRA_SQ = $(subst ','\'',$(PERLLIB_EXTRA))
 # from the dependency list, that would make each entry appear twice.
 LIBS = $(filter-out %.o, $(GITLIBS)) $(EXTLIBS)
 
-BASIC_CFLAGS += -DSHA1_HEADER='$(SHA1_HEADER_SQ)' \
-	$(COMPAT_CFLAGS)
+BASIC_CFLAGS += $(COMPAT_CFLAGS)
 LIB_OBJS += $(COMPAT_OBJS)
 
 # Quote for C
diff --git a/cache.h b/cache.h
index 61fc86e6d7..f98c95bf2a 100644
--- a/cache.h
+++ b/cache.h
@@ -10,8 +10,8 @@
 #include "trace.h"
 #include "string-list.h"
 #include "pack-revindex.h"
+#include "hash.h"
 
-#include SHA1_HEADER
 #ifndef platform_SHA_CTX
 /*
  * platform's underlying implementation of SHA-1; could be OpenSSL,
diff --git a/hash.h b/hash.h
new file mode 100644
index 0000000000..f0d9ddd0c2
--- /dev/null
+++ b/hash.h
@@ -0,0 +1,14 @@
+#ifndef HASH_H
+#define HASH_H
+
+#if defined(SHA1_PPC)
+#include "ppc/sha1.h"
+#elif defined(SHA1_APPLE)
+#include <CommonCrypto/CommonDigest.h>
+#elif defined(SHA1_OPENSSL)
+#include <openssl/sha.h>
+#else /* SHA1_BLK */
+#include "block-sha1/sha1.h"
+#endif
+
+#endif
Jeff King· Mar 14, 2017, 21:26 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

On Tue, Mar 14, 2017 at 01:44:48PM -0700, Junio C Hamano wrote:
> OK, then I'll queue this.  The selection still goes to BASIC_CFLAGS
> so the dependencies for re-compilation should be right, I'd think.
Yeah, I think that part should be fine.
> -- >8 --
> From: "brian m. carlson" <sandals@crustytoothpaste.net>
> Date: Sat, 11 Mar 2017 22:28:18 +0000
> Subject: [PATCH] hash.h: move SHA-1 implementation selection into a header file
Looks good to me.
Reviewed-by: Jeff King <peff@peff.net>
-Peff
Jonathan Nieder· Mar 14, 2017, 21:50 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

Junio C Hamano wrote:
Show 6 quoted lines
> [jc: make BLK_SHA1 the fallback default as discussed on list,
> e.g. <20170314201424.vccij5z2ortq4a4o@sigill.intra.peff.net>; also
> remove SHA1_HEADER and SHA1_HEADER_SQ that are no longer used].
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks for taking care of it.
Ramsay Jones· Mar 14, 2017, 23:42 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

On 14/03/17 20:44, Junio C Hamano wrote:
Show 19 quoted lines
> OK, then I'll queue this.  The selection still goes to BASIC_CFLAGS
> so the dependencies for re-compilation should be right, I'd think.
> 
> -- >8 --
> From: "brian m. carlson" <sandals@crustytoothpaste.net>
> Date: Sat, 11 Mar 2017 22:28:18 +0000
> Subject: [PATCH] hash.h: move SHA-1 implementation selection into a header file
> 
> Many developers use functionality in their editors that allows for quick
> syntax checks, including warning about questionable constructs.  This
> functionality allows rapid development with fewer errors.  However, such
> functionality generally does not allow the specification of
> project-specific defines or command-line options.
> 
> Since the SHA1_HEADER include is not defined in such a case, developers
> see spurious errors when using these tools.  Furthermore, while using a
> macro as the argument to #include is permitted by C11, it isn't
> permitted by C89 and C99, and there are known implementations which
> reject it.

C99 certainly allows a macro argument to #include (see, 6.10.2-4; there is also an example in 6.10.2-8).

I can't remember if it's allowed in C89/C90 (I think it is). I only have immediate access to the C99 and C11 standards (and I can't be bothered to search), so I can't say for sure.

ATB, Ramsay Jones

brian m. carlson· Mar 14, 2017, 23:46 UTC · re: Ramsay Jones · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

On Tue, Mar 14, 2017 at 11:42:20PM +0000, Ramsay Jones wrote:
Show 29 quoted lines
> 
> 
> On 14/03/17 20:44, Junio C Hamano wrote:
> > OK, then I'll queue this.  The selection still goes to BASIC_CFLAGS
> > so the dependencies for re-compilation should be right, I'd think.
> > 
> > -- >8 --
> > From: "brian m. carlson" <sandals@crustytoothpaste.net>
> > Date: Sat, 11 Mar 2017 22:28:18 +0000
> > Subject: [PATCH] hash.h: move SHA-1 implementation selection into a header file
> > 
> > Many developers use functionality in their editors that allows for quick
> > syntax checks, including warning about questionable constructs.  This
> > functionality allows rapid development with fewer errors.  However, such
> > functionality generally does not allow the specification of
> > project-specific defines or command-line options.
> > 
> > Since the SHA1_HEADER include is not defined in such a case, developers
> > see spurious errors when using these tools.  Furthermore, while using a
> > macro as the argument to #include is permitted by C11, it isn't
> > permitted by C89 and C99, and there are known implementations which
> > reject it.
> 
> C99 certainly allows a macro argument to #include (see, 6.10.2-4; there
> is also an example in 6.10.2-8).
> 
> I can't remember if it's allowed in C89/C90 (I think it is). I only
> have immediate access to the C99 and C11 standards (and I can't be
> bothered to search), so I can't say for sure.

You're right. I only have access to N1124 (the C99 final draft), but it does allow that. I could have sworn it was new in C11. I'm pretty certain it isn't allowed in C89, but I don't have access to that standard.

I know there have been reasonably standards-conforming compilers that have rejected it in the past, but I can't remember which ones (I think they were for proprietary Unices).

Junio, do you want to amend the commit message before you merge it?
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204
Ramsay Jones· Mar 15, 2017, 00:15 UTC · re: brian m. carlson · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

On 14/03/17 23:46, brian m. carlson wrote:
Show 35 quoted lines
> On Tue, Mar 14, 2017 at 11:42:20PM +0000, Ramsay Jones wrote:
>>
>>
>> On 14/03/17 20:44, Junio C Hamano wrote:
>>> OK, then I'll queue this.  The selection still goes to BASIC_CFLAGS
>>> so the dependencies for re-compilation should be right, I'd think.
>>>
>>> -- >8 --
>>> From: "brian m. carlson" <sandals@crustytoothpaste.net>
>>> Date: Sat, 11 Mar 2017 22:28:18 +0000
>>> Subject: [PATCH] hash.h: move SHA-1 implementation selection into a header file
>>>
>>> Many developers use functionality in their editors that allows for quick
>>> syntax checks, including warning about questionable constructs.  This
>>> functionality allows rapid development with fewer errors.  However, such
>>> functionality generally does not allow the specification of
>>> project-specific defines or command-line options.
>>>
>>> Since the SHA1_HEADER include is not defined in such a case, developers
>>> see spurious errors when using these tools.  Furthermore, while using a
>>> macro as the argument to #include is permitted by C11, it isn't
>>> permitted by C89 and C99, and there are known implementations which
>>> reject it.
>>
>> C99 certainly allows a macro argument to #include (see, 6.10.2-4; there
>> is also an example in 6.10.2-8).
>>
>> I can't remember if it's allowed in C89/C90 (I think it is). I only
>> have immediate access to the C99 and C11 standards (and I can't be
>> bothered to search), so I can't say for sure.
> 
> You're right.  I only have access to N1124 (the C99 final draft), but it
> does allow that.  I could have sworn it was new in C11.  I'm pretty
> certain it isn't allowed in C89, but I don't have access to that
> standard.

My copies of Harbison and Steele (Third and Fifth editions) claim that Standard C supports it also (by which they mean C89/C90).

> I know there have been reasonably standards-conforming compilers that
> have rejected it in the past, but I can't remember which ones (I think
> they were for proprietary Unices).
Yes, I think that happened to me on Irix, if I recall correctly.
> Junio, do you want to amend the commit message before you merge it?
Yes, please! ;-)

ATB, Ramsay Jones

Junio C Hamano· Mar 15, 2017, 15:57 UTC · re: Ramsay Jones · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
Show 12 quoted lines
> On 14/03/17 23:46, brian m. carlson wrote:
>>>>
>>>> Since the SHA1_HEADER include is not defined in such a case, developers
>>>> see spurious errors when using these tools.  Furthermore, while using a
>>>> macro as the argument to #include is permitted by C11, it isn't
>>>> permitted by C89 and C99, and there are known implementations which
>>>> reject it.
>>>
>
>> Junio, do you want to amend the commit message before you merge it?
>
> Yes, please! ;-)
If only you were a few hours quicker.

Let me see how bad the fallout is to revert the merge to 'next' and merge an amended version in.

I _think_ the whole "Furthermore" sentence can go, since nobody complained since cef661fc ("Add support for alternate SHA1 library implementations.", 2005-04-21) started using the Makefile construct.

Thanks.
Ramsay Jones· Mar 15, 2017, 19:48 UTC · re: Junio C Hamano · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

On 15/03/17 15:57, Junio C Hamano wrote:
Show 16 quoted lines
> Ramsay Jones <ramsay@ramsayjones.plus.com> writes:
> 
>> On 14/03/17 23:46, brian m. carlson wrote:
>>>>>
>>>>> Since the SHA1_HEADER include is not defined in such a case, developers
>>>>> see spurious errors when using these tools.  Furthermore, while using a
>>>>> macro as the argument to #include is permitted by C11, it isn't
>>>>> permitted by C89 and C99, and there are known implementations which
>>>>> reject it.
>>>>
>>
>>> Junio, do you want to amend the commit message before you merge it?
>>
>> Yes, please! ;-)
> 
> If only you were a few hours quicker.
Oops, sorry. When I wrote that I didn't know it was already in 'next'.

[I tend not to reply to emails as soon as I read them (because it often gets me into trouble!), but wait until I've read everything in my inbox. Unfortunately, I get so much email, it can be hours later before I respond ... (I keep saying that I will unsubscribe from some mailing lists ;-) ).]

> Let me see how bad the fallout is to revert the merge to 'next' and
> merge an amended version in.

Hmm, I didn't intend to add to your workload! Is it worth the hassle? In the great scheme of things, it's not a major issue. I dunno.

> I _think_ the whole "Furthermore" sentence can go, since nobody
> complained since cef661fc ("Add support for alternate SHA1 library
> implementations.", 2005-04-21) started using the Makefile construct.
Yep.

[BTW, I haven't finished reading everything in my inbox yet, hopefully I won't get into trouble. :P ]

ATB, Ramsay Jones

Jonathan Nieder· Mar 14, 2017, 21:56 UTC · re: Jeff King · lore

Re: [RFC PATCH] Move SHA-1 implementation selection into a header file

Jeff King wrote:
> On Tue, Mar 14, 2017 at 11:41:26AM -0700, Jonathan Nieder wrote:
>> brian m. carlson wrote:
Show 17 quoted lines
>>> --- a/cache.h
>>> +++ b/cache.h
>>> @@ -10,8 +10,8 @@
>>>  #include "trace.h"
>>>  #include "string-list.h"
>>>  #include "pack-revindex.h"
>>> +#include "hash.h"
>>>
>>> -#include SHA1_HEADER
>>
>> For what it's worth, the bazel build tool doesn't like this
>> '#include SHA1_HEADER' either.  Your fix looks like a straightforward
>> fix and we never encouraged directly customizing SHA1_HEADER.
>
> Hmm. I don't know how you're using bazel with git, but if it is doing
> something like generating header dependencies, would that mean that it
> potentially picks up the wrong dependency with brian's patch?

I believe it picks up all options as dependencies, which is good enough for me.

I have a custom BUILD file to build git with bazel. I like the reliable dependencies it generates (unless you do heavy contortions, files aren't available to the build commands unless the dependency is declared) and fast, parallel build with simple progress output. But keeping it up to date with every patch that changes the Makefile is not something I would wish on the git project.

One of these days I'd like to try making a tool to automatically generate the BUILD file, like contrib/buildsystems generates a Visual C project.

Regards, Jonathan

← back to recent threads