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

> On Wed, Jun 27, 2018 at 09:35:23PM +0200, Johannes Schindelin via GitGitGadget wrote:
> 
> > 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:

> > -	'-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.