Eric Wong <e@xxxxxxxxx> 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.
