Hi Peff,
On Thu, 28 Jun 2018, Jeff King wrote:
> 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:
Do you want me to update the commit message?
> > - '-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.
$ x="two spaces"
$ echo $x
two spaces
Maybe we should quote a little bit more religiously.
> 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.
Thanks ;-)
Ciao,
Dscho
