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

[Jeff King <peff@peff.net>]

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.

-Peff