threads / patch / 48796

patchMakefile: fix the "built from commit" code

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

## tl;dr

12 messages between Jun 28, 2018 and Jun 29, 2018. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Johannes Schindelin via GitGitGadget· Jun 28, 2018, 12:53 UTC · lore

In ed32b788c06 (version --build-options: report commit, too, if possible, 2017-12-15), we introduced code to let `git version --build-options` report the current commit from which the binaries were built, if any.

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

fatal: not a git repository (or any of the parent directories): .git

that gets printed to stderr if no current commit could be determined, and might scare the occasional developer who simply tries to build Git from scratch.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Thanks for taking the time to contribute to Git! Please be advised that the Git community does not use github.com for their contributions. Instead, we use a mailing list (git@vger.kernel.org) for code submissions, code reviews, and bug reports. Nevertheless, you can use submitGit to conveniently send your Pull Requests commits to our mailing list.

Please read the "guidelines for contributing" linked above!
Johannes Schindelin (1):
  Makefile: fix the "built from commit" code
 Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
base-commit: ed843436dd4924c10669820cc73daf50f0b4dabd
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-7/dscho/fix-build-options-commit-info-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-7/dscho/fix-build-options-commit-info-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/7
-- 
gitgitgadget
Johannes Schindelin via GitGitGadget· Jun 27, 2018, 19:35 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In ed32b788c06 (version --build-options: report commit, too, if possible, 2017-12-15), we introduced code to let `git version --build-options` report the current commit from which the binaries were built, if any.

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

fatal: not a git repository (or any of the parent directories): .git

that gets printed to stderr if no current commit could be determined, and might scare the occasional developer who simply tries to build Git from scratch.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to Makefile +3 −2
diff --git a/Makefile b/Makefile
index 0cb6590f2..c70f823a0 100644
--- a/Makefile
+++ b/Makefile
@@ -2021,8 +2021,9 @@ version.sp version.s version.o: GIT-VERSION-FILE GIT-USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
 	'-DGIT_VERSION="$(GIT_VERSION)"' \
 	'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)' \
-	'-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)"'
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \

-- 
gitgitgadget
Jeff King· Jun 28, 2018, 13:23 UTC · re: Johannes Schindelin via GitGitGadget · lore

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:
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
Johannes Schindelin· Jun 28, 2018, 16:23 UTC · re: Jeff King · lore

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

Hi Peff,
On Thu, 28 Jun 2018, Jeff King wrote:
Show 14 quoted lines
> 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?
Show 13 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.
Show 6 quoted lines
> 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.
Show 8 quoted lines
> 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

Junio C Hamano· Jun 28, 2018, 17:27 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 13 quoted lines
>> 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.
Both of you are wrong ;-)

Of course, the lack of dq around echo's argument makes shell split two and spaces into two args and feed them separately to echo, and causes echo to show them with a single SP in between. Peff's exampel should have been

	BAR=$FOO sh -c 'echo "$BAR"'

But that does not have much to do with the primary point Peff was talking about, which is that in this sequence:

	$ x="two  spaces"
	$ y="$x"
	$ z=$x
	$ echo "x=<$x>" "y=<$y>" "z=<$z>"

assignment to y and z behave identically, i.e. dq around "$x" when assigning to y is not needed.

Jeff King· Jun 28, 2018, 17:47 UTC · re: Junio C Hamano · lore

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

On Thu, Jun 28, 2018 at 10:27:32AM -0700, Junio C Hamano wrote:
Show 24 quoted lines
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> >> 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.
> 
> Both of you are wrong ;-)
> 
> Of course, the lack of dq around echo's argument makes shell split
> two and spaces into two args and feed them separately to echo, and
> causes echo to show them with a single SP in between.  Peff's
> exampel should have been
> 
> 	BAR=$FOO sh -c 'echo "$BAR"'

Yes, that's a better example. I was primarily trying to show that the outer shell did not barf with "spaces: command not found".

Show 10 quoted lines
> But that does not have much to do with the primary point Peff was
> talking about, which is that in this sequence:
> 
> 	$ x="two  spaces"
> 	$ y="$x"
> 	$ z=$x
> 	$ echo "x=<$x>" "y=<$y>" "z=<$z>"
> 
> assignment to y and z behave identically, i.e. dq around "$x" when
> assigning to y is not needed.

I actually had to test it to convince myself that one-shot assignments behaved the same way, but they do.

-Peff
Johannes Schindelin· Jun 29, 2018, 11:29 UTC · re: Jeff King · lore

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

Hi Peff,
On Thu, 28 Jun 2018, Jeff King wrote:
Show 19 quoted lines
> On Thu, Jun 28, 2018 at 10:27:32AM -0700, Junio C Hamano wrote:
> 
> > Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> > 
> > >> 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.
> > 
> > Both of you are wrong ;-)
Technically, you did not contradict anything I said.
Show 23 quoted lines
> > Of course, the lack of dq around echo's argument makes shell split
> > two and spaces into two args and feed them separately to echo, and
> > causes echo to show them with a single SP in between.  Peff's
> > exampel should have been
> > 
> > 	BAR=$FOO sh -c 'echo "$BAR"'
> 
> Yes, that's a better example. I was primarily trying to show that the
> outer shell did not barf with "spaces: command not found".
> 
> > But that does not have much to do with the primary point Peff was
> > talking about, which is that in this sequence:
> > 
> > 	$ x="two  spaces"
> > 	$ y="$x"
> > 	$ z=$x
> > 	$ echo "x=<$x>" "y=<$y>" "z=<$z>"
> > 
> > assignment to y and z behave identically, i.e. dq around "$x" when
> > assigning to y is not needed.
> 
> I actually had to test it to convince myself that one-shot assignments
> behaved the same way, but they do.

The mere fact that you had to test it out to convince yourself suggests to me that my suspicion "Maybe we should quote a little bit more religiously" was 100% spot on.

After all, almost *none* of the reviews on this mailing list involve anything like "testing it out". It happens in the mailing program, and it stays in the mailing program.

Ciao, Dscho

Jeff King· Jun 28, 2018, 17:49 UTC · re: Johannes Schindelin · lore

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

On Thu, Jun 28, 2018 at 06:23:56PM +0200, Johannes Schindelin wrote:
Show 18 quoted lines
> 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?

I'm OK either way. Probably not worth a re-roll unless you want to be completionist about commit messages (personally I do not mind occasionally jumping to the list archive to get historical context and reviews).

-Peff
Johannes Schindelin· Jun 28, 2018, 13:18 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

Team,
[Cc:ing Tim]
On Thu, 28 Jun 2018, Johannes Schindelin via GitGitGadget wrote:
Show 23 quoted lines
> In ed32b788c06 (version --build-options: report commit, too, if
> possible, 2017-12-15), we introduced code to let `git version
> --build-options` report the current commit from which the binaries were
> built, if any.
> 
> 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
> 
> fatal: not a git repository (or any of the parent directories): .git
> 
> that gets printed to stderr if no current commit could be determined,
> and might scare the occasional developer who simply tries to build Git
> from scratch.
> 
> Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>

Sorry for the repeated commit message. I meant to edit the cover letter before sending. It should have read something like this:

-- snip -- Fix "built from commit" logic

When I tried recently to build macOS installers via Tim Harper's wonderful project at https://github.com/timcharper/git_osx_installer, it worked (with a couple of quirks), but it reported to be built from a commit that I first could not place.

Turns out that the git_osx_installer project insists on building Git from a .tar.gz file (even if I have the source code right here, in a perfectly fine worktree). And due to a bug in the logic I introduced, it did not stop looking for a Git repository where it should have stopped. The end effect is that `git version --build-options` reports being built from git_osx_installer's HEAD.

This commit fixes that, and also suppresses the error when no repository could be found. -- snap --

Show 7 quoted lines
> Thanks for taking the time to contribute to Git! Please be advised that the
> Git community does not use github.com for their contributions. Instead, we use
> a mailing list (git@vger.kernel.org) for code submissions, code reviews, and
> bug reports. Nevertheless, you can use submitGit to conveniently send your Pull
> Requests commits to our mailing list.
> 
> Please read the "guidelines for contributing" linked above!
Again, sorry for failing to edit this before sending.
Show 10 quoted lines
> 
> Johannes Schindelin (1):
>   Makefile: fix the "built from commit" code
> 
>  Makefile | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> 
> base-commit: ed843436dd4924c10669820cc73daf50f0b4dabd
> Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-7/dscho/fix-build-options-commit-info-v1

This should be https://github.com/gitgitgadget/git/releases/tag/pr-7%2Fdscho%2Ffix-build-options-commit-info-v1 instead. Again: sorry!

> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-7/dscho/fix-build-options-commit-info-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/7
These are correct.

Ciao, Dscho

> -- 
> gitgitgadget
>
brian m. carlson· Jun 28, 2018, 23:14 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

On Thu, Jun 28, 2018 at 12:53:15PM +0000, Johannes Schindelin via GitGitGadget wrote:
Show 8 quoted lines
> Let's fix that quoting, and while at it, also suppress the unhelpful
> message
> 
> fatal: not a git repository (or any of the parent directories): .git
> 
> that gets printed to stderr if no current commit could be determined,
> and might scare the occasional developer who simply tries to build Git
> from scratch.
I saw that building Git 2.18.0 for $DAYJOB.  Thanks for fixing it.
The series looked good to me, too.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Johannes Schindelin via GitGitGadget· Jun 29, 2018, 12:16 UTC · re: Johannes Schindelin via GitGitGadget · lore

[PATCH v2 0/1] Fix "built from commit" logic

When I tried recently to build macOS installers via Tim Harper's wonderful project at https://github.com/timcharper/git_osx_installer, it worked (with a couple of quirks), but it reported to be built from a commit that I first could not place.

Turns out that the git_osx_installer project insists on building Git from a .tar.gz file (even if I have the source code right here, in a perfectly fine worktree). And due to a bug in the logic I introduced, it did not stop looking for a Git repository where it should have stopped. The end effect is that `git version --build-options` reports being built from git_osx_installer's HEAD.

This commit fixes that, and also suppresses the error when no repository could be found.
Changes since v1:
- the commit message now sports an explanatory paragraph, copy-edited from Peff's reply.
Johannes Schindelin (1):
  Makefile: fix the "built from commit" code
 Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
base-commit: e3331758f12da22f4103eec7efe1b5304a9be5e9
Published-As: https://github.com/gitgitgadget/git/releases/tags/pr-7%2Fdscho%2Ffix-build-options-commit-info-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-7/dscho/fix-build-options-commit-info-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/7
Range-diff vs v1:
 1:  e0e41d0b8 ! 1:  aca087479 Makefile: fix the "built from commit" code
     @@ -15,6 +15,11 @@
          Except that we got the quoting wrong, and that variable therefore does
          not have the desired effect.
      
     +    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.
     +
          Let's fix that quoting, and while at it, also suppress the unhelpful
          message
      
-- 
gitgitgadget
Johannes Schindelin via GitGitGadget· Jun 27, 2018, 19:35 UTC · re: Johannes Schindelin via GitGitGadget · lore

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

From: Johannes Schindelin <johannes.schindelin@gmx.de>

In ed32b788c06 (version --build-options: report commit, too, if possible, 2017-12-15), we introduced code to let `git version --build-options` report the current commit from which the binaries were built, if any.

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.

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.

Let's fix that quoting, and while at it, also suppress the unhelpful message

fatal: not a git repository (or any of the parent directories): .git

that gets printed to stderr if no current commit could be determined, and might scare the occasional developer who simply tries to build Git from scratch.

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
---
 Makefile | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to Makefile +3 −2
diff --git a/Makefile b/Makefile
index 0cb6590f2..c70f823a0 100644
--- a/Makefile
+++ b/Makefile
@@ -2021,8 +2021,9 @@ version.sp version.s version.o: GIT-VERSION-FILE GIT-USER-AGENT
 version.sp version.s version.o: EXTRA_CPPFLAGS = \
 	'-DGIT_VERSION="$(GIT_VERSION)"' \
 	'-DGIT_USER_AGENT=$(GIT_USER_AGENT_CQ_SQ)' \
-	'-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)"'
 
 $(BUILT_INS): git$X
 	$(QUIET_BUILT_IN)$(RM) $@ && \

-- 
gitgitgadget

← back to recent threads