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

Johannes Schindelin [off-list ref] writes:

+#ifdef SHA1_DC_AND_OPENSSL
+void (*SHA1_Init_func)(SHA_CTX_union *ctx) = (void *)SHA1DCInit;
+void (*SHA1_Update_func)(SHA_CTX_union *ctx, const void *pointer, size_t size) =
+	(void *)git_SHA1DCUpdate;
+int (*SHA1_Final_func)(unsigned char sha1[20], SHA_CTX_union *ctx) =
+	(void *)git_SHA1DCFinal;
+
+void toggle_sha1dc(int enable)
+{
+	if (enable) {
+		SHA1_Init_func = (void *)SHA1DCInit;
+		SHA1_Update_func = (void *)git_SHA1DCUpdate;
+		SHA1_Final_func = (void *)git_SHA1DCFinal;
+	} else {
+		SHA1_Init_func = (void *)SHA1_Init;
+		SHA1_Update_func = (void *)SHA1_Update;
+		SHA1_Final_func = (void *)SHA1_Final;
+	}
+}
+#endif

As I understand that this is a demonstration series, the approach
above is OK as an expedite way to illustrate one way how run-time
switching could be done.  The approach however is not very thread
friendly, though.

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.

Hi Junio,

On Thu, 30 Mar 2017, Junio C Hamano wrote:

Johannes Schindelin [off-list ref] writes:

quoted

+#ifdef SHA1_DC_AND_OPENSSL
+void (*SHA1_Init_func)(SHA_CTX_union *ctx) = (void *)SHA1DCInit;
+void (*SHA1_Update_func)(SHA_CTX_union *ctx, const void *pointer, size_t size) =
+	(void *)git_SHA1DCUpdate;
+int (*SHA1_Final_func)(unsigned char sha1[20], SHA_CTX_union *ctx) =
+	(void *)git_SHA1DCFinal;
+
+void toggle_sha1dc(int enable)
+{
+	if (enable) {
+		SHA1_Init_func = (void *)SHA1DCInit;
+		SHA1_Update_func = (void *)git_SHA1DCUpdate;
+		SHA1_Final_func = (void *)git_SHA1DCFinal;
+	} else {
+		SHA1_Init_func = (void *)SHA1_Init;
+		SHA1_Update_func = (void *)SHA1_Update;
+		SHA1_Final_func = (void *)SHA1_Final;
+	}
+}
+#endif

As I understand that this is a demonstration series, the approach
above is OK as an expedite way to illustrate one way how run-time
switching could be done.  The approach however is not very thread
friendly, though.

Indeed. However, the toggle is meant to be coarse, heavy-handed. I have to
protect my time as Git for Windows maintainer, so I have to keep this as
simple to maintain as possible while also benefitting my users. This
toggle is intended to be run very, very early in the cmd_main() functions.
As in: right when the config is read.

quoted

diff --git a/sha1dc/sha1.h b/sha1dc/sha1.h
index bd8bd928fb3..243c2fe0b6b 100644
--- a/sha1dc/sha1.h
+++ b/sha1dc/sha1.h

@@ -110,10 +110,26 @@ void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);
  */
 void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);
 
+#ifdef SHA1_DC_AND_OPENSSL
+extern void toggle_sha1dc(int enable);
+
+typedef union {
+	SHA1_CTX dc;
+	SHA_CTX openssl;
+} SHA_CTX_union;

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.

Thanks, at this stage it is pretty clear, though, that I will have to
maintain this. I am not willing to make this as configurable as you
suggested, as these patches will have to stay in git-for-windows/git.

Ciao,
Dscho