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

From: Patrick Steinhardt <ps@pks.im>
Date: Apr 2, 2025, 07:38 UTC
Message-ID: <Z-zpY3D01Fg4q0Je@pks.im>
In-Reply-To: <20250401203630.285451-2-jltobler@gmail.com>

On Tue, Apr 01, 2025 at 03:36:29PM -0500, Justin Tobler wrote:

Show 14 quoted lines

> diff --git a/Documentation/git-version.adoc b/Documentation/git-version.adoc
> index 80fa7754a6..f06758a7cf 100644
> --- a/Documentation/git-version.adoc
> +++ b/Documentation/git-version.adoc
> @@ -22,6 +22,9 @@ OPTIONS
>  --build-options::
>  	Include additional information about how git was built for diagnostic
>  	purposes.
> ++
> +Note that the SHA1 options `SHA1_APPLE`, `SHA1_OPENSSL`, and `SHA1_BLK` do not
> +have collision detection.
>  
>  GIT
>  ---

I think this note is somewhat funny for an unsuspecting reader. On the one hand they're going to be puzzled why you're talking about SHA1 in the first place because it isn't mentioned at all beforehand. And on the other hand they will wonder what collision detection even is in the first place.

So I would either drop this paragraph completely or expand it to give a bit more context.

Show 25 quoted lines

> diff --git a/hash.h b/hash.h
> index 4367acfec5..51cd0ec7b6 100644
> --- a/hash.h
> +++ b/hash.h
> @@ -2,16 +2,20 @@
>  #define HASH_H
>  
>  #if defined(SHA1_APPLE)
> +#define SHA1_BACKEND "SHA1_APPLE (No collision detection)"
>  #include <CommonCrypto/CommonDigest.h>
>  #elif defined(SHA1_OPENSSL)
> +#  define SHA1_BACKEND "SHA1_OPENSSL (No collision detection)"
>  #  include <openssl/sha.h>
>  #  if defined(OPENSSL_API_LEVEL) && OPENSSL_API_LEVEL >= 3
>  #    define SHA1_NEEDS_CLONE_HELPER
>  #    include "sha1/openssl.h"
>  #  endif
>  #elif defined(SHA1_DC)
> +#define SHA1_BACKEND "SHA1_DC"
>  #include "sha1dc_git.h"
>  #else /* SHA1_BLK */
> +#define SHA1_BACKEND "SHA1_BLK (No collision detection)"
>  #include "block-sha1/sha1.h"
>  #endif
>

This feels way less fragile indeed, thanks for adapting.

Show 22 quoted lines

> diff --git a/help.c b/help.c
> index c54bd9918a..3aebfb3681 100644
> --- a/help.c
> +++ b/help.c
> @@ -768,6 +769,12 @@ char *help_unknown_cmd(const char *cmd)
>  	exit(1);
>  }
>  
> +static void get_sha_impl(struct strbuf *buf)
> +{
> +	strbuf_addf(buf, "SHA-1: %s\n", SHA1_BACKEND);
> +	strbuf_addf(buf, "SHA-256: %s\n", SHA256_BACKEND);
> +}
> +
>  void get_version_info(struct strbuf *buf, int show_build_options)
>  {
>  	/*
> @@ -803,6 +810,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);

I don't quite see the need for a new function, but don't mind it too much, either.

Patrick