From: Johannes Schindelin via GitGitGadget <hidden> Date: 2018-06-28 12:53:22
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 <redacted>
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
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2018-06-28 12:53:23
From: Johannes Schindelin <redacted>
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 <redacted>
---
Makefile | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
From: Johannes Schindelin <hidden> Date: 2018-06-28 13:18:30
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 <redacted>
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.
From: Jeff King <hidden> Date: 2018-06-28 13:23:18
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:
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
From: Johannes Schindelin <hidden> Date: 2018-06-28 16:24:26
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:
quoted
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:
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.
From: Jeff King <hidden> Date: 2018-06-28 17:49:37
On Thu, Jun 28, 2018 at 06:23:56PM +0200, Johannes Schindelin wrote:
On Thu, 28 Jun 2018, Jeff King wrote:
quoted
On Wed, Jun 27, 2018 at 09:35:23PM +0200, Johannes Schindelin via GitGitGadget wrote:
quoted
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
From: brian m. carlson <hidden> Date: 2018-06-28 23:14:17
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
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2018-06-29 12:16:17
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
From: Johannes Schindelin via GitGitGadget <hidden> Date: 2018-06-29 12:16:20
From: Johannes Schindelin <redacted>
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 <redacted>
---
Makefile | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)