Re: [PATCH 2/7] Makefile: optionally compile with both SHA1DC and SHA1_OPENSSL

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 30, 2017, 16:47 UTC
Message-ID: <xmqqvaqq67am.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <xmqq37du7n9p.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 25 quoted lines

> ...
> The use of union is a good ingredient for a solution.  I would have
> chosen to do this slightly differently if I were doing it.
>
>         typedef struct {
>                 int safe;
>                 union {
>                         SHA1_CTX_SAFE safe;
>                         SHA1_CTX_FAST fast;
>                 } u;
>         } git_SHA_CTX;
>
>         void git_SHA1_Init(git_SHA_CTX *ctx, int safe);
> 	void git_SHA1_Update(git_SHA_CTX *ctx, const void *, unsigned long);
> 	git_SHA1_Final(uchar [20], git_SHA_CTX *ctx);
>
> where SHA1_CTX_FAST may be chosen from the Makefile just like we
> currently choose platform_SHA_CTX.  SHA1_CTX_SAFE could also be made
> configurable but it may be OK to hardcode it to refer to SHA1_CTX of
> DC's.
>
> As you already know, I am assuming that each codepath pretty much
> knows if it needs safe or fast one (e.g. the one used in csum-file.c
> knows it does not have to), so each git_SHA_CTX is told which one to
> use when it gets initialized.

And if we wanted to declare "git add" is always safe, we could still do

    int sha1_safety_global_override = -1; /* unspecified */

    void git_SHA1_Init(git_SHA_CTX *ctx, int safe)
    {
	if (sha1_safety_global_override >= 0)
	    ctx->safe = sha1_safety_global_override;
        else
	    ctx->safe = safe;

	if (ctx->safe)
	    SHA1DCInit(&(ctx->u.safe));
	else
	    platform_SHA1_Init(&(ctx->u.fast));
   }

and then have cmd_add() in builtin/add.c to flip that global override bit to say "this does not have to be safe". I personally do not think it is a good idea, but I am showing that it is still doable.

And as long as assignment to sha1_safety_global_override is done in a thread-friendly way, such a scheme would be more thread-friendly as a whole compared to the "toggle_sha1dc()" approach where each CTX instance does not know which side of the union it is being used us (which, if mixed-up, of course would lead to a funny behaviour).