On 25/03/31 09:19AM, Patrick Steinhardt wrote:
> On Sat, Mar 29, 2025 at 04:36:45AM -0700, Junio C Hamano wrote:
> > While I agree with the objective of the change, I am not sure how I
> > feel about the implementation. Given that
> >
> > - The code here, and probably the existing code paths that depend
> > on these SHA1_$WHOSE symbols, assume that only one of them is
> > defined;
> >
> > - The "git help --build-options" is not an end-user thing but more
> > is a developer thing.
> >
> > The thing I am most worried about is that it is unclear how the
> > order in which the SHA1_$WHOSE symbols are inspected here and
> > elsewhere in the code are kept in sync. What happens when, for
> > example, SHA1_OPENSSL and SHA1_APPLE_UNSAFE are both defined? The
> > above code will report that we are using SHA1_OPENSSL, but hash.h
> > would probably use SHA1_APPLE as it has its own if/elif/endif
> > cascade.
> >
> > Perhaps it does not matter, if the build infrastructure ensures that
> > the build fails unless one and only one of SHA1_$WHOSE is defined.
> >
> > But with the way how this part is written with an if/elif/endif
> > cascade, it makes readers spend time wondering how the precedence
> > order here is kept in sync throughout the system. If I am not
> > mistaken, the top-level Makefile has its own ifdef/else/if/endif*
> > cascade.
> >
> > I imagine that making all of the above not if/elif/endif chain, but
> > make them pretend as if they are independent and orthogonal choices,
> > would make it simpler to understand and also it will help us catch a
> > misconfiguration where more than one is defined, i.e.
> >
> > static void get_sha_impl(struct strbuf *buf)
> > {
> > #if defined(SHA1_OPENSSL)
> > strbuf_addstr(buf, "SHA-1: OpenSSL\n");
> > #endif
> > #if defined(SHA1_BLK)
> > strbuf_addstr(buf, "SHA-1: blk\n");
> > #endif
> > #if defined(SHA1_APPLE)
> > ...
> >
> >
> > That way, we wouldn't force future devlopers who are plugging new
> > implementations of SHA-256 wonder where is the right place in the
> > existing if/elif/endif cascade their new one fits. It also allows
> > us to catch misconfigurations to define more then one of them at the
> > same time, if such a thing becomes ever possible.
>
> Another option: we could ask the implementations themselves to define a
> symbol `SHA1_BACKEND` and use it here. This would automatically ensure
> that any implementation must define the symbol as we'd otherwise get a
> compile error. We could also conditionally define `SHA1_UNSAFE_BACKEND`
> depending on whether or not we have it.
The SHA backends get selected in hash.h, so we could conditionally
define symbol values based on the backend that gets selected there. This
has the benefit of centralizing backend selection in one place and the
printed build options could just depend on that.
I'll implement this approach instead in the next version.
-Justin
