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