| Message ID | 20250328170121.157563-2-jltobler@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | help: include SHA build options in version info | expand |
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
On Sat, Mar 29, 2025 at 04:36:45AM -0700, Junio C Hamano wrote: > 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. 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
On 25/03/29 04:36AM, Junio C Hamano wrote: > 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. Good point! Both hash.h and the Makefile have their own precedence defined which makes this even more confusing to understand what we would should use to keep things in sync. > 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. Keeping each of the options independent certainly keeps things more simple and avoids having to also manage the precedence of each option here. For most of these SHA related options we only expect one to be set at a time anyway. I'll do this in the next version. One exception though is that we do expect that when SHA1_DC is set, either DC_SHA1_SUBMODULE or DC_SHA1_EXTERNAL may also be set. In this scenario, maybe it would be fine for the printed build options to include both. Another option would be to treat the DC_SHA1_* options separately from the SHA1_* ones and have a separate prefix key. Maybe something like `DC-SHA-1: {DC_SHA1_SUBMODULE,DC_SHA1_EXTERNAL}`. I'm currently leaning towards the latter option here. > 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 Ya, using the internal symbol instead of a new human readable name is a better idea. I'll update in the next version. Thanks, -Justin
On 25/03/31 09:19AM, Patrick Steinhardt wrote: > On Sat, Mar 29, 2025 at 04:36:45AM -0700, Junio C Hamano wrote: > > 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. The SHA backends get selected in hash.h, so we could conditionally define symbol values based on the backend that gets selected there. This has the benefit of centralizing backend selection in one place and the printed build options could just depend on that. I'll implement this approach instead in the next version. -Justin
Patrick Steinhardt <ps@pks.im> writes: > 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. Much simpler and less error prone.
diff --git a/help.c b/help.c index c54bd9918a..32b5d4e6f5 100644 --- a/help.c +++ b/help.c @@ -768,6 +768,33 @@ char *help_unknown_cmd(const char *cmd) exit(1); } +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 +} + void get_version_info(struct strbuf *buf, int show_build_options) { /* @@ -803,6 +830,7 @@ void get_version_info(struct strbuf *buf, int show_build_options) #elif defined ZLIB_VERSION strbuf_addf(buf, "zlib: %s\n", ZLIB_VERSION); #endif + get_sha_impl(buf); } }
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. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- help.c | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+)