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

[Jeff King <peff@peff.net>]

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,