On Tue, 01 Aug 2017 17:56:00 +0200, Junio C Hamano wrote: > > Takashi Iwai <tiwai@xxxxxxx> 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
