Re: [PATCH v3] To make GIT-VERSION-FILE, search for git more widely

"Matthias Andree" [off-list ref] writes:

Am 09.05.2009, 18:55 Uhr, schrieb Junio C Hamano [off-list ref]:

quoted

"Matthias Andree" [off-list ref] writes:

quoted

quoted

Fine then.  Or you could just append "." to the $PATH ;-)

"." in the super user's PATH? Cool stuff, and so innovative.

I didn't mean to suggest PATH=$PATH:. *in the user's environment* ;-).
You do that inside GIT-VERSION-FILE, which is essentially the same thing
as running ./git$X from there.

No, it is not -- the scope of the GIT variable is much narrower than
doing  PATH=$PATH:. in the script.

If you cannot trust the top of your build directory you build git in (that
is why PATH=$PATH:. while running GIT-VERSION-FILE may be scary) and
anticipate a malicious third-party can somehow put random things (like
"test" or "cat", perhaps) there, you are already lost, don't you think?

That is where my "essentially the same" came from.

For any approach taken, we'll have to touch both the shell and the
Makefile, unless we want to manually redo things in the
GIT-VERSION-GEN  script that were already done automatically or
programmatically in  Makefile.

I actually like the rationale you mentioned in the thread (perhaps in the
original proposed commit message as well) that we should ask the freshly
built git to describe the version if available, falling back to whichever
git of random vintage found on the original $PATH.

If it weren't for $X [*1*], my preference would have been (as I said in
the discussion) to run ./git if available locally.

But I think your "deal with details like $X to figure out the name of the
freshly built git binary is in the Makefile, and pass it via GIT variable
to GIT-VERSION-GEN" is a sensible approach.  I do not remember if your
patch gave precedence to an installed git on the original $PATH or a
freshly built one, though---the precedent probably does not matter in
practice, and favoring the one found on $PATH over freshly built one does
have an advantage if we were to support cross compilation (I have a
suspicion that the current setup does not).

Thanks.


[Footnote]

*1* ... and perhaps VPATH as you mentioned earlier, but I do not know if
our current Makefile is set up to allow a layout that separates build
products from the source material, as I've never attempted to build git in
a setting where VPATH is involved.

Quoting Junio C Hamano [off-list ref]:

I actually like the rationale you mentioned in the thread (perhaps in the
original proposed commit message as well) that we should ask the freshly
built git to describe the version if available, falling back to whichever
git of random vintage found on the original $PATH.

If it weren't for $X [*1*], my preference would have been (as I said in
the discussion) to run ./git if available locally.

But I think your "deal with details like $X to figure out the name of the
freshly built git binary is in the Makefile, and pass it via GIT variable
to GIT-VERSION-GEN" is a sensible approach.  I do not remember if your
patch gave precedence to an installed git on the original $PATH or a
freshly built one, though---the precedent probably does not matter in
practice, and favoring the one found on $PATH over freshly built one does
have an advantage if we were to support cross compilation (I have a
suspicion that the current setup does not).

Thanks.

Junio, I think you forgot to take a follow-up action on this thread after sending this message.  The patch favors the git program in the current directory.

Do you want to ask Matthias to resend the patch with an updated log message?



P.S. a happy birthday ;-)

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

Nanako Shiraishi [off-list ref] writes:

Quoting Junio C Hamano [off-list ref]:

quoted

I actually like the rationale you mentioned in the thread (perhaps in the
original proposed commit message as well) that we should ask the freshly
built git to describe the version if available, falling back to whichever
git of random vintage found on the original $PATH.

If it weren't for $X [*1*], my preference would have been (as I said in
the discussion) to run ./git if available locally.

But I think your "deal with details like $X to figure out the name of the
freshly built git binary is in the Makefile, and pass it via GIT variable
to GIT-VERSION-GEN" is a sensible approach.  I do not remember if your
patch gave precedence to an installed git on the original $PATH or a
freshly built one, though---the precedent probably does not matter in
practice, and favoring the one found on $PATH over freshly built one does
have an advantage if we were to support cross compilation (I have a
suspicion that the current setup does not).

Thanks.

Junio, I think you forgot to take a follow-up action on this thread after sending this message.  The patch favors the git program in the current directory.

Indeed, I did, and I think I am Ok with the patch.  Thanks for a
reminder.

I thought there was an "simplicity" issue raised by J6t that was not
addressed, but after re-reading the thread I do not think it applies
(J6t?)

Do you want to ask Matthias to resend the patch with an updated log message?

The proposed commit log message indeed does look somewhat incompatible
with the normal style of our log messages, but I think I can rewrite it.

P.S. a happy birthday ;-)

Heh, depending on where you are, you are a day or so late, but thanks
anyway.

-- 
Nanako Shiraishi
http://ivory.ap.teacup.com/nanako3/

On Dienstag, 2. Juni 2009, Junio C Hamano wrote:

Nanako Shiraishi [off-list ref] writes:

quoted

Junio, I think you forgot to take a follow-up action on this thread after
sending this message.  The patch favors the git program in the current
directory.

Indeed, I did, and I think I am Ok with the patch.  Thanks for a
reminder.

I thought there was an "simplicity" issue raised by J6t that was not
addressed, but after re-reading the thread I do not think it applies
(J6t?)

Sorry, I don't recall anymore what I said; but since the thread petered out, I 
use this patch in the repository where I share Matthias' 'sudo make install' 
problem:

Subject: [PATCH] version-gen: Use just built git if no other git is in PATH

Signed-off-by: Johannes Sixt <redacted>

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 39cde78..4779313 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN

@@ -3,6 +3,9 @@
 GVF=GIT-VERSION-FILE
 DEF_VER=v1.6.3.GIT
 
+# use git that was just compiled if there is no git elsewhere in PATH
+PATH=$PATH:.
+
 LF='
 '
 

-- Hannes

Am 02.06.2009, 20:35 Uhr, schrieb Johannes Sixt [off-list ref]:

Hi Hannes,

that's not what I proposed at the time; I'll look how I ordered  
preferences at the time and will re-submit soon. AFAIR (I'll check the  
archives), Junio had preferred checking $(prefix) before . in order to  
support cross-builds.

Best regards

-- 
Matthias Andree

Situation: sudo make install can rebuilds the whole package even if
you've just built it before. For instance:

make configure
./configure    # defaults to --prefix=/usr/local
make all doc
sudo make install install-doc install-html # REBUILDS HAPPEN HERE

This causes the "sudo make install" to rebuild everything because it believes
the version had changed.  sudo strips $PATH for security reasons.

The underlying problem flow is:

1 - Makefile has "include GIT-VERSION-FILE", thus gmake builds
    GIT-VERSION-FILE early.
2 - GIT-VERSION-FILE depends on a .PHONY target (.FORCE-GIT-VERSION-FILE)
3 - Thus, GNU make *always* executes GIT-VERSION-GEN
4 - GIT-VERSION-GEN now, under the stripped $PATH, cannot find "git" and
    sees a different version number.
5 - GIT-VERSION-GEN notes the difference in versions and regenerates
    GIT-VERSION-FILE, with up-to-date timestamp.
6 - GNU make rebuilds everything because GIT-VERSION-FILE is new.

The patch makes GIT-VERSION-GEN look for git in $(prefix)/bin, then for
the newly built git$X executable, before falling back to plain "git" and
thus to the default version in GIT-VERSION-GEN. This increases chances
that we get the same version with the stripped $PATH and get away
without rebuild.

Junio C. Hamano suggested that we look into $(prefix)/bin before the
current work directory in order to aid cross-compiling.

Signed-off-by: Matthias Andree <redacted>
---
 GIT-VERSION-GEN         |    9 ++++-----
 Makefile                |    6 +++++-
 git-gui/GIT-VERSION-GEN |   18 ++++++++++--------
 git-gui/Makefile        |    6 +++++-
 4 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 39cde78..2334cc1 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN

@@ -2,6 +2,7 @@
 
 GVF=GIT-VERSION-FILE
 DEF_VER=v1.6.3.GIT
+type >/dev/null "$GIT" || GIT=git
 
 LF='
 '

@@ -12,12 +13,12 @@ if test -f version
 then
 	VN=$(cat version) || VN="$DEF_VER"
 elif test -d .git -o -f .git &&
-	VN=$(git describe --abbrev=4 HEAD 2>/dev/null) &&
+	VN=$($GIT describe --abbrev=4 HEAD 2>/dev/null) &&
 	case "$VN" in
 	*$LF*) (exit 1) ;;
 	v[0-9]*)
-		git update-index -q --refresh
-		test -z "$(git diff-index --name-only HEAD --)" ||
+		$GIT update-index -q --refresh
+		test -z "$($GIT diff-index --name-only HEAD --)" ||
 		VN="$VN-dirty" ;;
 	esac
 then

@@ -38,5 +39,3 @@ test "$VN" = "$VC" || {
 	echo >&2 "GIT_VERSION = $VN"
 	echo "GIT_VERSION = $VN" >$GVF
 }
-
-

diff --git a/Makefile b/Makefile
index 06c39e4..7adcf40 100644
--- a/Makefile
+++ b/Makefile

@@ -184,7 +184,11 @@ all::
 # programs as a tar, where bin/ and libexec/ might be on different file systems.
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
-	@$(SHELL_PATH) ./GIT-VERSION-GEN
+	@{ GIT=$(prefix)/bin/git$X ; test -x "$$GIT" ; } \
+	|| { GIT=./git$X ; test -x "$$GIT" ; } \
+	|| GIT=git ; \
+	export GIT ; \
+	$(SHELL_PATH) ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
 
 uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')

diff --git a/git-gui/GIT-VERSION-GEN b/git-gui/GIT-VERSION-GEN
index b3f937e..729e93f 100755
--- a/git-gui/GIT-VERSION-GEN
+++ b/git-gui/GIT-VERSION-GEN

@@ -3,6 +3,8 @@
 GVF=GIT-VERSION-FILE
 DEF_VER=0.12.GITGUI
 
+type >/dev/null "$GIT" || GIT=git
+
 LF='
 '
 

@@ -10,10 +12,10 @@ tree_search ()
 {
 	head=$1
 	tree=$2
-	for p in $(git rev-list --parents --max-count=1 $head 2>/dev/null)
+	for p in $($GIT rev-list --parents --max-count=1 $head 2>/dev/null)
 	do
-		test $tree = $(git rev-parse $p^{tree} 2>/dev/null) &&
-		vn=$(git describe --abbrev=4 $p 2>/dev/null) &&
+		test $tree = $($GIT rev-parse $p^{tree} 2>/dev/null) &&
+		vn=$($GIT describe --abbrev=4 $p 2>/dev/null) &&
 		case "$vn" in
 		gitgui-[0-9]*) echo $vn; break;;
 		esac

@@ -38,10 +40,10 @@ if test -f version &&
    VN=$(cat version)
 then
 	: happy
-elif prefix="$(git rev-parse --show-prefix 2>/dev/null)"
+elif prefix="$($GIT rev-parse --show-prefix 2>/dev/null)"
    test -n "$prefix" &&
-   head=$(git rev-list --max-count=1 HEAD -- . 2>/dev/null) &&
-   tree=$(git rev-parse --verify "HEAD:$prefix" 2>/dev/null) &&
+   head=$($GIT rev-list --max-count=1 HEAD -- . 2>/dev/null) &&
+   tree=$($GIT rev-parse --verify "HEAD:$prefix" 2>/dev/null) &&
    VN=$(tree_search $head $tree)
    case "$VN" in
    gitgui-[0-9]*) : happy ;;

@@ -49,7 +51,7 @@ elif prefix="$(git rev-parse --show-prefix 2>/dev/null)"
    esac
 then
 	VN=$(echo "$VN" | sed -e 's/^gitgui-//;s/-/./g');
-elif VN=$(git describe --abbrev=4 HEAD 2>/dev/null) &&
+elif VN=$($GIT describe --abbrev=4 HEAD 2>/dev/null) &&
    case "$VN" in
    gitgui-[0-9]*) : happy ;;
    *) (exit 1) ;;

@@ -60,7 +62,7 @@ else
 	VN="$DEF_VER"
 fi
 
-dirty=$(sh -c 'git diff-index --name-only HEAD' 2>/dev/null) || dirty=
+dirty=$(sh -c '$GIT diff-index --name-only HEAD' 2>/dev/null) || dirty=
 case "$dirty" in
 '')
 	;;

diff --git a/git-gui/Makefile b/git-gui/Makefile
index b3580e9..bbdb4d8 100644
--- a/git-gui/Makefile
+++ b/git-gui/Makefile

@@ -8,7 +8,11 @@ all::
 #
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
-	@$(SHELL_PATH) ./GIT-VERSION-GEN
+	@{ GIT=$(prefix)/bin/git$X ; test -x "$$GIT" ; } \
+	|| { GIT=./git$X ; test -x "$$GIT" ; } \
+	|| GIT=git ; \
+	export GIT ; \
+	$(SHELL_PATH) ./GIT-VERSION-GEN
 -include GIT-VERSION-FILE
 
 uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not')

-- 
1.6.3.1.267.gd260a