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

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

[Matthias Andree]

Situation: sudo make install 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 the current built git$X executable,
and in $(prefix)/bin/git, before falling back to plain "git" and thus to the
default version in GIT-VERSION-GEN.

Signed-off-by: Matthias Andree <matthias.andree@gmx.de>
---
 GIT-VERSION-GEN |    9 ++++-----
 Makefile        |    6 +++++-
 2 files changed, 9 insertions(+), 6 deletions(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 39cde78..d0dfef3 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -2,6 +2,7 @@
 
 GVF=GIT-VERSION-FILE
 DEF_VER=v1.6.3.GIT
+test -x "$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 6e21643..d6be483 100644
--- a/Makefile
+++ b/Makefile
@@ -177,7 +177,11 @@ all::
 # away (some NTFS drivers seem to zero the contents in that scenario).
 
 GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
-	@$(SHELL_PATH) ./GIT-VERSION-GEN
+	@{ GIT=./git$X ; test -x "$$GIT" ; } \
+	    || { GIT=$(prefix)/bin/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.geabc3

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

[Michael J Gruber]

Matthias Andree venit, vidit, dixit 07.05.2009 11:22:
> Situation: sudo make install 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 the current built git$X executable,
> and in $(prefix)/bin/git, before falling back to plain "git" and thus to the
> default version in GIT-VERSION-GEN.

Thanks for the detailed analysis, now I g[oi]t it!
According to the analysis, the problem would also appear with a standard
make run (without configure) as long as git is not in the sudoer's $PATH
($prefix isn't, no distro git in /usr).

> Signed-off-by: Matthias Andree <matthias.andree@gmx.de>
> ---
>  GIT-VERSION-GEN |    9 ++++-----
>  Makefile        |    6 +++++-
>  2 files changed, 9 insertions(+), 6 deletions(-)
> 
> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> index 39cde78..d0dfef3 100755
> --- a/GIT-VERSION-GEN
> +++ b/GIT-VERSION-GEN
> @@ -2,6 +2,7 @@
>  
>  GVF=GIT-VERSION-FILE
>  DEF_VER=v1.6.3.GIT
> +test -x "$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 6e21643..d6be483 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -177,7 +177,11 @@ all::
>  # away (some NTFS drivers seem to zero the contents in that scenario).
>  
>  GIT-VERSION-FILE: .FORCE-GIT-VERSION-FILE
> -	@$(SHELL_PATH) ./GIT-VERSION-GEN
> +	@{ GIT=./git$X ; test -x "$$GIT" ; } \
> +	    || { GIT=$(prefix)/bin/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')

Looks good to me. So, untested but reviewed by me.

Michael

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

[Matthias Andree]

Am 07.05.2009, 13:49 Uhr, schrieb Michael J Gruber  
<git@drmicha.warpmail.net>:

> Matthias Andree venit, vidit, dixit 07.05.2009 11:22:
> ...
>> 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 the current built git$X  
>> executable,
>> and in $(prefix)/bin/git, before falling back to plain "git" and thus  
>> to the
>> default version in GIT-VERSION-GEN.
>
> Thanks for the detailed analysis, now I g[oi]t it!
> According to the analysis, the problem would also appear with a standard
> make run (without configure) as long as git is not in the sudoer's $PATH
> ($prefix isn't, no distro git in /usr).

I am not sure how useful /this/ example is -- prefix=$HOME is default, no  
sudo required. make prefix=/opt/git might be a point though.

> [commit/diff]
> Looks good to me. So, untested but reviewed by me.

Thanks.

-- 
Matthias Andree

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

[Michael J Gruber]

Matthias Andree venit, vidit, dixit 07.05.2009 14:04:
> Am 07.05.2009, 13:49 Uhr, schrieb Michael J Gruber  
> <git@drmicha.warpmail.net>:
> 
>> Matthias Andree venit, vidit, dixit 07.05.2009 11:22:
>> ...
>>> 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 the current built git$X  
>>> executable,
>>> and in $(prefix)/bin/git, before falling back to plain "git" and thus  
>>> to the
>>> default version in GIT-VERSION-GEN.
>>
>> Thanks for the detailed analysis, now I g[oi]t it!
>> According to the analysis, the problem would also appear with a standard
>> make run (without configure) as long as git is not in the sudoer's $PATH
>> ($prefix isn't, no distro git in /usr).
> 
> I am not sure how useful /this/ example is -- prefix=$HOME is default, no  
> sudo required. make prefix=/opt/git might be a point though.

That's what I meant by my admittedly fuzzy "$prefix isn't".

I just wanted to point out that your PATCH fixes an easy which also
"ordinary" make usage (with prefix and sudo) has, because
autoconf/configure is considered a 2nd class citizen.

>> [commit/diff]
>> Looks good to me. So, untested but reviewed by me.
> 
> Thanks.
> 

Cheers,
Michael

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

[Matthias Andree]

Am 07.05.2009, 14:09 Uhr, schrieb Michael J Gruber  
<git@drmicha.warpmail.net>:

> I just wanted to point out that your PATCH fixes an easy which also
> "ordinary" make usage (with prefix and sudo) has, because
> autoconf/configure is considered a 2nd class citizen.

Well, configure is documented, so I don't care about the fare it pays for  
travelling the git.git repo. :-) I could probably also drop  
"prefix=/usr/local" in config.mak or thereabouts and no longer care about  
autoconf (that's the only I use it for).

-- 
Matthias Andree

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

[Junio C Hamano]

Matthias Andree <matthias.andree@gmx.de> writes:

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

Interesting.  I wonder if you need the change to the Makefile.

As long as GIT-VERSION-GEN notices that you have a freshly built git
available (test -x) and uses it, falling back to whatever on the PATH, it
would not have to touch GIT-VERSION-FILE, no?

IOW, instead of this:

> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
> index 39cde78..d0dfef3 100755
> --- a/GIT-VERSION-GEN
> +++ b/GIT-VERSION-GEN
> @@ -2,6 +2,7 @@
>  
>  GVF=GIT-VERSION-FILE
>  DEF_VER=v1.6.3.GIT
> +test -x "$GIT" || GIT=git

wouldn't it make more sense to do

	if test -x "git"
        then
        	GIT=./git
	elif test -x "git.exe"
        then
        	GIT=./git.exe
	else
        	GIT=git
	fi

and use the rest of the patch to GIT-VERSION-GEN, without touching
Makefile at all?

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

[Matthias Andree]

Am 08.05.2009, 02:05 Uhr, schrieb Junio C Hamano <gitster@pobox.com>:

> Matthias Andree <matthias.andree@gmx.de> writes:
>
>> 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.
>
> Interesting.  I wonder if you need the change to the Makefile.
>
> As long as GIT-VERSION-GEN notices that you have a freshly built git
> available (test -x) and uses it, falling back to whatever on the PATH, it
> would not have to touch GIT-VERSION-FILE, no?

Hi Junio,

Makefile (+ GNU make) is the actual culprit and introduces this cycle, why  
do we want to leave Makefile - of all things - alone?

Makefile also has all the info: (1) locations, for VPATH builds, (2)  
$(prefix), (3) $X (extension), so let's have it communicate that (through  
the $GIT variable). Let's not introduce second-guessing into the script.  
It would be error prone and manually duplicates efforts that either are  
already there or are automatic in Makefile. G-V-GEN is run under  
Makefile's control, so let's steer it into the right direction.

The G-V-GEN test -x is only there to fall back to a path search if the  
variable is unset when running things directly, rather than through make.

HTH
Matthias

> IOW, instead of this:
>
>> diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
>> index 39cde78..d0dfef3 100755
>> --- a/GIT-VERSION-GEN
>> +++ b/GIT-VERSION-GEN
>> @@ -2,6 +2,7 @@
>>
>>  GVF=GIT-VERSION-FILE
>>  DEF_VER=v1.6.3.GIT
>> +test -x "$GIT" || GIT=git
>
> wouldn't it make more sense to do
>
> 	if test -x "git"
>         then
>         	GIT=./git
> 	elif test -x "git.exe"
>         then
>         	GIT=./git.exe
> 	else
>         	GIT=git
> 	fi
>
> and use the rest of the patch to GIT-VERSION-GEN, without touching
> Makefile at all?



-- 
Matthias Andree

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

[Junio C Hamano]

"Matthias Andree" <matthias.andree@gmx.de> writes:

>> Interesting.  I wonder if you need the change to the Makefile.
>>
>> As long as GIT-VERSION-GEN notices that you have a freshly built git
>> available (test -x) and uses it, falling back to whatever on the PATH, it
>> would not have to touch GIT-VERSION-FILE, no?
>
> Hi Junio,
>
> ... Let's not introduce second-guessing into
> the script....

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

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

[Johannes Sixt]

Matthias Andree schrieb:
> Am 08.05.2009, 02:05 Uhr, schrieb Junio C Hamano <gitster@pobox.com>:
>> Interesting.  I wonder if you need the change to the Makefile.
>>
>> As long as GIT-VERSION-GEN notices that you have a freshly built git
>> available (test -x) and uses it, falling back to whatever on the PATH, it
>> would not have to touch GIT-VERSION-FILE, no?
> 
> Hi Junio,
> 
> Makefile (+ GNU make) is the actual culprit and introduces this cycle,
> why do we want to leave Makefile - of all things - alone?

Because it is not necessary to change it?

With Junio's proposed change the following:

   $ make && PATH=/bin:/usr/bin make

builds only once[*], whereas previously it built twice.

[*] It still builds twice in git-gui, but I think that your original patch
wouldn't fix that, either.

-- Hannes

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

[Matthias Andree]

Am 08.05.2009, 10:41 Uhr, schrieb Junio C Hamano <gitster@pobox.com>:

> "Matthias Andree" <matthias.andree@gmx.de> writes:
>
>>> Interesting.  I wonder if you need the change to the Makefile.
>>>
>>> As long as GIT-VERSION-GEN notices that you have a freshly built git
>>> available (test -x) and uses it, falling back to whatever on the PATH,  
>>> it
>>> would not have to touch GIT-VERSION-FILE, no?
>>
>> Hi Junio,
>>
>> ... Let's not introduce second-guessing into
>> the script....
>
> Fine then.  Or you could just append "." to the $PATH ;-)

"." in the super user's PATH? Cool stuff, and so innovative. Economy  
crisis special offer - get the barndoor-sized hole for a piece bird dung.  
Special offer valid only today, while stocks last...

SCNR.

-- 
Matthias Andree

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

[Junio C Hamano]

"Matthias Andree" <matthias.andree@gmx.de> writes:

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

What's innovative is whoever is running build as root.

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

[Francis Galiegue]

Le Saturday 09 May 2009 18:55:09 Junio C Hamano, vous avez écrit :
[...]
>
> What's innovative is whoever is running build as root.
>

Well, maybe not if you just download the source and use 
the "configure/make/make install" trinity. But when it comes to packaging, 
it's another story.

I've been doing RPM packaging for quite a few years. I have been fortunate 
enough that my first job was with a Linux distribution (it was called 
Mandrake at the time) and, first things first, they taught me how to set up 
an RPM environment to build as a regular user.

Believe it or not, but even as of today, in 2009, neither RHEL or its 
immediate derivative (CentOS) manage to build a decent set of rules to build 
as a non-root user. You have to make your own $HOME/.rpmmacros at the very 
least. So, unless you are a skilled enough packager, you cannot even build a 
package as a regular user. And some packages out there DO require skills as a 
packager to just be built as packages (qmail is one example).

And even as a regular user, and even though you can, say, alter all 
of /usr/local to be writeable by someone else than root, I wouldn't be 
surprised to hear that a LOT of Linux beginners, seeing that "make install 
doesn't work", resort to being root instead. Because it is a known fact that 
root can do everything.

Innovative? Not that much.

-- 
Francis Galiegue
fge@one2team.com
Ingénieur système
Mob : +33 (0) 683 877 875
Tel : +33 (0) 178 945 552
One2team
40 avenue Raymond Poincaré
75116 Paris

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

[Matthias Andree]

Am 09.05.2009, 18:55 Uhr, schrieb Junio C Hamano <gitster@pobox.com>:

> "Matthias Andree" <matthias.andree@gmx.de> writes:
>
>>> 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.
>
> What's innovative is whoever is running build as root.

Back to the real problem, and that is re-checking GIT-VERSION-FILE as part
of "make install". I wonder if we should just have GIT-VERSION-GEN exit if
git isn't in $PATH, and at most copy version there -- rather than stomp
the DEF_VER somewhere.

-- 
Matthias Andree

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

[Matthias Andree]

Am 09.05.2009, 18:55 Uhr, schrieb Junio C Hamano <gitster@pobox.com>:

> "Matthias Andree" <matthias.andree@gmx.de> writes:
>
>>> 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.

BTW, in the earlier version, I used type(1) to take $PATH search into  
account in case GIT=git; test -x does not do path search, unlike type.

> What's innovative is whoever is running build as root.

Yes, and that is why I found the PATH-dependent behaviour so irritating  
and wanted to fix it. I have another approach cooking that entails  
factoring out common code from ./git-gui/GIT-VERSION-GEN and  
./GIT-VERSION-GEN into ./git-gui/GIT-VERSION-SUBR.

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.

Please let me know if you're willing to accept a patch that touches both  
Makefile and the GIT-VERSION-* shell scripts. If you don't, I can quit  
here and not waste further time on submissions that are inacceptable  
anyhow, but just keep rebasing my local patch instead.

Best regards

-- 
Matthias Andree

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

[Junio C Hamano]

"Matthias Andree" <matthias.andree@gmx.de> writes:

> Am 09.05.2009, 18:55 Uhr, schrieb Junio C Hamano <gitster@pobox.com>:
>
>> "Matthias Andree" <matthias.andree@gmx.de> writes:
>>
>>>> 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.

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

[Nanako Shiraishi]

Quoting Junio C Hamano <gitster@pobox.com>:

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

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

[Junio C Hamano]

Nanako Shiraishi <nanako3@lavabit.com> writes:

> Quoting Junio C Hamano <gitster@pobox.com>:
>
>> 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/

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

[Johannes Sixt]

On Dienstag, 2. Juni 2009, Junio C Hamano wrote:
> Nanako Shiraishi <nanako3@lavabit.com> writes:
> > 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 <j6t@kdbg.org>

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

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

[Matthias Andree]

Am 02.06.2009, 20:35 Uhr, schrieb Johannes Sixt <j6t@kdbg.org>:

> On Dienstag, 2. Juni 2009, Junio C Hamano wrote:
>> Nanako Shiraishi <nanako3@lavabit.com> writes:
>> > 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 <j6t@kdbg.org>
>
> 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='
>  '

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

[PATCH v4] To make GIT-VERSION-FILE, search for git more widely

[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 <matthias.andree@gmx.de>
---
 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

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

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

> On Dienstag, 2. Juni 2009, Junio C Hamano wrote:
>> Nanako Shiraishi <nanako3@lavabit.com> writes:
>> > 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 <j6t@kdbg.org>
>
> 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='
>  '

I actually think this is much saner and cleaner (it certainly is smaller),
especially having seen Matthias's v4, which feels a tad overengineered.

I honestly do not understand why we need to bend backwards to cater to
"sudo".  Real men, when needing to do things as root, have always done
"su", and _if_ the environment is unsuited for the job, they can do:

	$ su
        # PATH=$PATH:/usr/local/bin make prefix=/usr/local install

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

[Matthias Andree]

Am 04.06.2009, 07:18 Uhr, schrieb Junio C Hamano <gitster@pobox.com>:

> Johannes Sixt <j6t@kdbg.org> writes:
>
>> On Dienstag, 2. Juni 2009, Junio C Hamano wrote:
>>> Nanako Shiraishi <nanako3@lavabit.com> writes:
>>> > 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 <j6t@kdbg.org>
>>
>> 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='
>>  '
>
> I actually think this is much saner and cleaner (it certainly is  
> smaller),
> especially having seen Matthias's v4, which feels a tad overengineered.

It's nothing more than

(a) followed your suggestion to look in $(prefix) first for cross-building  
support

(b) ported to git-gui as well (copy & paste-style)

Also, we certainly do not want to stuff "." in root's PATH, not even for  
simple scripts like GIT-VERSION-GEN.

> I honestly do not understand why we need to bend backwards to cater to
> "sudo".  Real men, when needing to do things as root, have always done
> "su", and _if_ the environment is unsuited for the job, they can do:
>
> 	$ su
>         # PATH=$PATH:/usr/local/bin make prefix=/usr/local install

sudo caches passwords for a couple of minutes, su does not, and su isn't  
available everywhere ("wheel" group on BSD and stuff); particularly, sudo  
is *the* get-root-tool on Ubuntu.

If you argue "real men", then break that damn rebuild cycle and either fix  
dependencies properly, rather than second-guessing in shell scripts at  
"make install" time, or add post-update hooks (or whatever) to update the  
GIT-VERSION-FILE...

"." doesn't belong in root's $PATH, period.

-- 
Matthias Andree