Re: [PATCH 2/2] Remember and use GIT_EXEC_PATH on exec()'s

Michal Ostrowski wrote:

On Tue, 2006-01-10 at 16:01 +0100, Andreas Ericsson wrote:

quoted

This is the case in the git potty already. git.c must prepend 
--exec-path to $PATH, or the whole idea of being able to move scripts 
out of the $PATH fails (at least it fails without changing quite a few 
of the scripts).

One could make all the scripts depend on GIT_EXEC_PATH instead of PATH.
At build time one could generate wrapper functions in git-sh-setup:

function git-foo () {
	$(GIT_EXEC_PATH)/git-foo $*;
}

Presuming that all scripts include git-sh-setup, no other shell script
changes would be needed.

Yuck, for two reasons.

* Not all scripts include git-sh-setup, and for good reasons. If this is 
what you intend please make sure you don't break anything in the process.

* This will spawn a sub-shell for each git-foo process called. Shells 
are way more expensive than the git potty, so the performance hit in 
iterations might be considerable. Think StGit and Cogito as well.


On a side-note, $* will break quoting (you should use "$@" instead, with 
double-quotes attached), and

	$(GIT_EXEC_PATH)/git-foo $*

will try to execute GIT_EXEC_PATH and prepend its output to the rest of 
the command, which is quite obviously wrong.

quoted

Since it's already in place in the potty and that's required to be in 
the $PATH, I think Junio's suggestion of running execlp("git", "git", 
...) is a good one. It will add one extra fork() and execve() for each 
clone/pull/push, but that isn't much of an issue, really.

The patch I posted most recently does something comparable; all exec's
by C git programs go through exec_git_cmd, which actually implements the
"git potty" logic (and git.c itself uses exec_git_cmd).  If there is to
be a consistent rule for how to exec a git program from a git C program,
I think that it's reasonable that there be an API to enforce it.  

True. Perhaps I misread your patch or your reasoning.

Note that the creation and use of such a function simply means that we
hide the logic that handles PATH/GIT_EXEC_PATH; how git_exec_cmd()
actually calls execve() and how PATH and GIT_EXEC_PATH are used is a
separate issue.  When it comes to the former, I think it is best to have
all exec's of git programs go through an interface that imposes the same
PATH/GIT_EXEC_PATH logics.  As to the latter, my only concern is that we
should never do 'setenv("PATH",....)'.

setenv("PATH", ..) is way preferrable over the git-setup.sh hackery 
suggested above, so long as it's only ever done in the git potty. That's 
what the potty is there for, after all.

quoted

quoted

An approach that I think is better is to require all exec's of git
programs from within git programs to use a specific git interface,
rather than letting each one set up it's own exec parameters.

A better idea would be to teach {send,upload}-pack about $GIT_EXEX_PATH 
and export it from your shells rc-file.

My shell's rc-file doesn't get invoked when using ssh as a transport;
that's part of the problem.

It does for me and everybody else. $HOME/.bashrc is read even for 
non-interactive shells. $HOME/.bash_profile isn't. If you're using 
git-shell you're in to a whole different situation, but you didn't say 
so so I don't think you are.

quoted

If a user invokes "/home/user/bin/git-foo" rather than 
"/home/user/bin/git foo" he/she will have to have the rest of the 
git-suite in the $PATH. Prepending whatever directory any git-* program 
happens to reside in to $PATH is not a good idea. 

Isn't this exactly what git.c is doing currently via prepend_to_path()?

No. git prepends whatever was passed in --exec-path=/some/path, what's 
found in $GIT_EXEC_PATH or the GIT_EXEC_PATH pre-processor macro set at 
compile-time, in that order of preference. There's a very big difference.

git programs exec other git programs, but they also exec non-git
programs.  I think it is not appropriate to change PATH (via
prepend_to_path) because this may result in unexpected behavior when
exec'ing non-git programs:

Suppose git is installed in /usr/bin, where a "diff" resided.
I've got my own version of "diff" in /home/user/bin.
PATH=/home/user/bin:/usr/bin.

If git now tries to execute "diff", after having run
prepend_to_path(), /usr/bin/diff gets executed, not /home/user/bin/diff.
The user has set up PATH to ensure that /home/user/bin/diff is the diff,
but by mucking with PATH we subvert their intentions.

Good point. Perhaps we should only prepend to path when the directory 
isn't already in $PATH, or append rather than prepend.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231