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

[Junio C Hamano <gitster@pobox.com>]

Justin Tobler <jltobler@gmail.com> 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.

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