From: Andreas Ericsson Date: Tue, 10 Jan 2006 19:13:34 GMT Subject: Re: [PATCH 2/2] Remember and use GIT_EXEC_PATH on exec()'s Message-ID: <43C4075E.4070407@op5.se> In-Reply-To: <1136910406.11717.579.camel@brick.watson.ibm.com> Michal Ostrowski wrote: > On Tue, 2006-01-10 at 16:01 +0100, Andreas Ericsson wrote: > > >>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. > >>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. > >>>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. >> >>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