[PATCH] hash: Allow building with the external sha1dc library

[PATCH] hash: Allow building with the external sha1dc library

[Takashi Iwai]

Some distros provide SHA1 collision detect code as a shared library.
It's the very same code as we have in git tree, and git can link with
it as well; at least, it may make maintenance easier, according to our
security guys.

This patch allows user to build git linking with the external sha1dc
library instead of the built-in sha1dc code.  User needs to define
DC_SHA1_EXTERNAL explicitly.  As default, the built-in sha1dc code is
used like before.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 Makefile         | 12 ++++++++++++
 hash.h           |  4 +++-
 sha1dc_git_ext.c | 11 +++++++++++
 sha1dc_git_ext.h | 25 +++++++++++++++++++++++++
 4 files changed, 51 insertions(+), 1 deletion(-)
 create mode 100644 sha1dc_git_ext.c
 create mode 100644 sha1dc_git_ext.h

diff --git a/Makefile b/Makefile
index 461c845d33cb..f1a262d56254 100644
--- a/Makefile
+++ b/Makefile
@@ -162,6 +162,12 @@ all::
 # algorithm. This is slower, but may detect attempted collision attacks.
 # Takes priority over other *_SHA1 knobs.
 #
+# Define DC_SHA1_EXTERNAL in addition to DC_SHA1 if you want to build / link
+# git with the external sha1collisiondetection library.
+# Without this option, i.e. the default behavior is to build git with its
+# own sha1dc code.  If any extra linker option is required, define them in
+# DC_SHA1_LINK variable in addition.
+#
 # Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
 # sha1collisiondetection shipped as a submodule instead of the
 # non-submodule copy in sha1dc/. This is an experimental option used
@@ -1472,6 +1478,11 @@ ifdef APPLE_COMMON_CRYPTO
 	BASIC_CFLAGS += -DSHA1_APPLE
 else
 	DC_SHA1 := YesPlease
+ifdef DC_SHA1_EXTERNAL
+	LIB_OBJS += sha1dc_git_ext.o
+	BASIC_CFLAGS += -DSHA1_DC -DDC_SHA1_EXTERNAL
+	EXTLIBS += $(DC_SHA1_LINK) -lsha1detectcoll
+else
 ifdef DC_SHA1_SUBMODULE
 	LIB_OBJS += sha1collisiondetection/lib/sha1.o
 	LIB_OBJS += sha1collisiondetection/lib/ubc_check.o
@@ -1492,6 +1503,7 @@ endif
 endif
 endif
 endif
+endif
 
 ifdef SHA1_MAX_BLOCK_SIZE
 	LIB_OBJS += compat/sha1-chunked.o
diff --git a/hash.h b/hash.h
index bef3e630a093..dce327d58d07 100644
--- a/hash.h
+++ b/hash.h
@@ -8,7 +8,9 @@
 #elif defined(SHA1_OPENSSL)
 #include <openssl/sha.h>
 #elif defined(SHA1_DC)
-#ifdef DC_SHA1_SUBMODULE
+#if defined(DC_SHA1_EXTERNAL)
+#include "sha1dc_git_ext.h"
+#elif defined(DC_SHA1_SUBMODULE)
 #include "sha1collisiondetection/lib/sha1.h"
 #else
 #include "sha1dc/sha1.h"
diff --git a/sha1dc_git_ext.c b/sha1dc_git_ext.c
new file mode 100644
index 000000000000..359439fc3d93
--- /dev/null
+++ b/sha1dc_git_ext.c
@@ -0,0 +1,11 @@
+/* Only for DC_SHA1_EXTERNAL; sharing the same hooks as built-in sha1dc */
+
+#include "cache.h"
+#include <sha1.h>
+#include "sha1dc_git.c"
+
+void git_SHA1DCInit(SHA1_CTX *ctx)
+{
+	SHA1DCInit(ctx);
+	SHA1DCSetSafeHash(ctx, 0);
+}
diff --git a/sha1dc_git_ext.h b/sha1dc_git_ext.h
new file mode 100644
index 000000000000..d0ea8ce518db
--- /dev/null
+++ b/sha1dc_git_ext.h
@@ -0,0 +1,25 @@
+/*
+ * This file is included by hash.h for DC_SHA1_EXTERNAL
+ */
+
+#include <sha1.h>
+
+/*
+ * Same as SHA1DCInit, but with default save_hash=0
+ */
+void git_SHA1DCInit(SHA1_CTX *);
+
+/*
+ * Same as SHA1DCFinal, but convert collision attack case into a verbose die().
+ */
+void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);
+
+/*
+ * Same as SHA1DCUpdate, but adjust types to match git's usual interface.
+ */
+void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);
+
+#define platform_SHA_CTX SHA1_CTX
+#define platform_SHA1_Init git_SHA1DCInit
+#define platform_SHA1_Update git_SHA1DCUpdate
+#define platform_SHA1_Final git_SHA1DCFinal
-- 
2.13.3

Re: [PATCH] hash: Allow building with the external sha1dc library

[Junio C Hamano]

Takashi Iwai <tiwai@suse.de> writes:

> Some distros provide SHA1 collision detect code as a shared library.
> It's the very same code as we have in git tree, and git can link with
> it as well; at least, it may make maintenance easier, according to our
> security guys.
>
> This patch allows user to build git linking with the external sha1dc
> library instead of the built-in sha1dc code.  User needs to define
> DC_SHA1_EXTERNAL explicitly.  As default, the built-in sha1dc code is
> used like before.
>
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---

I do not have such an environment to test this patch, but it looks
like a very sensible thing to do.  Will queue; thanks.

>  Makefile         | 12 ++++++++++++
>  hash.h           |  4 +++-
>  sha1dc_git_ext.c | 11 +++++++++++
>  sha1dc_git_ext.h | 25 +++++++++++++++++++++++++
>  4 files changed, 51 insertions(+), 1 deletion(-)
>  create mode 100644 sha1dc_git_ext.c
>  create mode 100644 sha1dc_git_ext.h
>
> diff --git a/Makefile b/Makefile
> index 461c845d33cb..f1a262d56254 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -162,6 +162,12 @@ all::
>  # algorithm. This is slower, but may detect attempted collision attacks.
>  # Takes priority over other *_SHA1 knobs.
>  #
> +# Define DC_SHA1_EXTERNAL in addition to DC_SHA1 if you want to build / link
> +# git with the external sha1collisiondetection library.
> +# Without this option, i.e. the default behavior is to build git with its
> +# own sha1dc code.  If any extra linker option is required, define them in
> +# DC_SHA1_LINK variable in addition.
> +#
>  # Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
>  # sha1collisiondetection shipped as a submodule instead of the
>  # non-submodule copy in sha1dc/. This is an experimental option used
> @@ -1472,6 +1478,11 @@ ifdef APPLE_COMMON_CRYPTO
>  	BASIC_CFLAGS += -DSHA1_APPLE
>  else
>  	DC_SHA1 := YesPlease
> +ifdef DC_SHA1_EXTERNAL
> +	LIB_OBJS += sha1dc_git_ext.o
> +	BASIC_CFLAGS += -DSHA1_DC -DDC_SHA1_EXTERNAL
> +	EXTLIBS += $(DC_SHA1_LINK) -lsha1detectcoll
> +else
>  ifdef DC_SHA1_SUBMODULE
>  	LIB_OBJS += sha1collisiondetection/lib/sha1.o
>  	LIB_OBJS += sha1collisiondetection/lib/ubc_check.o
> @@ -1492,6 +1503,7 @@ endif
>  endif
>  endif
>  endif
> +endif
>  
>  ifdef SHA1_MAX_BLOCK_SIZE
>  	LIB_OBJS += compat/sha1-chunked.o
> diff --git a/hash.h b/hash.h
> index bef3e630a093..dce327d58d07 100644
> --- a/hash.h
> +++ b/hash.h
> @@ -8,7 +8,9 @@
>  #elif defined(SHA1_OPENSSL)
>  #include <openssl/sha.h>
>  #elif defined(SHA1_DC)
> -#ifdef DC_SHA1_SUBMODULE
> +#if defined(DC_SHA1_EXTERNAL)
> +#include "sha1dc_git_ext.h"
> +#elif defined(DC_SHA1_SUBMODULE)
>  #include "sha1collisiondetection/lib/sha1.h"
>  #else
>  #include "sha1dc/sha1.h"
> diff --git a/sha1dc_git_ext.c b/sha1dc_git_ext.c
> new file mode 100644
> index 000000000000..359439fc3d93
> --- /dev/null
> +++ b/sha1dc_git_ext.c
> @@ -0,0 +1,11 @@
> +/* Only for DC_SHA1_EXTERNAL; sharing the same hooks as built-in sha1dc */
> +
> +#include "cache.h"
> +#include <sha1.h>
> +#include "sha1dc_git.c"
> +
> +void git_SHA1DCInit(SHA1_CTX *ctx)
> +{
> +	SHA1DCInit(ctx);
> +	SHA1DCSetSafeHash(ctx, 0);
> +}
> diff --git a/sha1dc_git_ext.h b/sha1dc_git_ext.h
> new file mode 100644
> index 000000000000..d0ea8ce518db
> --- /dev/null
> +++ b/sha1dc_git_ext.h
> @@ -0,0 +1,25 @@
> +/*
> + * This file is included by hash.h for DC_SHA1_EXTERNAL
> + */
> +
> +#include <sha1.h>
> +
> +/*
> + * Same as SHA1DCInit, but with default save_hash=0
> + */
> +void git_SHA1DCInit(SHA1_CTX *);
> +
> +/*
> + * Same as SHA1DCFinal, but convert collision attack case into a verbose die().
> + */
> +void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);
> +
> +/*
> + * Same as SHA1DCUpdate, but adjust types to match git's usual interface.
> + */
> +void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);
> +
> +#define platform_SHA_CTX SHA1_CTX
> +#define platform_SHA1_Init git_SHA1DCInit
> +#define platform_SHA1_Update git_SHA1DCUpdate
> +#define platform_SHA1_Final git_SHA1DCFinal

Re: [PATCH] hash: Allow building with the external sha1dc library

[Ævar Arnfjörð Bjarmason]

On Tue, Jul 25, 2017 at 7:57 AM, Takashi Iwai <tiwai@suse.de> wrote:
> Some distros provide SHA1 collision detect code as a shared library.
> It's the very same code as we have in git tree, and git can link with
> it as well; at least, it may make maintenance easier, according to our
> security guys.
>
> This patch allows user to build git linking with the external sha1dc
> library instead of the built-in sha1dc code.  User needs to define
> DC_SHA1_EXTERNAL explicitly.  As default, the built-in sha1dc code is
> used like before.

This whole thing sounds sensible. I reviewed this (but like Junio
haven't tested it with a lib) and I think it would be worth noting the
following in the commit message / Makefile documentation:

* The "sha1detectcoll" *.so name for the "sha1collisiondetection"
library is not something you or suse presumably) made up, it's a name
the sha1collisiondetection.git itself creates for its library. I think
the Makefile docs you've added here are a bit confusing, you talk
about the "external sha1collisiondetection library" but then link
against sha1detectcoll". It's worth calling out this difference in the
docs IMO. I.e. not talk about the sha1detectcoll.so library form of
sha1collisiondetection, not the sha1collisiondetection project name as
a library.

* It might be worth noting that this is *not* linking against the same
code we ship ourselves due to the difference in defining
SHA1DC_INIT_SAFE_HASH_DEFAULT for the git project's needs in the one
we build, hence your need to have a git_SHA1DCInit() wrapper whereas
we call SHA1DCInit() directly. It might be interesting to note that
the library version will always be *slightly* slower (although the
difference will be trivial).

* Nothing in your commit message or docs explains why DC_SHA1_LINK is
needed. We don't have these sorts of variables for other external
libraries we link to, why the difference?

Some other things I observed:

* We now have much of the same header code copy/pasted between
sha1dc_git.h and sha1dc_git_ext.h, did you consider just always
including the former but making what it's doing conditional on
DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory
glance, but again your commit message doesn't list that among options
considered & discarded.

* I think it makes sense to spew out a "not both!" error in the
Makefile if you set DC_SHA1_EXTERNAL=Y and DC_SHA1_SUBMODULE=Y. See my
94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) for an
example of how to do this.

* The whole business of "#include <sha1.h>" looks very fragile, are
there really no other packages in e.g. suse that ship a sha1.h? Debian
has libmd-dev that ships /usr/include/sha1.h that conflicts with this:
https://packages.debian.org/search?searchon=contents&keywords=sha1.h&mode=exactfilename&suite=unstable&arch=any

Shipping a sha1.h as opposed to a sha1collisiondetection.h or
sha1detectcoll.h or whatever seems like a *really* bad decision by
upstream that should be the subject of at least seeing if they'll take
a pull request to fix it before you package it or before we include
something that'll probably need to be fixed / worked around anyway in
Git.

> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  Makefile         | 12 ++++++++++++
>  hash.h           |  4 +++-
>  sha1dc_git_ext.c | 11 +++++++++++
>  sha1dc_git_ext.h | 25 +++++++++++++++++++++++++
>  4 files changed, 51 insertions(+), 1 deletion(-)
>  create mode 100644 sha1dc_git_ext.c
>  create mode 100644 sha1dc_git_ext.h
>
> diff --git a/Makefile b/Makefile
> index 461c845d33cb..f1a262d56254 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -162,6 +162,12 @@ all::
>  # algorithm. This is slower, but may detect attempted collision attacks.
>  # Takes priority over other *_SHA1 knobs.
>  #
> +# Define DC_SHA1_EXTERNAL in addition to DC_SHA1 if you want to build / link
> +# git with the external sha1collisiondetection library.
> +# Without this option, i.e. the default behavior is to build git with its
> +# own sha1dc code.  If any extra linker option is required, define them in
> +# DC_SHA1_LINK variable in addition.
> +#
>  # Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
>  # sha1collisiondetection shipped as a submodule instead of the
>  # non-submodule copy in sha1dc/. This is an experimental option used
> @@ -1472,6 +1478,11 @@ ifdef APPLE_COMMON_CRYPTO
>         BASIC_CFLAGS += -DSHA1_APPLE
>  else
>         DC_SHA1 := YesPlease
> +ifdef DC_SHA1_EXTERNAL
> +       LIB_OBJS += sha1dc_git_ext.o
> +       BASIC_CFLAGS += -DSHA1_DC -DDC_SHA1_EXTERNAL
> +       EXTLIBS += $(DC_SHA1_LINK) -lsha1detectcoll
> +else
>  ifdef DC_SHA1_SUBMODULE
>         LIB_OBJS += sha1collisiondetection/lib/sha1.o
>         LIB_OBJS += sha1collisiondetection/lib/ubc_check.o
> @@ -1492,6 +1503,7 @@ endif
>  endif
>  endif
>  endif
> +endif
>
>  ifdef SHA1_MAX_BLOCK_SIZE
>         LIB_OBJS += compat/sha1-chunked.o
> diff --git a/hash.h b/hash.h
> index bef3e630a093..dce327d58d07 100644
> --- a/hash.h
> +++ b/hash.h
> @@ -8,7 +8,9 @@
>  #elif defined(SHA1_OPENSSL)
>  #include <openssl/sha.h>
>  #elif defined(SHA1_DC)
> -#ifdef DC_SHA1_SUBMODULE
> +#if defined(DC_SHA1_EXTERNAL)
> +#include "sha1dc_git_ext.h"
> +#elif defined(DC_SHA1_SUBMODULE)
>  #include "sha1collisiondetection/lib/sha1.h"
>  #else
>  #include "sha1dc/sha1.h"
> diff --git a/sha1dc_git_ext.c b/sha1dc_git_ext.c
> new file mode 100644
> index 000000000000..359439fc3d93
> --- /dev/null
> +++ b/sha1dc_git_ext.c
> @@ -0,0 +1,11 @@
> +/* Only for DC_SHA1_EXTERNAL; sharing the same hooks as built-in sha1dc */
> +
> +#include "cache.h"
> +#include <sha1.h>
> +#include "sha1dc_git.c"
> +
> +void git_SHA1DCInit(SHA1_CTX *ctx)
> +{
> +       SHA1DCInit(ctx);
> +       SHA1DCSetSafeHash(ctx, 0);
> +}



> diff --git a/sha1dc_git_ext.h b/sha1dc_git_ext.h
> new file mode 100644
> index 000000000000..d0ea8ce518db
> --- /dev/null
> +++ b/sha1dc_git_ext.h
> @@ -0,0 +1,25 @@
> +/*
> + * This file is included by hash.h for DC_SHA1_EXTERNAL
> + */
> +
> +#include <sha1.h>
> +
> +/*
> + * Same as SHA1DCInit, but with default save_hash=0
> + */
> +void git_SHA1DCInit(SHA1_CTX *);
> +
> +/*
> + * Same as SHA1DCFinal, but convert collision attack case into a verbose die().
> + */
> +void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);
> +
> +/*
> + * Same as SHA1DCUpdate, but adjust types to match git's usual interface.
> + */
> +void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);
> +
> +#define platform_SHA_CTX SHA1_CTX
> +#define platform_SHA1_Init git_SHA1DCInit
> +#define platform_SHA1_Update git_SHA1DCUpdate
> +#define platform_SHA1_Final git_SHA1DCFinal
> --
> 2.13.3
>

Re: [PATCH] hash: Allow building with the external sha1dc library

[Ævar Arnfjörð Bjarmason]

On Fri, Jul 28, 2017 at 5:58 PM, Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Tue, Jul 25, 2017 at 7:57 AM, Takashi Iwai <tiwai@suse.de> wrote:
>> Some distros provide SHA1 collision detect code as a shared library.
>> It's the very same code as we have in git tree, and git can link with
>> it as well; at least, it may make maintenance easier, according to our
>> security guys.
>>
>> This patch allows user to build git linking with the external sha1dc
>> library instead of the built-in sha1dc code.  User needs to define
>> DC_SHA1_EXTERNAL explicitly.  As default, the built-in sha1dc code is
>> used like before.
>
> This whole thing sounds sensible. I reviewed this (but like Junio
> haven't tested it with a lib) and I think it would be worth noting the
> following in the commit message / Makefile documentation:
>
> * The "sha1detectcoll" *.so name for the "sha1collisiondetection"
> library is not something you or suse presumably) made up, it's a name
> the sha1collisiondetection.git itself creates for its library. I think
> the Makefile docs you've added here are a bit confusing, you talk
> about the "external sha1collisiondetection library" but then link
> against sha1detectcoll". It's worth calling out this difference in the
> docs IMO. I.e. not talk about the sha1detectcoll.so library form of
> sha1collisiondetection, not the sha1collisiondetection project name as
> a library.
>
> * It might be worth noting that this is *not* linking against the same
> code we ship ourselves due to the difference in defining
> SHA1DC_INIT_SAFE_HASH_DEFAULT for the git project's needs in the one
> we build, hence your need to have a git_SHA1DCInit() wrapper whereas
> we call SHA1DCInit() directly. It might be interesting to note that
> the library version will always be *slightly* slower (although the
> difference will be trivial).
>
> * Nothing in your commit message or docs explains why DC_SHA1_LINK is
> needed. We don't have these sorts of variables for other external
> libraries we link to, why the difference?
>
> Some other things I observed:
>
> * We now have much of the same header code copy/pasted between
> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always
> including the former but making what it's doing conditional on
> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory
> glance, but again your commit message doesn't list that among options
> considered & discarded.
>
> * I think it makes sense to spew out a "not both!" error in the
> Makefile if you set DC_SHA1_EXTERNAL=Y and DC_SHA1_SUBMODULE=Y. See my
> 94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) for an
> example of how to do this.
>
> * The whole business of "#include <sha1.h>" looks very fragile, are
> there really no other packages in e.g. suse that ship a sha1.h? Debian
> has libmd-dev that ships /usr/include/sha1.h that conflicts with this:
> https://packages.debian.org/search?searchon=contents&keywords=sha1.h&mode=exactfilename&suite=unstable&arch=any
>
> Shipping a sha1.h as opposed to a sha1collisiondetection.h or
> sha1detectcoll.h or whatever seems like a *really* bad decision by
> upstream that should be the subject of at least seeing if they'll take
> a pull request to fix it before you package it or before we include
> something that'll probably need to be fixed / worked around anyway in
> Git.

I sent this last bit a tad too soon in a checkout of sha1collisiondetection.git:

    $ make PREFIX=/tmp/local install >/dev/null 2>&1 && find /tmp/local/ -type f
    /tmp/local/include/sha1dc/sha1.h
    /tmp/local/bin/sha1dcsum
    /tmp/local/bin/sha1dcsum_partialcoll
    /tmp/local/lib/libsha1detectcoll.a
    /tmp/local/lib/libsha1detectcoll.so.1.0.0
    /tmp/local/lib/libsha1detectcoll.la

So the upstream library expects you (and it's documented in their README) to do:

    #include <sha1dc/sha1.h>

But your patch is just doing:

    #include <sha1.h>

At best this seems like a trivial bug and at worst us encoding some
Suse-specific packaging convention in git, since other distros would
presumably want to package this in /usr/include/sha1dc/sha1.h as
upstream suggests. I.e. using the ambiguous sha1.h name is not
something upstream's doing by default, it's something you're doing in
your package.

>> Signed-off-by: Takashi Iwai <tiwai@suse.de>
>> ---
>>  Makefile         | 12 ++++++++++++
>>  hash.h           |  4 +++-
>>  sha1dc_git_ext.c | 11 +++++++++++
>>  sha1dc_git_ext.h | 25 +++++++++++++++++++++++++
>>  4 files changed, 51 insertions(+), 1 deletion(-)
>>  create mode 100644 sha1dc_git_ext.c
>>  create mode 100644 sha1dc_git_ext.h
>>
>> diff --git a/Makefile b/Makefile
>> index 461c845d33cb..f1a262d56254 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -162,6 +162,12 @@ all::
>>  # algorithm. This is slower, but may detect attempted collision attacks.
>>  # Takes priority over other *_SHA1 knobs.
>>  #
>> +# Define DC_SHA1_EXTERNAL in addition to DC_SHA1 if you want to build / link
>> +# git with the external sha1collisiondetection library.
>> +# Without this option, i.e. the default behavior is to build git with its
>> +# own sha1dc code.  If any extra linker option is required, define them in
>> +# DC_SHA1_LINK variable in addition.
>> +#
>>  # Define DC_SHA1_SUBMODULE in addition to DC_SHA1 to use the
>>  # sha1collisiondetection shipped as a submodule instead of the
>>  # non-submodule copy in sha1dc/. This is an experimental option used
>> @@ -1472,6 +1478,11 @@ ifdef APPLE_COMMON_CRYPTO
>>         BASIC_CFLAGS += -DSHA1_APPLE
>>  else
>>         DC_SHA1 := YesPlease
>> +ifdef DC_SHA1_EXTERNAL
>> +       LIB_OBJS += sha1dc_git_ext.o
>> +       BASIC_CFLAGS += -DSHA1_DC -DDC_SHA1_EXTERNAL
>> +       EXTLIBS += $(DC_SHA1_LINK) -lsha1detectcoll
>> +else
>>  ifdef DC_SHA1_SUBMODULE
>>         LIB_OBJS += sha1collisiondetection/lib/sha1.o
>>         LIB_OBJS += sha1collisiondetection/lib/ubc_check.o
>> @@ -1492,6 +1503,7 @@ endif
>>  endif
>>  endif
>>  endif
>> +endif
>>
>>  ifdef SHA1_MAX_BLOCK_SIZE
>>         LIB_OBJS += compat/sha1-chunked.o
>> diff --git a/hash.h b/hash.h
>> index bef3e630a093..dce327d58d07 100644
>> --- a/hash.h
>> +++ b/hash.h
>> @@ -8,7 +8,9 @@
>>  #elif defined(SHA1_OPENSSL)
>>  #include <openssl/sha.h>
>>  #elif defined(SHA1_DC)
>> -#ifdef DC_SHA1_SUBMODULE
>> +#if defined(DC_SHA1_EXTERNAL)
>> +#include "sha1dc_git_ext.h"
>> +#elif defined(DC_SHA1_SUBMODULE)
>>  #include "sha1collisiondetection/lib/sha1.h"
>>  #else
>>  #include "sha1dc/sha1.h"
>> diff --git a/sha1dc_git_ext.c b/sha1dc_git_ext.c
>> new file mode 100644
>> index 000000000000..359439fc3d93
>> --- /dev/null
>> +++ b/sha1dc_git_ext.c
>> @@ -0,0 +1,11 @@
>> +/* Only for DC_SHA1_EXTERNAL; sharing the same hooks as built-in sha1dc */
>> +
>> +#include "cache.h"
>> +#include <sha1.h>
>> +#include "sha1dc_git.c"
>> +
>> +void git_SHA1DCInit(SHA1_CTX *ctx)
>> +{
>> +       SHA1DCInit(ctx);
>> +       SHA1DCSetSafeHash(ctx, 0);
>> +}
>
>
>
>> diff --git a/sha1dc_git_ext.h b/sha1dc_git_ext.h
>> new file mode 100644
>> index 000000000000..d0ea8ce518db
>> --- /dev/null
>> +++ b/sha1dc_git_ext.h
>> @@ -0,0 +1,25 @@
>> +/*
>> + * This file is included by hash.h for DC_SHA1_EXTERNAL
>> + */
>> +
>> +#include <sha1.h>
>> +
>> +/*
>> + * Same as SHA1DCInit, but with default save_hash=0
>> + */
>> +void git_SHA1DCInit(SHA1_CTX *);
>> +
>> +/*
>> + * Same as SHA1DCFinal, but convert collision attack case into a verbose die().
>> + */
>> +void git_SHA1DCFinal(unsigned char [20], SHA1_CTX *);
>> +
>> +/*
>> + * Same as SHA1DCUpdate, but adjust types to match git's usual interface.
>> + */
>> +void git_SHA1DCUpdate(SHA1_CTX *ctx, const void *data, unsigned long len);
>> +
>> +#define platform_SHA_CTX SHA1_CTX
>> +#define platform_SHA1_Init git_SHA1DCInit
>> +#define platform_SHA1_Update git_SHA1DCUpdate
>> +#define platform_SHA1_Final git_SHA1DCFinal
>> --
>> 2.13.3
>>

Re: [PATCH] hash: Allow building with the external sha1dc library

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> So the upstream library expects you (and it's documented in their README) to do:
>
>     #include <sha1dc/sha1.h>
>
> But your patch is just doing:
>
>     #include <sha1.h>
>
> At best this seems like a trivial bug and at worst us encoding some
> Suse-specific packaging convention in git, since other distros would
> presumably want to package this in /usr/include/sha1dc/sha1.h as
> upstream suggests. I.e. using the ambiguous sha1.h name is not
> something upstream's doing by default, it's something you're doing in
> your package.

It seems there still needs a bit more work on this patch.  Thanks
for reviewing and pointing out what needs to be addressed.

Re: [PATCH] hash: Allow building with the external sha1dc library

[Takashi Iwai]

On Fri, 28 Jul 2017 18:04:18 +0200,
Ævar Arnfjörð Bjarmason wrote:
> 
> On Fri, Jul 28, 2017 at 5:58 PM, Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
> > On Tue, Jul 25, 2017 at 7:57 AM, Takashi Iwai <tiwai@suse.de> wrote:
> >> Some distros provide SHA1 collision detect code as a shared library.
> >> It's the very same code as we have in git tree, and git can link with
> >> it as well; at least, it may make maintenance easier, according to our
> >> security guys.
> >>
> >> This patch allows user to build git linking with the external sha1dc
> >> library instead of the built-in sha1dc code.  User needs to define
> >> DC_SHA1_EXTERNAL explicitly.  As default, the built-in sha1dc code is
> >> used like before.
> >
> > This whole thing sounds sensible. I reviewed this (but like Junio
> > haven't tested it with a lib) and I think it would be worth noting the
> > following in the commit message / Makefile documentation:
> >
> > * The "sha1detectcoll" *.so name for the "sha1collisiondetection"
> > library is not something you or suse presumably) made up, it's a name
> > the sha1collisiondetection.git itself creates for its library. I think
> > the Makefile docs you've added here are a bit confusing, you talk
> > about the "external sha1collisiondetection library" but then link
> > against sha1detectcoll". It's worth calling out this difference in the
> > docs IMO. I.e. not talk about the sha1detectcoll.so library form of
> > sha1collisiondetection, not the sha1collisiondetection project name as
> > a library.
> >
> > * It might be worth noting that this is *not* linking against the same
> > code we ship ourselves due to the difference in defining
> > SHA1DC_INIT_SAFE_HASH_DEFAULT for the git project's needs in the one
> > we build, hence your need to have a git_SHA1DCInit() wrapper whereas
> > we call SHA1DCInit() directly. It might be interesting to note that
> > the library version will always be *slightly* slower (although the
> > difference will be trivial).
> >
> > * Nothing in your commit message or docs explains why DC_SHA1_LINK is
> > needed. We don't have these sorts of variables for other external
> > libraries we link to, why the difference?
> >
> > Some other things I observed:
> >
> > * We now have much of the same header code copy/pasted between
> > sha1dc_git.h and sha1dc_git_ext.h, did you consider just always
> > including the former but making what it's doing conditional on
> > DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory
> > glance, but again your commit message doesn't list that among options
> > considered & discarded.
> >
> > * I think it makes sense to spew out a "not both!" error in the
> > Makefile if you set DC_SHA1_EXTERNAL=Y and DC_SHA1_SUBMODULE=Y. See my
> > 94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) for an
> > example of how to do this.
> >
> > * The whole business of "#include <sha1.h>" looks very fragile, are
> > there really no other packages in e.g. suse that ship a sha1.h? Debian
> > has libmd-dev that ships /usr/include/sha1.h that conflicts with this:
> > https://packages.debian.org/search?searchon=contents&keywords=sha1.h&mode=exactfilename&suite=unstable&arch=any
> >
> > Shipping a sha1.h as opposed to a sha1collisiondetection.h or
> > sha1detectcoll.h or whatever seems like a *really* bad decision by
> > upstream that should be the subject of at least seeing if they'll take
> > a pull request to fix it before you package it or before we include
> > something that'll probably need to be fixed / worked around anyway in
> > Git.
> 
> I sent this last bit a tad too soon in a checkout of sha1collisiondetection.git:
> 
>     $ make PREFIX=/tmp/local install >/dev/null 2>&1 && find /tmp/local/ -type f
>     /tmp/local/include/sha1dc/sha1.h
>     /tmp/local/bin/sha1dcsum
>     /tmp/local/bin/sha1dcsum_partialcoll
>     /tmp/local/lib/libsha1detectcoll.a
>     /tmp/local/lib/libsha1detectcoll.so.1.0.0
>     /tmp/local/lib/libsha1detectcoll.la
> 
> So the upstream library expects you (and it's documented in their README) to do:
> 
>     #include <sha1dc/sha1.h>
> 
> But your patch is just doing:
> 
>     #include <sha1.h>
>
> At best this seems like a trivial bug and at worst us encoding some
> Suse-specific packaging convention in git, since other distros would
> presumably want to package this in /usr/include/sha1dc/sha1.h as
> upstream suggests. I.e. using the ambiguous sha1.h name is not
> something upstream's doing by default, it's something you're doing in
> your package.

Actually it seems to be a wrong usage of $INCLUDEDIR in the upstream
Makefile, and SUSE package blindly override $INCLUDEDIR.

But sha1dc/sha1.h looks like the correct path, as README.md mentions,
indeed, so maybe we need to work around in SUSE package side.

Andreas, could you work on it please?


thanks,

Takashi

Re: [PATCH] hash: Allow building with the external sha1dc library

[Junio C Hamano]

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Fri, Jul 28, 2017 at 5:58 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>
> I sent this last bit a tad too soon in a checkout of sha1collisiondetection.git:
>
>     $ make PREFIX=/tmp/local install >/dev/null 2>&1 && find /tmp/local/ -type f
>     /tmp/local/include/sha1dc/sha1.h
>     /tmp/local/bin/sha1dcsum
>     /tmp/local/bin/sha1dcsum_partialcoll
>     /tmp/local/lib/libsha1detectcoll.a
>     /tmp/local/lib/libsha1detectcoll.so.1.0.0
>     /tmp/local/lib/libsha1detectcoll.la
>
> So the upstream library expects you (and it's documented in their README) to do:
>
>     #include <sha1dc/sha1.h>
>
> But your patch is just doing:
>
>     #include <sha1.h>
>
> At best this seems like a trivial bug and at worst us encoding some
> Suse-specific packaging convention in git, since other distros would
> presumably want to package this in /usr/include/sha1dc/sha1.h as
> upstream suggests. I.e. using the ambiguous sha1.h name is not
> something upstream's doing by default, it's something you're doing in
> your package.

I do not think I saw any updates to this thread.  Should I consider
the topic ti/external-sha1dc now abandoned?

As we have finished Git 2.14 cycle, in preparation for the next one,
the 'next' branch will be rewound and rebuilt early next week.  I do
not mind tentatively ejecting some topics that needs fix-ups out of
'next' to give them a clean restart.  If there will be a reroll that
addresses the concerns raised during the discussion, please let me
know.

Thanks.

Re: [PATCH] hash: Allow building with the external sha1dc library

[Takashi Iwai]

On Sat, 12 Aug 2017 02:42:25 +0200,
Junio C Hamano wrote:
> 
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
> 
> > On Fri, Jul 28, 2017 at 5:58 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> >
> > I sent this last bit a tad too soon in a checkout of sha1collisiondetection.git:
> >
> >     $ make PREFIX=/tmp/local install >/dev/null 2>&1 && find /tmp/local/ -type f
> >     /tmp/local/include/sha1dc/sha1.h
> >     /tmp/local/bin/sha1dcsum
> >     /tmp/local/bin/sha1dcsum_partialcoll
> >     /tmp/local/lib/libsha1detectcoll.a
> >     /tmp/local/lib/libsha1detectcoll.so.1.0.0
> >     /tmp/local/lib/libsha1detectcoll.la
> >
> > So the upstream library expects you (and it's documented in their README) to do:
> >
> >     #include <sha1dc/sha1.h>
> >
> > But your patch is just doing:
> >
> >     #include <sha1.h>
> >
> > At best this seems like a trivial bug and at worst us encoding some
> > Suse-specific packaging convention in git, since other distros would
> > presumably want to package this in /usr/include/sha1dc/sha1.h as
> > upstream suggests. I.e. using the ambiguous sha1.h name is not
> > something upstream's doing by default, it's something you're doing in
> > your package.
> 
> I do not think I saw any updates to this thread.  Should I consider
> the topic ti/external-sha1dc now abandoned?

Sorry for the silence, as I've been too busy for other tasks (there
were too many security bugs in the last weeks in many packages...)

> As we have finished Git 2.14 cycle, in preparation for the next one,
> the 'next' branch will be rewound and rebuilt early next week.  I do
> not mind tentatively ejecting some topics that needs fix-ups out of
> 'next' to give them a clean restart.  If there will be a reroll that
> addresses the concerns raised during the discussion, please let me
> know.

Feel free to drop my branch, then.  I'm going to resubmit the fix in
anyway, hopefully in the next week.

One thing I'm not entirely sure is about including the sha1.h as
  #include <sha1dc/sha1.h>

Although the header is installed to sha1dc/sha1.h as default in the
upstream package, the intention of this sha1.h is a sort of
replacement of md1 (and possibly other) sha1.h.  It's a drop-in.
So in one side, including <sha1.h> with some include path is the
intended behavior, while <sha1dc/sha1.h> would work (likely in a more
safer manner) in practice.

I'm inclined to go for <sha1dc/sha1.h> and fix SUSE sha1dc package
before that, but I'd like to hear from others, too.


thanks,

Takashi

Re: [PATCH] hash: Allow building with the external sha1dc library

[Takashi Iwai]

On Fri, 28 Jul 2017 17:58:14 +0200,
Ævar Arnfjörð Bjarmason wrote:
> 
> On Tue, Jul 25, 2017 at 7:57 AM, Takashi Iwai <tiwai@suse.de> wrote:
> > Some distros provide SHA1 collision detect code as a shared library.
> > It's the very same code as we have in git tree, and git can link with
> > it as well; at least, it may make maintenance easier, according to our
> > security guys.
> >
> > This patch allows user to build git linking with the external sha1dc
> > library instead of the built-in sha1dc code.  User needs to define
> > DC_SHA1_EXTERNAL explicitly.  As default, the built-in sha1dc code is
> > used like before.
> 
> This whole thing sounds sensible. I reviewed this (but like Junio
> haven't tested it with a lib) and I think it would be worth noting the
> following in the commit message / Makefile documentation:

Hi,

sorry for the late reply, and thanks for the review.

> * The "sha1detectcoll" *.so name for the "sha1collisiondetection"
> library is not something you or suse presumably) made up, it's a name
> the sha1collisiondetection.git itself creates for its library. I think
> the Makefile docs you've added here are a bit confusing, you talk
> about the "external sha1collisiondetection library" but then link
> against sha1detectcoll". It's worth calling out this difference in the
> docs IMO. I.e. not talk about the sha1detectcoll.so library form of
> sha1collisiondetection, not the sha1collisiondetection project name as
> a library.

Heh, I was confused and annoyed by this inconsistency, too :)
IIRC, I used "sha1collisiondetection" since the text for
DC_SHA1_SUBMODULE refers to this term already.

I'll keep using the more readable term (e.g. SHA1 collision-detection
or sha1dc as abbreviated form) instead of the project name.

> * It might be worth noting that this is *not* linking against the same
> code we ship ourselves due to the difference in defining
> SHA1DC_INIT_SAFE_HASH_DEFAULT for the git project's needs in the one
> we build, hence your need to have a git_SHA1DCInit() wrapper whereas
> we call SHA1DCInit() directly. It might be interesting to note that
> the library version will always be *slightly* slower (although the
> difference will be trivial).

Well, it's just a matter of definition of "code".  If you think of the
source code, it is the same code.  The difference is merely the build
option and how it's compiled.  The performance difference is also not
worth to note.  If the call pattern differs due to shlib, it does
change no matter whether it's with the same option or not.  Using
shlib always implies that, you must know it.

In anyway, I'm going to rephrase "code" with "source code" to avoid
confusion.

> * Nothing in your commit message or docs explains why DC_SHA1_LINK is
> needed. We don't have these sorts of variables for other external
> libraries we link to, why the difference?

IMO, it's other way round.  If we didn't provide any such option for
using external library, we were too lazy.

Actually, I was lazy and didn't provide DC_SHA1_CFLAGS, and you've
proven this to be bad -- the library header might be installed in a
different location than the standard path.  The DC_SHA1_CFLAGS can be
set as default to -Isha1dc so that it covers that path.

I'll add this to the revised patch.

> Some other things I observed:
> 
> * We now have much of the same header code copy/pasted between
> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always
> including the former but making what it's doing conditional on
> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory
> glance, but again your commit message doesn't list that among options
> considered & discarded.

I don't mind either way, there is no perfect solution in this case.
As you know, many people think the ifdef ugly no matter how.

I leave the decision to maintainer.  Just let me know which option is
preferred.


> * I think it makes sense to spew out a "not both!" error in the
> Makefile if you set DC_SHA1_EXTERNAL=Y and DC_SHA1_SUBMODULE=Y. See my
> 94da9193a6 ("grep: add support for PCRE v2", 2017-06-01) for an
> example of how to do this.

Yeah, that's a good point.  Will add the check.

> * The whole business of "#include <sha1.h>" looks very fragile, are
> there really no other packages in e.g. suse that ship a sha1.h? Debian
> has libmd-dev that ships /usr/include/sha1.h that conflicts with this:
> https://packages.debian.org/search?searchon=contents&keywords=sha1.h&mode=exactfilename&suite=unstable&arch=any
> 
> Shipping a sha1.h as opposed to a sha1collisiondetection.h or
> sha1detectcoll.h or whatever seems like a *really* bad decision by
> upstream that should be the subject of at least seeing if they'll take
> a pull request to fix it before you package it or before we include
> something that'll probably need to be fixed / worked around anyway in
> Git.

Yeah, a unique header name would be better indeed.


thanks,

Takashi

Re: [PATCH] hash: Allow building with the external sha1dc library

[Junio C Hamano]

Takashi Iwai <tiwai@suse.de> writes:

> On Fri, 28 Jul 2017 17:58:14 +0200,
> Ævar Arnfjörð Bjarmason wrote:
>>  ...
>> * We now have much of the same header code copy/pasted between
>> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always
>> including the former but making what it's doing conditional on
>> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory
>> glance, but again your commit message doesn't list that among options
>> considered & discarded.
>
> I don't mind either way, there is no perfect solution in this case.
> As you know, many people think the ifdef ugly no matter how.
>
> I leave the decision to maintainer.  Just let me know which option is
> preferred.

Yeah, I also found it somewhat confusing to have these two headers
that look quite similar to each other at the top-level of the tree.

What's the "conditional" part between the two headers?  Is it just
whether the header for underlying library is included?  I wonder if
it's just the matter of adjusting "hash.h" to read like this

    ...
    #if defined(DC_SHA1_EXTERNAL)
   -#include "sha1dc_git_ext.h"
   +#include <sha1dc/sha1.h>
   +#include "sha1dc_git.h"
    #elif  defined(DC_SHA1_SUBMODULE)
    ...

or are there heavier tweaks needed that won't be solved by continuing
along the same line?  As _ext.h variant is included only at this place,
if we can do with minimum tweaks around here without introducing it,
it may be ideal, I would think.

Thanks.

Re: [PATCH] hash: Allow building with the external sha1dc library

[Takashi Iwai]

On Tue, 01 Aug 2017 17:56:00 +0200,
Junio C Hamano wrote:
> 
> Takashi Iwai <tiwai@suse.de> writes:
> 
> > On Fri, 28 Jul 2017 17:58:14 +0200,
> > Ævar Arnfjörð Bjarmason wrote:
> >>  ...
> >> * We now have much of the same header code copy/pasted between
> >> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always
> >> including the former but making what it's doing conditional on
> >> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory
> >> glance, but again your commit message doesn't list that among options
> >> considered & discarded.
> >
> > I don't mind either way, there is no perfect solution in this case.
> > As you know, many people think the ifdef ugly no matter how.
> >
> > I leave the decision to maintainer.  Just let me know which option is
> > preferred.
> 
> Yeah, I also found it somewhat confusing to have these two headers
> that look quite similar to each other at the top-level of the tree.
> 
> What's the "conditional" part between the two headers?  Is it just
> whether the header for underlying library is included?  I wonder if
> it's just the matter of adjusting "hash.h" to read like this
> 
>     ...
>     #if defined(DC_SHA1_EXTERNAL)
>    -#include "sha1dc_git_ext.h"
>    +#include <sha1dc/sha1.h>
>    +#include "sha1dc_git.h"
>     #elif  defined(DC_SHA1_SUBMODULE)
>     ...
> 
> or are there heavier tweaks needed that won't be solved by continuing
> along the same line?  As _ext.h variant is included only at this place,
> if we can do with minimum tweaks around here without introducing it,
> it may be ideal, I would think.

Well, a tricky part is that currently sha1dc_git.h is included from
sha1dc/sha1.h implicitly by SHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_H
definition.  IMO, we should stop this, and use the standard inclusion
instead, i.e. in hash.h,

#if defined(DC_SHA1_EXTERNAL)
#include <sha1dc/sha1.h>
#elif defined(DC_SHA1_SUBMODULE)
#include "sha1collisiondetection/lib/sha1.h"
#else
#include "sha1dc/sha1.h"
#endif
#include "sha1dc_git.h"

In sha1dc_git.h, we'd need another ifdef for DC_SHA1_EXTERNAL to
define the own git_SHA1DCInit(), but it's trivial. 


Takashi

Re: [PATCH] hash: Allow building with the external sha1dc library

[Ævar Arnfjörð Bjarmason]

On Tue, Aug 01 2017, Takashi Iwai jotted:

> On Tue, 01 Aug 2017 17:56:00 +0200,
> Junio C Hamano wrote:
>>
>> Takashi Iwai <tiwai@suse.de> writes:
>>
>> > On Fri, 28 Jul 2017 17:58:14 +0200,
>> > Ævar Arnfjörð Bjarmason wrote:
>> >>  ...
>> >> * We now have much of the same header code copy/pasted between
>> >> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always
>> >> including the former but making what it's doing conditional on
>> >> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory
>> >> glance, but again your commit message doesn't list that among options
>> >> considered & discarded.
>> >
>> > I don't mind either way, there is no perfect solution in this case.
>> > As you know, many people think the ifdef ugly no matter how.
>> >
>> > I leave the decision to maintainer.  Just let me know which option is
>> > preferred.
>>
>> Yeah, I also found it somewhat confusing to have these two headers
>> that look quite similar to each other at the top-level of the tree.
>>
>> What's the "conditional" part between the two headers?  Is it just
>> whether the header for underlying library is included?  I wonder if
>> it's just the matter of adjusting "hash.h" to read like this
>>
>>     ...
>>     #if defined(DC_SHA1_EXTERNAL)
>>    -#include "sha1dc_git_ext.h"
>>    +#include <sha1dc/sha1.h>
>>    +#include "sha1dc_git.h"
>>     #elif  defined(DC_SHA1_SUBMODULE)
>>     ...
>>
>> or are there heavier tweaks needed that won't be solved by continuing
>> along the same line?  As _ext.h variant is included only at this place,
>> if we can do with minimum tweaks around here without introducing it,
>> it may be ideal, I would think.
>
> Well, a tricky part is that currently sha1dc_git.h is included from
> sha1dc/sha1.h implicitly by SHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_H
> definition.  IMO, we should stop this, and use the standard inclusion
> instead, i.e. in hash.h,

It's just like this because when I hacked up that facility I was making
the bare minimum change needed to not make local modifications to the
upstream code. If there's better ways to do this in the presence of
DC_SHA1_EXTERNAL & without that would be most welcome. Thanks.

> #if defined(DC_SHA1_EXTERNAL)
> #include <sha1dc/sha1.h>
> #elif defined(DC_SHA1_SUBMODULE)
> #include "sha1collisiondetection/lib/sha1.h"
> #else
> #include "sha1dc/sha1.h"
> #endif
> #include "sha1dc_git.h"
>
> In sha1dc_git.h, we'd need another ifdef for DC_SHA1_EXTERNAL to
> define the own git_SHA1DCInit(), but it's trivial.
>
>
> Takashi

Re: [PATCH] hash: Allow building with the external sha1dc library

[Takashi Iwai]

On Tue, 01 Aug 2017 21:55:45 +0200,
Ævar Arnfjörð Bjarmason wrote:
> 
> 
> On Tue, Aug 01 2017, Takashi Iwai jotted:
> 
> > On Tue, 01 Aug 2017 17:56:00 +0200,
> > Junio C Hamano wrote:
> >>
> >> Takashi Iwai <tiwai@suse.de> writes:
> >>
> >> > On Fri, 28 Jul 2017 17:58:14 +0200,
> >> > Ævar Arnfjörð Bjarmason wrote:
> >> >>  ...
> >> >> * We now have much of the same header code copy/pasted between
> >> >> sha1dc_git.h and sha1dc_git_ext.h, did you consider just always
> >> >> including the former but making what it's doing conditional on
> >> >> DC_SHA1_EXTERNAL? I don't know if it would be worth it from a cursory
> >> >> glance, but again your commit message doesn't list that among options
> >> >> considered & discarded.
> >> >
> >> > I don't mind either way, there is no perfect solution in this case.
> >> > As you know, many people think the ifdef ugly no matter how.
> >> >
> >> > I leave the decision to maintainer.  Just let me know which option is
> >> > preferred.
> >>
> >> Yeah, I also found it somewhat confusing to have these two headers
> >> that look quite similar to each other at the top-level of the tree.
> >>
> >> What's the "conditional" part between the two headers?  Is it just
> >> whether the header for underlying library is included?  I wonder if
> >> it's just the matter of adjusting "hash.h" to read like this
> >>
> >>     ...
> >>     #if defined(DC_SHA1_EXTERNAL)
> >>    -#include "sha1dc_git_ext.h"
> >>    +#include <sha1dc/sha1.h>
> >>    +#include "sha1dc_git.h"
> >>     #elif  defined(DC_SHA1_SUBMODULE)
> >>     ...
> >>
> >> or are there heavier tweaks needed that won't be solved by continuing
> >> along the same line?  As _ext.h variant is included only at this place,
> >> if we can do with minimum tweaks around here without introducing it,
> >> it may be ideal, I would think.
> >
> > Well, a tricky part is that currently sha1dc_git.h is included from
> > sha1dc/sha1.h implicitly by SHA1DC_CUSTOM_TRAILING_INCLUDE_SHA1_H
> > definition.  IMO, we should stop this, and use the standard inclusion
> > instead, i.e. in hash.h,
> 
> It's just like this because when I hacked up that facility I was making
> the bare minimum change needed to not make local modifications to the
> upstream code. If there's better ways to do this in the presence of
> DC_SHA1_EXTERNAL & without that would be most welcome. Thanks.

Can't it be like the code below?  sha1.h is included only via hash.h,
and sha1dc_git.h doesn't matter for the compile of sha1dc/*.c.
Or am I missing something...?

> > #if defined(DC_SHA1_EXTERNAL)
> > #include <sha1dc/sha1.h>
> > #elif defined(DC_SHA1_SUBMODULE)
> > #include "sha1collisiondetection/lib/sha1.h"
> > #else
> > #include "sha1dc/sha1.h"
> > #endif
> > #include "sha1dc_git.h"


thanks,

Takashi



> >
> > In sha1dc_git.h, we'd need another ifdef for DC_SHA1_EXTERNAL to
> > define the own git_SHA1DCInit(), but it's trivial.
> >
> >
> > Takashi
>