Re: [PATCH 1/4] Makefile: don't error out under DC_SHA1_EXTERNAL if DC_SHA1_SUBMODULE=auto
- From
Jeff King <peff@peff.net>
- Date
- Dec 5, 2017, 06:53 UTC
- Message-ID
- <20171205065329.GA4788@sigill.intra.peff.net>
- In-Reply-To
- <20171128213214.12477-2-avarab@gmail.com>
On Tue, Nov 28, 2017 at 09:32:11PM +0000, Ævar Arnfjörð Bjarmason wrote:
Show 12 quoted lines
> 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:
Show 12 quoted lines
> 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