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

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

[Johannes Schindelin via GitGitGadget]

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

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

[Johannes Schindelin via GitGitGadget]

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(-)

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

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

[Jeff King]

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

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

[Johannes Schindelin]

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

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

[Junio C Hamano]

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

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.

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

[Jeff King]

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 ;-)
> 
> 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.

-Peff

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

[Johannes Schindelin]

Hi Peff,

On Thu, 28 Jun 2018, Jeff King wrote:

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

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

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

[Jeff King]

On Thu, Jun 28, 2018 at 06:23:56PM +0200, Johannes Schindelin wrote:

> 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

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

[Johannes Schindelin]

Team,

[Cc:ing Tim]

On Thu, 28 Jun 2018, Johannes Schindelin via GitGitGadget wrote:

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

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

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

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

[brian m. carlson]

[-- Attachment #1: Type: text/plain, Size: 601 bytes --]

On Thu, Jun 28, 2018 at 12:53:15PM +0000, Johannes Schindelin via GitGitGadget wrote:
> 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 867 bytes --]

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

[Johannes Schindelin via GitGitGadget]

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

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

[Johannes Schindelin via GitGitGadget]

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(-)

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