On Tue, Nov 28, 2017 at 09:32:11PM +0000, Ævar Arnfjörð Bjarmason wrote: > Fix a logic error in the initial introduction of DC_SHA1_EXTERNAL. If > git.git has a sha1collisiondetection submodule checked out the logic > to set DC_SHA1_SUBMODULE=auto would interact badly with the check for > whether DC_SHA1_SUBMODULE was set. > > It would error out, meaning that there's no way to build git with > DC_SHA1_EXTERNAL=YesPlease without deinit-ing the submodule. > > Instead, adjust the logic to only fire if the variable is to something > else than "auto" which would mean it's a mistake on the part of > whoever's building git, not just the Makefile tripping over its own > logic. This all makes sense, and I agree your patch is an improvement. One minor whitespace nit: > diff --git a/Makefile b/Makefile > index e53750ca01..8fe8278126 100644 > --- a/Makefile > +++ b/Makefile > @@ -1497,7 +1497,9 @@ else > LIB_OBJS += sha1dc_git.o > ifdef DC_SHA1_EXTERNAL > ifdef DC_SHA1_SUBMODULE > +ifneq ($(DC_SHA1_SUBMODULE),auto) > $(error Only set DC_SHA1_EXTERNAL or DC_SHA1_SUBMODULE, not both) > +endif > endif The indentation here is funky. Unfortunately I think $(error) can't be tab-indented, so it has to either stay at the left-most side, or we have to use spaces. But the ifneq/endif pair can be indented. Ordinarily I'd say it's fine to keep it at the outermost (because of the weird error indent), but note that we're _inside_ an already-indented ifdef/endif pair. We should either stay inside there, or we should put the outer one to the left for consistency. -Peff
