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
> <ava...@gmail.com> wrote:
> > On Tue, Jul 25, 2017 at 7:57 AM, Takashi Iwai <ti...@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
