On Sat, Mar 29, 2025 at 04:36:45AM -0700, Junio C Hamano wrote:
> Justin Tobler <jltobler@xxxxxxxxx> writes:
>
> > When the `--build-options` flag is used with git-version(1), additional
> > information about the built version of Git is printed. During build
> > time, different SHA implementations may be configured, but this
> > information is not included in the version info.
> >
> > Add the SHA implementations Git is built with to the version info.
> > ...
> > +static void get_sha_impl(struct strbuf *buf)
> > +{
> > +#if defined(SHA1_OPENSSL)
> > + strbuf_addstr(buf, "SHA-1: OpenSSL\n");
> > +#elif defined(SHA1_BLK)
> > + strbuf_addstr(buf, "SHA-1: blk\n");
> > +#elif defined(SHA1_APPLE)
> > + strbuf_addstr(buf, "SHA-1: Apple CommonCrypto\n");
> > +#elif defined(DC_SHA1_EXTERNAL)
> > + strbuf_addstr(buf, "SHA-1: Collision Detection (External)\n");
> > +#elif defined(DC_SHA1_SUBMODULE)
> > + strbuf_addstr(buf, "SHA-1: Collision Detection (Submodule)\n");
> > +#elif defined(SHA1_DC)
> > + strbuf_addstr(buf, "SHA-1: Collision Detection\n");
> > +#endif
> > +
> > +#if defined(SHA256_OPENSSL)
> > + strbuf_addstr(buf, "SHA-256: OpenSSL\n");
> > +#elif defined(SHA256_NETTLE)
> > + strbuf_addstr(buf, "SHA-256: Nettle\n");
> > +#elif defined(SHA256_GCRYPT)
> > + strbuf_addstr(buf, "SHA-256: gcrypt\n");
> > +#elif defined(SHA256_BLK)
> > + strbuf_addstr(buf, "SHA-256: blk\n");
> > +#endif
> > +}
>
> 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.
Patrick
