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