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

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

[brian m. carlson]

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

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

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

[Jeff King]

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.

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:

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,

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

[brian m. carlson]

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

On Sun, Mar 12, 2017 at 09:01:49AM -0400, Jeff King wrote:
> 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.

> 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?

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

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

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

[Jeff King]

On Sun, Mar 12, 2017 at 04:51:19PM +0000, brian m. carlson wrote:

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

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

> > +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

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

[Jonathan Nieder]

Hi,

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.

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

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

[Jeff King]

On Tue, Mar 14, 2017 at 11:41:26AM -0700, Jonathan Nieder wrote:

> 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?

> 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

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

[Junio C Hamano]

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(-)

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

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

[Jeff King]

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

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

[Jonathan Nieder]

Junio C Hamano wrote:

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

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

[Jonathan Nieder]

Jeff King wrote:
> On Tue, Mar 14, 2017 at 11:41:26AM -0700, Jonathan Nieder wrote:
>> 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?

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

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

[Ramsay Jones]

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.

ATB,
Ramsay Jones

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

[brian m. carlson]

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

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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

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

[Ramsay Jones]

On 14/03/17 23:46, brian m. carlson wrote:
> 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

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

[Junio C Hamano]

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.

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.

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

[Ramsay Jones]

On 15/03/17 15:57, Junio C Hamano wrote:
> 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