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

[Jeff King <peff@peff.net>]

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