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

[Takashi Iwai <tiwai@suse.de>]

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