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

> 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