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

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

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.