Re: [PATCH 2/2] Remember and use GIT_EXEC_PATH on exec()'s
- From
Andreas Ericsson <ae@op5.se>
- Date
- Jan 10, 2006, 19:13 UTC
- Message-ID
- <43C4075E.4070407@op5.se>
- In-Reply-To
- <1136910406.11717.579.camel@brick.watson.ibm.com>
Michal Ostrowski wrote:
Show 19 quoted lines
> 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.
Show 14 quoted lines
>
>>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.
Show 8 quoted lines
> 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.
Show 14 quoted lines
>
>>>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.
Show 9 quoted lines
>> >>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.
Show 14 quoted lines
> 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