Re: [PATCH 1/2] help: include SHA implementation in version info

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 29, 2025, 11:36 UTC
Message-ID: <xmqq8qoodq5u.fsf@gitster.g>
In-Reply-To: <20250328170121.157563-2-jltobler@gmail.com>

Justin Tobler <jltobler@gmail.com> writes:

Show 33 quoted lines

> 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.

Also, wouldn't it make more sense to just reuse the internal symbol for reporting, i.e.

	strbuf_addstr(buf, "SHA-1: SHA1_OPENSSL\n");

instead of having to come up with "human readable" name here