Re: [PATCH 2/2] avoid SHA-1 functions deprecated in OpenSSL 3+

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

Eric Wong <e@80x24.org> writes:

> diff --git a/hash-ll.h b/hash-ll.h
> index 087b421bd5..10d84cc208 100644
> --- a/hash-ll.h
> +++ b/hash-ll.h
> @@ -45,6 +49,10 @@
>  #define git_SHA1_Update		platform_SHA1_Update
>  #define git_SHA1_Final		platform_SHA1_Final
>  
> +#ifdef platform_SHA1_Clone
> +#define git_SHA1_Clone	platform_SHA1_Clone
> +#endif
> +
> ...
> +#ifndef SHA1_NEEDS_CLONE_HELPER
>  static inline void git_SHA1_Clone(git_SHA_CTX *dst, const git_SHA_CTX *src)
>  {
>  	memcpy(dst, src, sizeof(*dst));
>  }
> +#endif

This smelled a bit strange in that all the other platform_* stuff is
"if a platform sha-1 header implements platform_SHA1_*, we will use
it to define git_SHA1_* (which is the symbol we use in the code)"
plus its inverse "if there is no specific platform_SHA1_*, we assume
OpenSSL compatible ones and use them as platform_SHA1_* (which in
turn will be used as git_SHA1-*)".

And that is why "#ifndef platform_SHA1_CTX" block gave us default
values for them.  And from that point of view, the first hunk
(i.e. "if SHA1_CLONE is defined for the platform, we use it") is
entirely sensible.

But I did not get why we guard the other hunk with a different CPP
macro.  If we have platform_SHA1_Clone already defined, and then
NEEDS_CLONE_HELPER not defined, we end up creating an static inline
platform_SHA1_CLONE here, and I was not sure if that is what we
wanted to do.

The answer to the above puzzle (at least it was a puzzle to me) is
that the new header "sha1/openssl.h" added by this series does have
platform_SHA1_Clone defined, and the code that includes it define
NEEDS_CLONE_HELPER to avoid this "static inline", so the CPP macro
SHA1_NEEDS_CLONE_HELPER means "we need more than just a straight
bitwise copy to clone the SHA context, which is provided elsewhere
in the form of platform_SHA1_Clone".

So everything evens out.  If we are with newer OpenSSL, we will
include sha1/openssl.h and get both platform_SHA1_Clone and
SHA1_NEEDS_CLONE_HELPER defined.  If we are with older OpenSSL or
non-OpenSSL, we do not get platform_SHA1_Clone (because the "#ifndef
platform_SHA1_CTX" block does not have a fallback default defined)
and we do not get SHA1_NEEDS_CLONE_HELPER either.  We either use the
memcpy() fallback only when we are not working with newer OpenSSL or
whatever defines its own platform_SHA1_Clone.  So the patch smelled
a bit strange, but there isn't anything incorrect per-se.

But then is this making folks unnecessary work when they add
non-OpenSSL support that needs more than just memcpy() to clone the
context?  What breaks if we turn these two hunks into

	#ifdef platform_SHA1_Clone
	#define git_SHA1_Clone platform_SHA1_Clone
	#else
	static inline void git_SHA1_Clone(git_SHA_CTX *dst, git_SHA_CTX *src)
	{
		memcpy(dst, src, sizeof(*dst));
	}
	#endif

and drop the requirement that they must define SHA1_NEEDS_CLONE_HELPER
if they want to define their own platform_SHA1_Clone()?

Thanks.  Everything else in the patch made sense (even though I am
not familiar with the EVP API) to me.