patchhash: Allow building with the external sha1dc library

> 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

> 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
>

> 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
>>

> 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.

> 
> 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.

> 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.

> 
> Æ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.

> 
> 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).

> 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.

> * 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.

> 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.

> 
> 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.

> 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

> 
> 
> 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
>