{"thread":{"id":"46461","subject":"[PATCH] hash: Allow building with the external sha1dc library","startedAt":"2017-07-25T05:57:50Z","lastAt":"2017-08-12T06:50:53Z","messageCount":13,"participants":["Takashi Iwai","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"325027","messageId":"s5hh8y19hyg.wl-tiwai@suse.de","threadId":"46461","inReplyTo":null,"subject":"[PATCH] hash: Allow building with the external sha1dc library","fromName":"Takashi Iwai","fromEmail":"tiwai@suse.de","sentAt":"2017-07-25T05:57:43Z","receivedAt":"2017-07-25T05:57:50Z","isPatch":true,"sender":{"key":"tiwai@suse.de","avatar":"https://avatars.githubusercontent.com/u/306482?v=4"},"body":"Some distros provide SHA1 collision detect code as a shared library.\nIt's the very same code as we have in git tree, and git can link with\nit as well; at least, it may make maintenance easier, according to our\nsecurity guys.\n\nThis patch allows user to build git linking with the external sha1dc\nlibrary instead of the built-in sha1dc code.  User needs to define\nDC_SHA1_EXTERNAL explicitly.  As default, the built-in sha1dc code is\nused like before.\n\nSigned-off-by: Takashi Iwai <tiwai@suse.de>\n---\n Makefile         | 12 ++++++++++++\n hash.h           |  4 +++-\n sha1dc_git_ext.c | 11 +++++++++++\n sha1dc_git_ext.h | 25 +++++++++++++++++++++++++\n 4 files changed, 51 insertions(+), 1 deletion(-)\n create mode 100644 sha1dc_git_ext.c\n create mode 100644 sha1dc_git_ext.h\n\ndiff --git a/Makefile b/Makefile\nindex 461c845d33cb..f1a262d56254 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -162,6 +162,12 @@ all::\n # algorithm. This is slower, but may detect attempted collision attacks.\n # Takes priority over other *_SHA1 knobs.\n #\n+# Define DC_SHA1_EXTERNAL in addition to DC_SHA1 if you want to build / link\n+# git with the external sha1collisiondetection library.\n+# Without this option, i.e. the default behavior is to build git with its\n+# own sha1dc code.  If any extra linker option is required, define them in\n+# DC_SHA1_LINK variable in addition.\n+#\n # Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the\n # sha1collisiondetection shipped as a submodule instead of the\n # non-submodule copy in sha1dc/. This is an experimental option used\n@@ -1472,6 +1478,11 @@ ifdef APPLE_COMMON_CRYPTO\n \tBASIC_CFLAGS += -DSHA1_APPLE\n else\n \tDC_SHA1 := YesPlease\n+ifdef DC_SHA1_EXTERNAL\n+\tLIB_OBJS += sha1dc_git_ext.o\n+\tBASIC_CFLAGS += -DSHA1_DC -DDC_SHA1_EXTERNAL\n+\tEXTLIBS += $(DC_SHA1_LINK) -lsha1detectcoll\n+else\n ifdef DC_SHA1_SUBMODULE\n \tLIB_OBJS += sha1collisiondetection/lib/sha1.o\n \tLIB_OBJS += sha1collisiondetection/lib/ubc_check.o\n@@ -1492,6 +1503,7 @@ endif\n endif\n endif\n endif\n+endif\n \n ifdef SHA1_MAX_BLOCK_SIZE\n \tLIB_OBJS += compat/sha1-chunked.o\ndiff --git a/hash.h b/hash.h\nindex bef3e630a093..dce327d58d07 100644\n--- a/hash.h\n+++ b/hash.h\n@@ -8,7 +8,9 @@\n #elif defined(SHA1_OPENSSL)\n #include <openssl/sha.h>\n #elif defined(SHA1_DC)\n-#ifdef DC_SHA1_SUBMODULE\n+#if defined(DC_SHA1_EXTERNAL)\n+#include \"sha1dc_git_ext.h\"\n+#elif defined(DC_SHA1_SUBMODULE)\n #include \"sha1collisiondetection/lib/sha1.h\"\n #else\n #include \"sha1dc/sha1.h\"\ndiff --git a/sha1dc_git_ext.c b/sha1dc_git_ext.c\nnew file mode 100644\nindex 000000000000..359439fc3d93\n--- /dev/null\n+++ b/sha1dc_git_ext.c\n@@ -0,0 +1,11 @@\n+/* Only for DC_SHA1_EXTERNAL; sharing the same hooks as built-in sha1dc */\n+\n+#include \"cache.h\"\n+#include <sha1.h>\n+#include \"sha1dc_git.c\"\n+\n+void git_SHA1DCInit(SHA1_CTX *ctx)\n+{\n+\tSHA1DCInit(ctx);\n+\tSHA1DCSetSafeHash(ctx, 0);\n+}\ndiff --git a/sha1dc_git_ext.h b/sha1dc_git_ext.h\nnew file mode 100644\nindex 000000000000..d0ea8ce518db\n--- /dev/null\n+++ b/sha1dc_git_ext.h\n@@ -0,0 +1,25 @@\n+/*\n+ * This file is included by hash.h for DC_SHA1_EXTERNAL\n+ */\n+\n+#include <sha1.h>\n+\n+/*\n+ * Same as SHA1DCInit, but with default save_hash=0\n+ */\n+void git_SHA1DCInit(SHA1_CTX *);\n+\n+/*\n+ * Same as SHA1DCFinal, but convert collision attack case into a verbose die().\n+ */\n+void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);\n+\n+/*\n+ * Same as SHA1DCUpdate, but adjust types to match git's usual interface.\n+ */\n+void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);\n+\n+#define platform_SHA_CTX SHA1_CTX\n+#define platform_SHA1_Init git_SHA1DCInit\n+#define platform_SHA1_Update git_SHA1DCUpdate\n+#define platform_SHA1_Final git_SHA1DCFinal\n-- \n2.13.3\n\n"},{"id":"325057","messageId":"xmqq60egbal8.fsf@gitster.mtv.corp.google.com","threadId":"46461","inReplyTo":"s5hh8y19hyg.wl-tiwai@suse.de","subject":"Re: [PATCH] hash: Allow building with the external sha1dc library","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-25T19:06:11Z","receivedAt":"2017-07-25T19:06:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Takashi Iwai <tiwai@suse.de> writes:\n\n> Some distros provide SHA1 collision detect code as a shared library.\n> It's the very same code as we have in git tree, and git can link with\n> it as well; at least, it may make maintenance easier, according to our\n> security guys.\n>\n> This patch allows user to build git linking with the external sha1dc\n> library instead of the built-in sha1dc code.  User needs to define\n> DC_SHA1_EXTERNAL explicitly.  As default, the built-in sha1dc code is\n> used like before.\n>\n> Signed-off-by: Takashi Iwai <tiwai@suse.de>\n> ---\n\nI do not have such an environment to test this patch, but it looks\nlike a very sensible thing to do.  Will queue; thanks.\n\n>  Makefile         | 12 ++++++++++++\n>  hash.h           |  4 +++-\n>  sha1dc_git_ext.c | 11 +++++++++++\n>  sha1dc_git_ext.h | 25 +++++++++++++++++++++++++\n>  4 files changed, 51 insertions(+), 1 deletion(-)\n>  create mode 100644 sha1dc_git_ext.c\n>  create mode 100644 sha1dc_git_ext.h\n>\n> diff --git a/Makefile b/Makefile\n> index 461c845d33cb..f1a262d56254 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -162,6 +162,12 @@ all::\n>  # algorithm. This is slower, but may detect attempted collision attacks.\n>  # Takes priority over other *_SHA1 knobs.\n>  #\n> +# Define DC_SHA1_EXTERNAL in addition to DC_SHA1 if you want to build / link\n> +# git with the external sha1collisiondetection library.\n> +# Without this option, i.e. the default behavior is to build git with its\n> +# own sha1dc code.  If any extra linker option is required, define them in\n> +# DC_SHA1_LINK variable in addition.\n> +#\n>  # Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the\n>  # sha1collisiondetection shipped as a submodule instead of the\n>  # non-submodule copy in sha1dc/. This is an experimental option used\n> @@ -1472,6 +1478,11 @@ ifdef APPLE_COMMON_CRYPTO\n>  \tBASIC_CFLAGS += -DSHA1_APPLE\n>  else\n>  \tDC_SHA1 := YesPlease\n> +ifdef DC_SHA1_EXTERNAL\n> +\tLIB_OBJS += sha1dc_git_ext.o\n> +\tBASIC_CFLAGS += -DSHA1_DC -DDC_SHA1_EXTERNAL\n> +\tEXTLIBS += $(DC_SHA1_LINK) -lsha1detectcoll\n> +else\n>  ifdef DC_SHA1_SUBMODULE\n>  \tLIB_OBJS += sha1collisiondetection/lib/sha1.o\n>  \tLIB_OBJS += sha1collisiondetection/lib/ubc_check.o\n> @@ -1492,6 +1503,7 @@ endif\n>  endif\n>  endif\n>  endif\n> +endif\n>  \n>  ifdef SHA1_MAX_BLOCK_SIZE\n>  \tLIB_OBJS += compat/sha1-chunked.o\n> diff --git a/hash.h b/hash.h\n> index bef3e630a093..dce327d58d07 100644\n> --- a/hash.h\n> +++ b/hash.h\n> @@ -8,7 +8,9 @@\n>  #elif defined(SHA1_OPENSSL)\n>  #include <openssl/sha.h>\n>  #elif defined(SHA1_DC)\n> -#ifdef DC_SHA1_SUBMODULE\n> +#if defined(DC_SHA1_EXTERNAL)\n> +#include \"sha1dc_git_ext.h\"\n> +#elif defined(DC_SHA1_SUBMODULE)\n>  #include \"sha1collisiondetection/lib/sha1.h\"\n>  #else\n>  #include \"sha1dc/sha1.h\"\n> diff --git a/sha1dc_git_ext.c b/sha1dc_git_ext.c\n> new file mode 100644\n> index 000000000000..359439fc3d93\n> --- /dev/null\n> +++ b/sha1dc_git_ext.c\n> @@ -0,0 +1,11 @@\n> +/* Only for DC_SHA1_EXTERNAL; sharing the same hooks as built-in sha1dc */\n> +\n> +#include \"cache.h\"\n> +#include <sha1.h>\n> +#include \"sha1dc_git.c\"\n> +\n> +void git_SHA1DCInit(SHA1_CTX *ctx)\n> +{\n> +\tSHA1DCInit(ctx);\n> +\tSHA1DCSetSafeHash(ctx, 0);\n> +}\n> diff --git a/sha1dc_git_ext.h b/sha1dc_git_ext.h\n> new file mode 100644\n> index 000000000000..d0ea8ce518db\n> --- /dev/null\n> +++ b/sha1dc_git_ext.h\n> @@ -0,0 +1,25 @@\n> +/*\n> + * This file is included by hash.h for DC_SHA1_EXTERNAL\n> + */\n> +\n> +#include <sha1.h>\n> +\n> +/*\n> + * Same as SHA1DCInit, but with default save_hash=0\n> + */\n> +void git_SHA1DCInit(SHA1_CTX *);\n> +\n> +/*\n> + * Same as SHA1DCFinal, but convert collision attack case into a verbose die().\n> + */\n> +void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);\n> +\n> +/*\n> + * Same as SHA1DCUpdate, but adjust types to match git's usual interface.\n> + */\n> +void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);\n> +\n> +#define platform_SHA_CTX SHA1_CTX\n> +#define platform_SHA1_Init git_SHA1DCInit\n> +#define platform_SHA1_Update git_SHA1DCUpdate\n> +#define platform_SHA1_Final git_SHA1DCFinal\n"},{"id":"325220","messageId":"CACBZZX5yv-NzL7H-CH1yMeM9dWkz=PUhx=2wek_jBGpsz1=EAA@mail.gmail.com","threadId":"46461","inReplyTo":"s5hh8y19hyg.wl-tiwai@suse.de","subject":"Re: [PATCH] hash: Allow building with the external sha1dc library","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-07-28T15:58:14Z","receivedAt":"2017-07-28T15:58:41Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Jul 25, 2017 at 7:57 AM, Takashi Iwai <tiwai@suse.de> wrote:\n> Some distros provide SHA1 collision detect code as a shared library.\n> It's the very same code as we have in git tree, and git can link with\n> it as well; at least, it may make maintenance easier, according to our\n> security guys.\n>\n> This patch allows user to build git linking with the external sha1dc\n> library instead of the built-in sha1dc code.  User needs to define\n> DC_SHA1_EXTERNAL explicitly.  As default, the built-in sha1dc code is\n> used like before.\n\nThis whole thing sounds sensible. I reviewed this (but like Junio\nhaven't tested it with a lib) and I think it would be worth noting the\nfollowing in the commit message / Makefile documentation:\n\n* The \"sha1detectcoll\" *.so name for the \"sha1collisiondetection\"\nlibrary is not something you or suse presumably) made up, it's a name\nthe sha1collisiondetection.git itself creates for its library. I think\nthe Makefile docs you've added here are a bit confusing, you talk\nabout the \"external sha1collisiondetection library\" but then link\nagainst sha1detectcoll\". It's worth calling out this difference in the\ndocs IMO. I.e. not talk about the sha1detectcoll.so library form of\nsha1collisiondetection, not the sha1collisiondetection project name as\na library.\n\n* It might be worth noting that this is *not* linking against the same\ncode we ship ourselves due to the difference in defining\nSHA1DC_INIT_SAFE_HASH_DEFAULT for the git project's needs in the one\nwe build, hence your need to have a git_SHA1DCInit() wrapper whereas\nwe call SHA1DCInit() directly. It might be interesting to note that\nthe library version will always be *slightly* slower (although the\ndifference will be trivial).\n\n* Nothing in your commit message or docs explains why DC_SHA1_LINK is\nneeded. We don't have these sorts of variables for other external\nlibraries we link to, why the difference?\n\nSome other things I observed:\n\n* We now have much of the same header code copy/pasted between\nsha1dc_git.h and sha1dc_git_ext.h, did you consider just always\nincluding the former but making what it's doing conditional on\nDC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory\nglance, but again your commit message doesn't list that among options\nconsidered & discarded.\n\n* I think it makes sense to spew out a \"not both!\" error in the\nMakefile if you set DC_SHA1_EXTERNAL=Y and DC_SHA1_SUBMODULE=Y. See my\n94da9193a6 (\"grep: add support for PCRE v2\", 2017-06-01) for an\nexample of how to do this.\n\n* The whole business of \"#include <sha1.h>\" looks very fragile, are\nthere really no other packages in e.g. suse that ship a sha1.h? Debian\nhas libmd-dev that ships /usr/include/sha1.h that conflicts with this:\nhttps://packages.debian.org/search?searchon=contents&keywords=sha1.h&mode=exactfilename&suite=unstable&arch=any\n\nShipping a sha1.h as opposed to a sha1collisiondetection.h or\nsha1detectcoll.h or whatever seems like a *really* bad decision by\nupstream that should be the subject of at least seeing if they'll take\na pull request to fix it before you package it or before we include\nsomething that'll probably need to be fixed / worked around anyway in\nGit.\n\n> Signed-off-by: Takashi Iwai <tiwai@suse.de>\n> ---\n>  Makefile         | 12 ++++++++++++\n>  hash.h           |  4 +++-\n>  sha1dc_git_ext.c | 11 +++++++++++\n>  sha1dc_git_ext.h | 25 +++++++++++++++++++++++++\n>  4 files changed, 51 insertions(+), 1 deletion(-)\n>  create mode 100644 sha1dc_git_ext.c\n>  create mode 100644 sha1dc_git_ext.h\n>\n> diff --git a/Makefile b/Makefile\n> index 461c845d33cb..f1a262d56254 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -162,6 +162,12 @@ all::\n>  # algorithm. This is slower, but may detect attempted collision attacks.\n>  # Takes priority over other *_SHA1 knobs.\n>  #\n> +# Define DC_SHA1_EXTERNAL in addition to DC_SHA1 if you want to build / link\n> +# git with the external sha1collisiondetection library.\n> +# Without this option, i.e. the default behavior is to build git with its\n> +# own sha1dc code.  If any extra linker option is required, define them in\n> +# DC_SHA1_LINK variable in addition.\n> +#\n>  # Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the\n>  # sha1collisiondetection shipped as a submodule instead of the\n>  # non-submodule copy in sha1dc/. This is an experimental option used\n> @@ -1472,6 +1478,11 @@ ifdef APPLE_COMMON_CRYPTO\n>         BASIC_CFLAGS += -DSHA1_APPLE\n>  else\n>         DC_SHA1 := YesPlease\n> +ifdef DC_SHA1_EXTERNAL\n> +       LIB_OBJS += sha1dc_git_ext.o\n> +       BASIC_CFLAGS += -DSHA1_DC -DDC_SHA1_EXTERNAL\n> +       EXTLIBS += $(DC_SHA1_LINK) -lsha1detectcoll\n> +else\n>  ifdef DC_SHA1_SUBMODULE\n>         LIB_OBJS += sha1collisiondetection/lib/sha1.o\n>         LIB_OBJS += sha1collisiondetection/lib/ubc_check.o\n> @@ -1492,6 +1503,7 @@ endif\n>  endif\n>  endif\n>  endif\n> +endif\n>\n>  ifdef SHA1_MAX_BLOCK_SIZE\n>         LIB_OBJS += compat/sha1-chunked.o\n> diff --git a/hash.h b/hash.h\n> index bef3e630a093..dce327d58d07 100644\n> --- a/hash.h\n> +++ b/hash.h\n> @@ -8,7 +8,9 @@\n>  #elif defined(SHA1_OPENSSL)\n>  #include <openssl/sha.h>\n>  #elif defined(SHA1_DC)\n> -#ifdef DC_SHA1_SUBMODULE\n> +#if defined(DC_SHA1_EXTERNAL)\n> +#include \"sha1dc_git_ext.h\"\n> +#elif defined(DC_SHA1_SUBMODULE)\n>  #include \"sha1collisiondetection/lib/sha1.h\"\n>  #else\n>  #include \"sha1dc/sha1.h\"\n> diff --git a/sha1dc_git_ext.c b/sha1dc_git_ext.c\n> new file mode 100644\n> index 000000000000..359439fc3d93\n> --- /dev/null\n> +++ b/sha1dc_git_ext.c\n> @@ -0,0 +1,11 @@\n> +/* Only for DC_SHA1_EXTERNAL; sharing the same hooks as built-in sha1dc */\n> +\n> +#include \"cache.h\"\n> +#include <sha1.h>\n> +#include \"sha1dc_git.c\"\n> +\n> +void git_SHA1DCInit(SHA1_CTX *ctx)\n> +{\n> +       SHA1DCInit(ctx);\n> +       SHA1DCSetSafeHash(ctx, 0);\n> +}\n\n\n\n> diff --git a/sha1dc_git_ext.h b/sha1dc_git_ext.h\n> new file mode 100644\n> index 000000000000..d0ea8ce518db\n> --- /dev/null\n> +++ b/sha1dc_git_ext.h\n> @@ -0,0 +1,25 @@\n> +/*\n> + * This file is included by hash.h for DC_SHA1_EXTERNAL\n> + */\n> +\n> +#include <sha1.h>\n> +\n> +/*\n> + * Same as SHA1DCInit, but with default save_hash=0\n> + */\n> +void git_SHA1DCInit(SHA1_CTX *);\n> +\n> +/*\n> + * Same as SHA1DCFinal, but convert collision attack case into a verbose die().\n> + */\n> +void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);\n> +\n> +/*\n> + * Same as SHA1DCUpdate, but adjust types to match git's usual interface.\n> + */\n> +void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);\n> +\n> +#define platform_SHA_CTX SHA1_CTX\n> +#define platform_SHA1_Init git_SHA1DCInit\n> +#define platform_SHA1_Update git_SHA1DCUpdate\n> +#define platform_SHA1_Final git_SHA1DCFinal\n> --\n> 2.13.3\n>\n"},{"id":"325221","messageId":"CACBZZX7M=H8tNkZXpHBvv0rbY58EJk4dkoUzGKMftWoKUqF8sA@mail.gmail.com","threadId":"46461","inReplyTo":"CACBZZX5yv-NzL7H-CH1yMeM9dWkz=PUhx=2wek_jBGpsz1=EAA@mail.gmail.com","subject":"Re: [PATCH] hash: Allow building with the external sha1dc library","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-07-28T16:04:18Z","receivedAt":"2017-07-28T16:04:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Fri, Jul 28, 2017 at 5:58 PM, Ævar Arnfjörð Bjarmason\n<avarab@gmail.com> wrote:\n> On Tue, Jul 25, 2017 at 7:57 AM, Takashi Iwai <tiwai@suse.de> wrote:\n>> Some distros provide SHA1 collision detect code as a shared library.\n>> It's the very same code as we have in git tree, and git can link with\n>> it as well; at least, it may make maintenance easier, according to our\n>> security guys.\n>>\n>> This patch allows user to build git linking with the external sha1dc\n>> library instead of the built-in sha1dc code.  User needs to define\n>> DC_SHA1_EXTERNAL explicitly.  As default, the built-in sha1dc code is\n>> used like before.\n>\n> This whole thing sounds sensible. I reviewed this (but like Junio\n> haven't tested it with a lib) and I think it would be worth noting the\n> following in the commit message / Makefile documentation:\n>\n> * The \"sha1detectcoll\" *.so name for the \"sha1collisiondetection\"\n> library is not something you or suse presumably) made up, it's a name\n> the sha1collisiondetection.git itself creates for its library. I think\n> the Makefile docs you've added here are a bit confusing, you talk\n> about the \"external sha1collisiondetection library\" but then link\n> against sha1detectcoll\". It's worth calling out this difference in the\n> docs IMO. I.e. not talk about the sha1detectcoll.so library form of\n> sha1collisiondetection, not the sha1collisiondetection project name as\n> a library.\n>\n> * It might be worth noting that this is *not* linking against the same\n> code we ship ourselves due to the difference in defining\n> SHA1DC_INIT_SAFE_HASH_DEFAULT for the git project's needs in the one\n> we build, hence your need to have a git_SHA1DCInit() wrapper whereas\n> we call SHA1DCInit() directly. It might be interesting to note that\n> the library version will always be *slightly* slower (although the\n> difference will be trivial).\n>\n> * Nothing in your commit message or docs explains why DC_SHA1_LINK is\n> needed. We don't have these sorts of variables for other external\n> libraries we link to, why the difference?\n>\n> Some other things I observed:\n>\n> * We now have much of the same header code copy/pasted between\n> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always\n> including the former but making what it's doing conditional on\n> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory\n> glance, but again your commit message doesn't list that among options\n> considered & discarded.\n>\n> * I think it makes sense to spew out a \"not both!\" error in the\n> Makefile if you set DC_SHA1_EXTERNAL=Y and DC_SHA1_SUBMODULE=Y. See my\n> 94da9193a6 (\"grep: add support for PCRE v2\", 2017-06-01) for an\n> example of how to do this.\n>\n> * The whole business of \"#include <sha1.h>\" looks very fragile, are\n> there really no other packages in e.g. suse that ship a sha1.h? Debian\n> has libmd-dev that ships /usr/include/sha1.h that conflicts with this:\n> https://packages.debian.org/search?searchon=contents&keywords=sha1.h&mode=exactfilename&suite=unstable&arch=any\n>\n> Shipping a sha1.h as opposed to a sha1collisiondetection.h or\n> sha1detectcoll.h or whatever seems like a *really* bad decision by\n> upstream that should be the subject of at least seeing if they'll take\n> a pull request to fix it before you package it or before we include\n> something that'll probably need to be fixed / worked around anyway in\n> Git.\n\nI sent this last bit a tad too soon in a checkout of sha1collisiondetection.git:\n\n    $ make PREFIX=/tmp/local install >/dev/null 2>&1 && find /tmp/local/ -type f\n    /tmp/local/include/sha1dc/sha1.h\n    /tmp/local/bin/sha1dcsum\n    /tmp/local/bin/sha1dcsum_partialcoll\n    /tmp/local/lib/libsha1detectcoll.a\n    /tmp/local/lib/libsha1detectcoll.so.1.0.0\n    /tmp/local/lib/libsha1detectcoll.la\n\nSo the upstream library expects you (and it's documented in their README) to do:\n\n    #include <sha1dc/sha1.h>\n\nBut your patch is just doing:\n\n    #include <sha1.h>\n\nAt best this seems like a trivial bug and at worst us encoding some\nSuse-specific packaging convention in git, since other distros would\npresumably want to package this in /usr/include/sha1dc/sha1.h as\nupstream suggests. I.e. using the ambiguous sha1.h name is not\nsomething upstream's doing by default, it's something you're doing in\nyour package.\n\n>> Signed-off-by: Takashi Iwai <tiwai@suse.de>\n>> ---\n>>  Makefile         | 12 ++++++++++++\n>>  hash.h           |  4 +++-\n>>  sha1dc_git_ext.c | 11 +++++++++++\n>>  sha1dc_git_ext.h | 25 +++++++++++++++++++++++++\n>>  4 files changed, 51 insertions(+), 1 deletion(-)\n>>  create mode 100644 sha1dc_git_ext.c\n>>  create mode 100644 sha1dc_git_ext.h\n>>\n>> diff --git a/Makefile b/Makefile\n>> index 461c845d33cb..f1a262d56254 100644\n>> --- a/Makefile\n>> +++ b/Makefile\n>> @@ -162,6 +162,12 @@ all::\n>>  # algorithm. This is slower, but may detect attempted collision attacks.\n>>  # Takes priority over other *_SHA1 knobs.\n>>  #\n>> +# Define DC_SHA1_EXTERNAL in addition to DC_SHA1 if you want to build / link\n>> +# git with the external sha1collisiondetection library.\n>> +# Without this option, i.e. the default behavior is to build git with its\n>> +# own sha1dc code.  If any extra linker option is required, define them in\n>> +# DC_SHA1_LINK variable in addition.\n>> +#\n>>  # Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the\n>>  # sha1collisiondetection shipped as a submodule instead of the\n>>  # non-submodule copy in sha1dc/. This is an experimental option used\n>> @@ -1472,6 +1478,11 @@ ifdef APPLE_COMMON_CRYPTO\n>>         BASIC_CFLAGS += -DSHA1_APPLE\n>>  else\n>>         DC_SHA1 := YesPlease\n>> +ifdef DC_SHA1_EXTERNAL\n>> +       LIB_OBJS += sha1dc_git_ext.o\n>> +       BASIC_CFLAGS += -DSHA1_DC -DDC_SHA1_EXTERNAL\n>> +       EXTLIBS += $(DC_SHA1_LINK) -lsha1detectcoll\n>> +else\n>>  ifdef DC_SHA1_SUBMODULE\n>>         LIB_OBJS += sha1collisiondetection/lib/sha1.o\n>>         LIB_OBJS += sha1collisiondetection/lib/ubc_check.o\n>> @@ -1492,6 +1503,7 @@ endif\n>>  endif\n>>  endif\n>>  endif\n>> +endif\n>>\n>>  ifdef SHA1_MAX_BLOCK_SIZE\n>>         LIB_OBJS += compat/sha1-chunked.o\n>> diff --git a/hash.h b/hash.h\n>> index bef3e630a093..dce327d58d07 100644\n>> --- a/hash.h\n>> +++ b/hash.h\n>> @@ -8,7 +8,9 @@\n>>  #elif defined(SHA1_OPENSSL)\n>>  #include <openssl/sha.h>\n>>  #elif defined(SHA1_DC)\n>> -#ifdef DC_SHA1_SUBMODULE\n>> +#if defined(DC_SHA1_EXTERNAL)\n>> +#include \"sha1dc_git_ext.h\"\n>> +#elif defined(DC_SHA1_SUBMODULE)\n>>  #include \"sha1collisiondetection/lib/sha1.h\"\n>>  #else\n>>  #include \"sha1dc/sha1.h\"\n>> diff --git a/sha1dc_git_ext.c b/sha1dc_git_ext.c\n>> new file mode 100644\n>> index 000000000000..359439fc3d93\n>> --- /dev/null\n>> +++ b/sha1dc_git_ext.c\n>> @@ -0,0 +1,11 @@\n>> +/* Only for DC_SHA1_EXTERNAL; sharing the same hooks as built-in sha1dc */\n>> +\n>> +#include \"cache.h\"\n>> +#include <sha1.h>\n>> +#include \"sha1dc_git.c\"\n>> +\n>> +void git_SHA1DCInit(SHA1_CTX *ctx)\n>> +{\n>> +       SHA1DCInit(ctx);\n>> +       SHA1DCSetSafeHash(ctx, 0);\n>> +}\n>\n>\n>\n>> diff --git a/sha1dc_git_ext.h b/sha1dc_git_ext.h\n>> new file mode 100644\n>> index 000000000000..d0ea8ce518db\n>> --- /dev/null\n>> +++ b/sha1dc_git_ext.h\n>> @@ -0,0 +1,25 @@\n>> +/*\n>> + * This file is included by hash.h for DC_SHA1_EXTERNAL\n>> + */\n>> +\n>> +#include <sha1.h>\n>> +\n>> +/*\n>> + * Same as SHA1DCInit, but with default save_hash=0\n>> + */\n>> +void git_SHA1DCInit(SHA1_CTX *);\n>> +\n>> +/*\n>> + * Same as SHA1DCFinal, but convert collision attack case into a verbose die().\n>> + */\n>> +void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);\n>> +\n>> +/*\n>> + * Same as SHA1DCUpdate, but adjust types to match git's usual interface.\n>> + */\n>> +void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);\n>> +\n>> +#define platform_SHA_CTX SHA1_CTX\n>> +#define platform_SHA1_Init git_SHA1DCInit\n>> +#define platform_SHA1_Update git_SHA1DCUpdate\n>> +#define platform_SHA1_Final git_SHA1DCFinal\n>> --\n>> 2.13.3\n>>\n"},{"id":"325351","messageId":"xmqqk22ogs1w.fsf@gitster.mtv.corp.google.com","threadId":"46461","inReplyTo":"CACBZZX7M=H8tNkZXpHBvv0rbY58EJk4dkoUzGKMftWoKUqF8sA@mail.gmail.com","subject":"Re: [PATCH] hash: Allow building with the external sha1dc library","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-07-31T22:28:11Z","receivedAt":"2017-07-31T22:28:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> So the upstream library expects you (and it's documented in their README) to do:\n>\n>     #include <sha1dc/sha1.h>\n>\n> But your patch is just doing:\n>\n>     #include <sha1.h>\n>\n> At best this seems like a trivial bug and at worst us encoding some\n> Suse-specific packaging convention in git, since other distros would\n> presumably want to package this in /usr/include/sha1dc/sha1.h as\n> upstream suggests. I.e. using the ambiguous sha1.h name is not\n> something upstream's doing by default, it's something you're doing in\n> your package.\n\nIt seems there still needs a bit more work on this patch.  Thanks\nfor reviewing and pointing out what needs to be addressed.\n\n"},{"id":"325363","messageId":"s5h8tj37scz.wl-tiwai@suse.de","threadId":"46461","inReplyTo":"CACBZZX5yv-NzL7H-CH1yMeM9dWkz=PUhx=2wek_jBGpsz1=EAA@mail.gmail.com","subject":"Re: [PATCH] hash: Allow building with the external sha1dc library","fromName":"Takashi Iwai","fromEmail":"tiwai@suse.de","sentAt":"2017-08-01T05:46:20Z","receivedAt":"2017-08-01T05:46:27Z","isPatch":true,"sender":{"key":"tiwai@suse.de","avatar":"https://avatars.githubusercontent.com/u/306482?v=4"},"body":"On Fri, 28 Jul 2017 17:58:14 +0200,\nÆvar Arnfjörð Bjarmason wrote:\n> \n> On Tue, Jul 25, 2017 at 7:57 AM, Takashi Iwai <tiwai@suse.de> wrote:\n> > Some distros provide SHA1 collision detect code as a shared library.\n> > It's the very same code as we have in git tree, and git can link with\n> > it as well; at least, it may make maintenance easier, according to our\n> > security guys.\n> >\n> > This patch allows user to build git linking with the external sha1dc\n> > library instead of the built-in sha1dc code.  User needs to define\n> > DC_SHA1_EXTERNAL explicitly.  As default, the built-in sha1dc code is\n> > used like before.\n> \n> This whole thing sounds sensible. I reviewed this (but like Junio\n> haven't tested it with a lib) and I think it would be worth noting the\n> following in the commit message / Makefile documentation:\n\nHi,\n\nsorry for the late reply, and thanks for the review.\n\n> * The \"sha1detectcoll\" *.so name for the \"sha1collisiondetection\"\n> library is not something you or suse presumably) made up, it's a name\n> the sha1collisiondetection.git itself creates for its library. I think\n> the Makefile docs you've added here are a bit confusing, you talk\n> about the \"external sha1collisiondetection library\" but then link\n> against sha1detectcoll\". It's worth calling out this difference in the\n> docs IMO. I.e. not talk about the sha1detectcoll.so library form of\n> sha1collisiondetection, not the sha1collisiondetection project name as\n> a library.\n\nHeh, I was confused and annoyed by this inconsistency, too :)\nIIRC, I used \"sha1collisiondetection\" since the text for\nDC_SHA1_SUBMODULE refers to this term already.\n\nI'll keep using the more readable term (e.g. SHA1 collision-detection\nor sha1dc as abbreviated form) instead of the project name.\n\n> * It might be worth noting that this is *not* linking against the same\n> code we ship ourselves due to the difference in defining\n> SHA1DC_INIT_SAFE_HASH_DEFAULT for the git project's needs in the one\n> we build, hence your need to have a git_SHA1DCInit() wrapper whereas\n> we call SHA1DCInit() directly. It might be interesting to note that\n> the library version will always be *slightly* slower (although the\n> difference will be trivial).\n\nWell, it's just a matter of definition of \"code\".  If you think of the\nsource code, it is the same code.  The difference is merely the build\noption and how it's compiled.  The performance difference is also not\nworth to note.  If the call pattern differs due to shlib, it does\nchange no matter whether it's with the same option or not.  Using\nshlib always implies that, you must know it.\n\nIn anyway, I'm going to rephrase \"code\" with \"source code\" to avoid\nconfusion.\n\n> * Nothing in your commit message or docs explains why DC_SHA1_LINK is\n> needed. We don't have these sorts of variables for other external\n> libraries we link to, why the difference?\n\nIMO, it's other way round.  If we didn't provide any such option for\nusing external library, we were too lazy.\n\nActually, I was lazy and didn't provide DC_SHA1_CFLAGS, and you've\nproven this to be bad -- the library header might be installed in a\ndifferent location than the standard path.  The DC_SHA1_CFLAGS can be\nset as default to -Isha1dc so that it covers that path.\n\nI'll add this to the revised patch.\n\n> Some other things I observed:\n> \n> * We now have much of the same header code copy/pasted between\n> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always\n> including the former but making what it's doing conditional on\n> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory\n> glance, but again your commit message doesn't list that among options\n> considered & discarded.\n\nI don't mind either way, there is no perfect solution in this case.\nAs you know, many people think the ifdef ugly no matter how.\n\nI leave the decision to maintainer.  Just let me know which option is\npreferred.\n\n\n> * I think it makes sense to spew out a \"not both!\" error in the\n> Makefile if you set DC_SHA1_EXTERNAL=Y and DC_SHA1_SUBMODULE=Y. See my\n> 94da9193a6 (\"grep: add support for PCRE v2\", 2017-06-01) for an\n> example of how to do this.\n\nYeah, that's a good point.  Will add the check.\n\n> * The whole business of \"#include <sha1.h>\" looks very fragile, are\n> there really no other packages in e.g. suse that ship a sha1.h? Debian\n> has libmd-dev that ships /usr/include/sha1.h that conflicts with this:\n> https://packages.debian.org/search?searchon=contents&keywords=sha1.h&mode=exactfilename&suite=unstable&arch=any\n> \n> Shipping a sha1.h as opposed to a sha1collisiondetection.h or\n> sha1detectcoll.h or whatever seems like a *really* bad decision by\n> upstream that should be the subject of at least seeing if they'll take\n> a pull request to fix it before you package it or before we include\n> something that'll probably need to be fixed / worked around anyway in\n> Git.\n\nYeah, a unique header name would be better indeed.\n\n\nthanks,\n\nTakashi\n"},{"id":"325364","messageId":"s5h7eyn7s1x.wl-tiwai@suse.de","threadId":"46461","inReplyTo":"CACBZZX7M=H8tNkZXpHBvv0rbY58EJk4dkoUzGKMftWoKUqF8sA@mail.gmail.com","subject":"Re: [PATCH] hash: Allow building with the external sha1dc library","fromName":"Takashi Iwai","fromEmail":"tiwai@suse.de","sentAt":"2017-08-01T05:52:58Z","receivedAt":"2017-08-01T05:53:04Z","isPatch":true,"sender":{"key":"tiwai@suse.de","avatar":"https://avatars.githubusercontent.com/u/306482?v=4"},"body":"On Fri, 28 Jul 2017 18:04:18 +0200,\nÆvar Arnfjörð Bjarmason wrote:\n> \n> On Fri, Jul 28, 2017 at 5:58 PM, Ævar Arnfjörð Bjarmason\n> <avarab@gmail.com> wrote:\n> > On Tue, Jul 25, 2017 at 7:57 AM, Takashi Iwai <tiwai@suse.de> wrote:\n> >> Some distros provide SHA1 collision detect code as a shared library.\n> >> It's the very same code as we have in git tree, and git can link with\n> >> it as well; at least, it may make maintenance easier, according to our\n> >> security guys.\n> >>\n> >> This patch allows user to build git linking with the external sha1dc\n> >> library instead of the built-in sha1dc code.  User needs to define\n> >> DC_SHA1_EXTERNAL explicitly.  As default, the built-in sha1dc code is\n> >> used like before.\n> >\n> > This whole thing sounds sensible. I reviewed this (but like Junio\n> > haven't tested it with a lib) and I think it would be worth noting the\n> > following in the commit message / Makefile documentation:\n> >\n> > * The \"sha1detectcoll\" *.so name for the \"sha1collisiondetection\"\n> > library is not something you or suse presumably) made up, it's a name\n> > the sha1collisiondetection.git itself creates for its library. I think\n> > the Makefile docs you've added here are a bit confusing, you talk\n> > about the \"external sha1collisiondetection library\" but then link\n> > against sha1detectcoll\". It's worth calling out this difference in the\n> > docs IMO. I.e. not talk about the sha1detectcoll.so library form of\n> > sha1collisiondetection, not the sha1collisiondetection project name as\n> > a library.\n> >\n> > * It might be worth noting that this is *not* linking against the same\n> > code we ship ourselves due to the difference in defining\n> > SHA1DC_INIT_SAFE_HASH_DEFAULT for the git project's needs in the one\n> > we build, hence your need to have a git_SHA1DCInit() wrapper whereas\n> > we call SHA1DCInit() directly. It might be interesting to note that\n> > the library version will always be *slightly* slower (although the\n> > difference will be trivial).\n> >\n> > * Nothing in your commit message or docs explains why DC_SHA1_LINK is\n> > needed. We don't have these sorts of variables for other external\n> > libraries we link to, why the difference?\n> >\n> > Some other things I observed:\n> >\n> > * We now have much of the same header code copy/pasted between\n> > sha1dc_git.h and sha1dc_git_ext.h, did you consider just always\n> > including the former but making what it's doing conditional on\n> > DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory\n> > glance, but again your commit message doesn't list that among options\n> > considered & discarded.\n> >\n> > * I think it makes sense to spew out a \"not both!\" error in the\n> > Makefile if you set DC_SHA1_EXTERNAL=Y and DC_SHA1_SUBMODULE=Y. See my\n> > 94da9193a6 (\"grep: add support for PCRE v2\", 2017-06-01) for an\n> > example of how to do this.\n> >\n> > * The whole business of \"#include <sha1.h>\" looks very fragile, are\n> > there really no other packages in e.g. suse that ship a sha1.h? Debian\n> > has libmd-dev that ships /usr/include/sha1.h that conflicts with this:\n> > https://packages.debian.org/search?searchon=contents&keywords=sha1.h&mode=exactfilename&suite=unstable&arch=any\n> >\n> > Shipping a sha1.h as opposed to a sha1collisiondetection.h or\n> > sha1detectcoll.h or whatever seems like a *really* bad decision by\n> > upstream that should be the subject of at least seeing if they'll take\n> > a pull request to fix it before you package it or before we include\n> > something that'll probably need to be fixed / worked around anyway in\n> > Git.\n> \n> I sent this last bit a tad too soon in a checkout of sha1collisiondetection.git:\n> \n>     $ make PREFIX=/tmp/local install >/dev/null 2>&1 && find /tmp/local/ -type f\n>     /tmp/local/include/sha1dc/sha1.h\n>     /tmp/local/bin/sha1dcsum\n>     /tmp/local/bin/sha1dcsum_partialcoll\n>     /tmp/local/lib/libsha1detectcoll.a\n>     /tmp/local/lib/libsha1detectcoll.so.1.0.0\n>     /tmp/local/lib/libsha1detectcoll.la\n> \n> So the upstream library expects you (and it's documented in their README) to do:\n> \n>     #include <sha1dc/sha1.h>\n> \n> But your patch is just doing:\n> \n>     #include <sha1.h>\n>\n> At best this seems like a trivial bug and at worst us encoding some\n> Suse-specific packaging convention in git, since other distros would\n> presumably want to package this in /usr/include/sha1dc/sha1.h as\n> upstream suggests. I.e. using the ambiguous sha1.h name is not\n> something upstream's doing by default, it's something you're doing in\n> your package.\n\nActually it seems to be a wrong usage of $INCLUDEDIR in the upstream\nMakefile, and SUSE package blindly override $INCLUDEDIR.\n\nBut sha1dc/sha1.h looks like the correct path, as README.md mentions,\nindeed, so maybe we need to work around in SUSE package side.\n\nAndreas, could you work on it please?\n\n\nthanks,\n\nTakashi\n"},{"id":"325375","messageId":"xmqqbmnzgu3z.fsf@gitster.mtv.corp.google.com","threadId":"46461","inReplyTo":"s5h8tj37scz.wl-tiwai@suse.de","subject":"Re: [PATCH] hash: Allow building with the external sha1dc library","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-01T15:56:00Z","receivedAt":"2017-08-01T16:01:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Takashi Iwai <tiwai@suse.de> writes:\n\n> On Fri, 28 Jul 2017 17:58:14 +0200,\n> Ævar Arnfjörð Bjarmason wrote:\n>>  ...\n>> * We now have much of the same header code copy/pasted between\n>> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always\n>> including the former but making what it's doing conditional on\n>> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory\n>> glance, but again your commit message doesn't list that among options\n>> considered & discarded.\n>\n> I don't mind either way, there is no perfect solution in this case.\n> As you know, many people think the ifdef ugly no matter how.\n>\n> I leave the decision to maintainer.  Just let me know which option is\n> preferred.\n\nYeah, I also found it somewhat confusing to have these two headers\nthat look quite similar to each other at the top-level of the tree.\n\nWhat's the \"conditional\" part between the two headers?  Is it just\nwhether the header for underlying library is included?  I wonder if\nit's just the matter of adjusting \"hash.h\" to read like this\n\n    ...\n    #if defined(DC_SHA1_EXTERNAL)\n   -#include \"sha1dc_git_ext.h\"\n   +#include <sha1dc/sha1.h>\n   +#include \"sha1dc_git.h\"\n    #elif  defined(DC_SHA1_SUBMODULE)\n    ...\n\nor are there heavier tweaks needed that won't be solved by continuing\nalong the same line?  As _ext.h variant is included only at this place,\nif we can do with minimum tweaks around here without introducing it,\nit may be ideal, I would think.\n\nThanks.\n\n"},{"id":"325379","messageId":"s5hmv7jw9lx.wl-tiwai@suse.de","threadId":"46461","inReplyTo":"xmqqbmnzgu3z.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] hash: Allow building with the external sha1dc library","fromName":"Takashi Iwai","fromEmail":"tiwai@suse.de","sentAt":"2017-08-01T16:12:10Z","receivedAt":"2017-08-01T16:13:16Z","isPatch":true,"sender":{"key":"tiwai@suse.de","avatar":"https://avatars.githubusercontent.com/u/306482?v=4"},"body":"On Tue, 01 Aug 2017 17:56:00 +0200,\nJunio C Hamano wrote:\n> \n> Takashi Iwai <tiwai@suse.de> writes:\n> \n> > On Fri, 28 Jul 2017 17:58:14 +0200,\n> > Ævar Arnfjörð Bjarmason wrote:\n> >>  ...\n> >> * We now have much of the same header code copy/pasted between\n> >> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always\n> >> including the former but making what it's doing conditional on\n> >> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory\n> >> glance, but again your commit message doesn't list that among options\n> >> considered & discarded.\n> >\n> > I don't mind either way, there is no perfect solution in this case.\n> > As you know, many people think the ifdef ugly no matter how.\n> >\n> > I leave the decision to maintainer.  Just let me know which option is\n> > preferred.\n> \n> Yeah, I also found it somewhat confusing to have these two headers\n> that look quite similar to each other at the top-level of the tree.\n> \n> What's the \"conditional\" part between the two headers?  Is it just\n> whether the header for underlying library is included?  I wonder if\n> it's just the matter of adjusting \"hash.h\" to read like this\n> \n>     ...\n>     #if defined(DC_SHA1_EXTERNAL)\n>    -#include \"sha1dc_git_ext.h\"\n>    +#include <sha1dc/sha1.h>\n>    +#include \"sha1dc_git.h\"\n>     #elif  defined(DC_SHA1_SUBMODULE)\n>     ...\n> \n> or are there heavier tweaks needed that won't be solved by continuing\n> along the same line?  As _ext.h variant is included only at this place,\n> if we can do with minimum tweaks around here without introducing it,\n> it may be ideal, I would think.\n\nWell, a tricky part is that currently sha1dc_git.h is included from\nsha1dc/sha1.h implicitly by SHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_H\ndefinition.  IMO, we should stop this, and use the standard inclusion\ninstead, i.e. in hash.h,\n\n#if defined(DC_SHA1_EXTERNAL)\n#include <sha1dc/sha1.h>\n#elif defined(DC_SHA1_SUBMODULE)\n#include \"sha1collisiondetection/lib/sha1.h\"\n#else\n#include \"sha1dc/sha1.h\"\n#endif\n#include \"sha1dc_git.h\"\n\nIn sha1dc_git.h, we'd need another ifdef for DC_SHA1_EXTERNAL to\ndefine the own git_SHA1DCInit(), but it's trivial. \n\n\nTakashi\n"},{"id":"325397","messageId":"87ini7m5a6.fsf@gmail.com","threadId":"46461","inReplyTo":"s5hmv7jw9lx.wl-tiwai@suse.de","subject":"Re: [PATCH] hash: Allow building with the external sha1dc library","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-08-01T19:55:45Z","receivedAt":"2017-08-01T19:55:54Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 01 2017, Takashi Iwai jotted:\n\n> On Tue, 01 Aug 2017 17:56:00 +0200,\n> Junio C Hamano wrote:\n>>\n>> Takashi Iwai <tiwai@suse.de> writes:\n>>\n>> > On Fri, 28 Jul 2017 17:58:14 +0200,\n>> > Ævar Arnfjörð Bjarmason wrote:\n>> >>  ...\n>> >> * We now have much of the same header code copy/pasted between\n>> >> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always\n>> >> including the former but making what it's doing conditional on\n>> >> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory\n>> >> glance, but again your commit message doesn't list that among options\n>> >> considered & discarded.\n>> >\n>> > I don't mind either way, there is no perfect solution in this case.\n>> > As you know, many people think the ifdef ugly no matter how.\n>> >\n>> > I leave the decision to maintainer.  Just let me know which option is\n>> > preferred.\n>>\n>> Yeah, I also found it somewhat confusing to have these two headers\n>> that look quite similar to each other at the top-level of the tree.\n>>\n>> What's the \"conditional\" part between the two headers?  Is it just\n>> whether the header for underlying library is included?  I wonder if\n>> it's just the matter of adjusting \"hash.h\" to read like this\n>>\n>>     ...\n>>     #if defined(DC_SHA1_EXTERNAL)\n>>    -#include \"sha1dc_git_ext.h\"\n>>    +#include <sha1dc/sha1.h>\n>>    +#include \"sha1dc_git.h\"\n>>     #elif  defined(DC_SHA1_SUBMODULE)\n>>     ...\n>>\n>> or are there heavier tweaks needed that won't be solved by continuing\n>> along the same line?  As _ext.h variant is included only at this place,\n>> if we can do with minimum tweaks around here without introducing it,\n>> it may be ideal, I would think.\n>\n> Well, a tricky part is that currently sha1dc_git.h is included from\n> sha1dc/sha1.h implicitly by SHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_H\n> definition.  IMO, we should stop this, and use the standard inclusion\n> instead, i.e. in hash.h,\n\nIt's just like this because when I hacked up that facility I was making\nthe bare minimum change needed to not make local modifications to the\nupstream code. If there's better ways to do this in the presence of\nDC_SHA1_EXTERNAL & without that would be most welcome. Thanks.\n\n> #if defined(DC_SHA1_EXTERNAL)\n> #include <sha1dc/sha1.h>\n> #elif defined(DC_SHA1_SUBMODULE)\n> #include \"sha1collisiondetection/lib/sha1.h\"\n> #else\n> #include \"sha1dc/sha1.h\"\n> #endif\n> #include \"sha1dc_git.h\"\n>\n> In sha1dc_git.h, we'd need another ifdef for DC_SHA1_EXTERNAL to\n> define the own git_SHA1DCInit(), but it's trivial.\n>\n>\n> Takashi\n"},{"id":"325404","messageId":"s5hmv7j57xj.wl-tiwai@suse.de","threadId":"46461","inReplyTo":"87ini7m5a6.fsf@gmail.com","subject":"Re: [PATCH] hash: Allow building with the external sha1dc library","fromName":"Takashi Iwai","fromEmail":"tiwai@suse.de","sentAt":"2017-08-01T20:50:32Z","receivedAt":"2017-08-01T20:50:39Z","isPatch":true,"sender":{"key":"tiwai@suse.de","avatar":"https://avatars.githubusercontent.com/u/306482?v=4"},"body":"On Tue, 01 Aug 2017 21:55:45 +0200,\nÆvar Arnfjörð Bjarmason wrote:\n> \n> \n> On Tue, Aug 01 2017, Takashi Iwai jotted:\n> \n> > On Tue, 01 Aug 2017 17:56:00 +0200,\n> > Junio C Hamano wrote:\n> >>\n> >> Takashi Iwai <tiwai@suse.de> writes:\n> >>\n> >> > On Fri, 28 Jul 2017 17:58:14 +0200,\n> >> > Ævar Arnfjörð Bjarmason wrote:\n> >> >>  ...\n> >> >> * We now have much of the same header code copy/pasted between\n> >> >> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always\n> >> >> including the former but making what it's doing conditional on\n> >> >> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory\n> >> >> glance, but again your commit message doesn't list that among options\n> >> >> considered & discarded.\n> >> >\n> >> > I don't mind either way, there is no perfect solution in this case.\n> >> > As you know, many people think the ifdef ugly no matter how.\n> >> >\n> >> > I leave the decision to maintainer.  Just let me know which option is\n> >> > preferred.\n> >>\n> >> Yeah, I also found it somewhat confusing to have these two headers\n> >> that look quite similar to each other at the top-level of the tree.\n> >>\n> >> What's the \"conditional\" part between the two headers?  Is it just\n> >> whether the header for underlying library is included?  I wonder if\n> >> it's just the matter of adjusting \"hash.h\" to read like this\n> >>\n> >>     ...\n> >>     #if defined(DC_SHA1_EXTERNAL)\n> >>    -#include \"sha1dc_git_ext.h\"\n> >>    +#include <sha1dc/sha1.h>\n> >>    +#include \"sha1dc_git.h\"\n> >>     #elif  defined(DC_SHA1_SUBMODULE)\n> >>     ...\n> >>\n> >> or are there heavier tweaks needed that won't be solved by continuing\n> >> along the same line?  As _ext.h variant is included only at this place,\n> >> if we can do with minimum tweaks around here without introducing it,\n> >> it may be ideal, I would think.\n> >\n> > Well, a tricky part is that currently sha1dc_git.h is included from\n> > sha1dc/sha1.h implicitly by SHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_H\n> > definition.  IMO, we should stop this, and use the standard inclusion\n> > instead, i.e. in hash.h,\n> \n> It's just like this because when I hacked up that facility I was making\n> the bare minimum change needed to not make local modifications to the\n> upstream code. If there's better ways to do this in the presence of\n> DC_SHA1_EXTERNAL & without that would be most welcome. Thanks.\n\nCan't it be like the code below?  sha1.h is included only via hash.h,\nand sha1dc_git.h doesn't matter for the compile of sha1dc/*.c.\nOr am I missing something...?\n\n> > #if defined(DC_SHA1_EXTERNAL)\n> > #include <sha1dc/sha1.h>\n> > #elif defined(DC_SHA1_SUBMODULE)\n> > #include \"sha1collisiondetection/lib/sha1.h\"\n> > #else\n> > #include \"sha1dc/sha1.h\"\n> > #endif\n> > #include \"sha1dc_git.h\"\n\n\nthanks,\n\nTakashi\n\n\n\n> >\n> > In sha1dc_git.h, we'd need another ifdef for DC_SHA1_EXTERNAL to\n> > define the own git_SHA1DCInit(), but it's trivial.\n> >\n> >\n> > Takashi\n> \n"},{"id":"326184","messageId":"xmqqfucxy5u6.fsf@gitster.mtv.corp.google.com","threadId":"46461","inReplyTo":"CACBZZX7M=H8tNkZXpHBvv0rbY58EJk4dkoUzGKMftWoKUqF8sA@mail.gmail.com","subject":"Re: [PATCH] hash: Allow building with the external sha1dc library","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-12T00:42:25Z","receivedAt":"2017-08-12T00:42:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n> On Fri, Jul 28, 2017 at 5:58 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> I sent this last bit a tad too soon in a checkout of sha1collisiondetection.git:\n>\n>     $ make PREFIX=/tmp/local install >/dev/null 2>&1 && find /tmp/local/ -type f\n>     /tmp/local/include/sha1dc/sha1.h\n>     /tmp/local/bin/sha1dcsum\n>     /tmp/local/bin/sha1dcsum_partialcoll\n>     /tmp/local/lib/libsha1detectcoll.a\n>     /tmp/local/lib/libsha1detectcoll.so.1.0.0\n>     /tmp/local/lib/libsha1detectcoll.la\n>\n> So the upstream library expects you (and it's documented in their README) to do:\n>\n>     #include <sha1dc/sha1.h>\n>\n> But your patch is just doing:\n>\n>     #include <sha1.h>\n>\n> At best this seems like a trivial bug and at worst us encoding some\n> Suse-specific packaging convention in git, since other distros would\n> presumably want to package this in /usr/include/sha1dc/sha1.h as\n> upstream suggests. I.e. using the ambiguous sha1.h name is not\n> something upstream's doing by default, it's something you're doing in\n> your package.\n\nI do not think I saw any updates to this thread.  Should I consider\nthe topic ti/external-sha1dc now abandoned?\n\nAs we have finished Git 2.14 cycle, in preparation for the next one,\nthe 'next' branch will be rewound and rebuilt early next week.  I do\nnot mind tentatively ejecting some topics that needs fix-ups out of\n'next' to give them a clean restart.  If there will be a reroll that\naddresses the concerns raised during the discussion, please let me\nknow.\n\nThanks.\n\n"},{"id":"326191","messageId":"s5h1sohxos8.wl-tiwai@suse.de","threadId":"46461","inReplyTo":"xmqqfucxy5u6.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] hash: Allow building with the external sha1dc library","fromName":"Takashi Iwai","fromEmail":"tiwai@suse.de","sentAt":"2017-08-12T06:50:47Z","receivedAt":"2017-08-12T06:50:53Z","isPatch":true,"sender":{"key":"tiwai@suse.de","avatar":"https://avatars.githubusercontent.com/u/306482?v=4"},"body":"On Sat, 12 Aug 2017 02:42:25 +0200,\nJunio C Hamano wrote:\n> \n> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n> \n> > On Fri, Jul 28, 2017 at 5:58 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n> >\n> > I sent this last bit a tad too soon in a checkout of sha1collisiondetection.git:\n> >\n> >     $ make PREFIX=/tmp/local install >/dev/null 2>&1 && find /tmp/local/ -type f\n> >     /tmp/local/include/sha1dc/sha1.h\n> >     /tmp/local/bin/sha1dcsum\n> >     /tmp/local/bin/sha1dcsum_partialcoll\n> >     /tmp/local/lib/libsha1detectcoll.a\n> >     /tmp/local/lib/libsha1detectcoll.so.1.0.0\n> >     /tmp/local/lib/libsha1detectcoll.la\n> >\n> > So the upstream library expects you (and it's documented in their README) to do:\n> >\n> >     #include <sha1dc/sha1.h>\n> >\n> > But your patch is just doing:\n> >\n> >     #include <sha1.h>\n> >\n> > At best this seems like a trivial bug and at worst us encoding some\n> > Suse-specific packaging convention in git, since other distros would\n> > presumably want to package this in /usr/include/sha1dc/sha1.h as\n> > upstream suggests. I.e. using the ambiguous sha1.h name is not\n> > something upstream's doing by default, it's something you're doing in\n> > your package.\n> \n> I do not think I saw any updates to this thread.  Should I consider\n> the topic ti/external-sha1dc now abandoned?\n\nSorry for the silence, as I've been too busy for other tasks (there\nwere too many security bugs in the last weeks in many packages...)\n\n> As we have finished Git 2.14 cycle, in preparation for the next one,\n> the 'next' branch will be rewound and rebuilt early next week.  I do\n> not mind tentatively ejecting some topics that needs fix-ups out of\n> 'next' to give them a clean restart.  If there will be a reroll that\n> addresses the concerns raised during the discussion, please let me\n> know.\n\nFeel free to drop my branch, then.  I'm going to resubmit the fix in\nanyway, hopefully in the next week.\n\nOne thing I'm not entirely sure is about including the sha1.h as\n  #include <sha1dc/sha1.h>\n\nAlthough the header is installed to sha1dc/sha1.h as default in the\nupstream package, the intention of this sha1.h is a sort of\nreplacement of md1 (and possibly other) sha1.h.  It's a drop-in.\nSo in one side, including <sha1.h> with some include path is the\nintended behavior, while <sha1dc/sha1.h> would work (likely in a more\nsafer manner) in practice.\n\nI'm inclined to go for <sha1dc/sha1.h> and fix SUSE sha1dc package\nbefore that, but I'd like to hear from others, too.\n\n\nthanks,\n\nTakashi\n"}]}