Re: [PATCH 1/1] Makefile: fix the "built from commit" code

From: Jeff King <peff@peff.net>
Date: Jun 28, 2018, 13:23 UTC
Message-ID: <20180628132314.GA14026@sigill.intra.peff.net>
In-Reply-To: <e0e41d0b88b4104737b9ee80710c1bec91c9d759.1530190395.git.gitgitgadget@gmail.com>

On Wed, Jun 27, 2018 at 09:35:23PM +0200, Johannes Schindelin via GitGitGadget wrote:

Show 10 quoted lines

> To prevent erroneous commits from being reported (e.g. when unpacking
> Git's source code from a .tar.gz file into a subdirectory of a different
> Git project, as e.g. git_osx_installer does), we painstakingly set
> GIT_CEILING_DIRECTORIES when trying to determine the current commit.
> 
> Except that we got the quoting wrong, and that variable therefore does
> not have the desired effect.
> 
> Let's fix that quoting, and while at it, also suppress the unhelpful
> message

I had to stare at the code for a bit to figure out what was wrong:

Show 5 quoted lines

> -	'-DGIT_BUILT_FROM_COMMIT="$(shell GIT_CEILING_DIRECTORIES=\"$(CURDIR)/..\" \
> -		git rev-parse -q --verify HEAD || :)"'
> +	'-DGIT_BUILT_FROM_COMMIT="$(shell \
> +		GIT_CEILING_DIRECTORIES="$(CURDIR)/.." \
> +		git rev-parse -q --verify HEAD 2>/dev/null)"'

The issue is that the $(shell) is resolved before the output is stuffed into the command-line with -DGIT_BUILT_FROM_COMMIT, and therefore is _not_ inside quotes. And thus backslashing the quotes is wrong, as the quote gets literally inserted into the CEILING_DIRECTORIES variable.

I thought at first we could not need the quotes in the post-image either, because shell variable assignments do not do word-splitting. I.e.:

  FOO='with spaces'
  BAR=$FOO sh -c 'echo $BAR'

works just fine. But $(CURDIR) here is not a shell variable, but rather a Makefile variable, so it's expanded before we hit the shell. So we need the quotes. And unfortunately it also breaks if $(CURDIR) contains exotic metacharacters. If we cared we could use single quotes and $(CURDIR_SQ), but I suspect it would be far from the first thing to break in such a case.

Which is a long-winded way of saying the patch looks correct to me.

-Peff