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

From: Junio C Hamano <junkio@cox.net>
Date: Jan 10, 2006, 02:52 UTC
Message-ID: <7vwth8bxqd.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <1136849810.11717.518.camel@brick.watson.ibm.com>

Michal Ostrowski <mostrows@watson.ibm.com> writes:

> Calls to git_setup_exec_path() are inserted on paths that will execute
> other git programs. git_setup_exec_path() will ensure that the git
> installation directories are in the path.

About fetch-clone.c (which is shared by fetch-pack and clone-pack), it runs "git-index-pack" from finish_pack and "git-unpack-objects" from unpack_pack, so spelling these exec with execlp("git", "git", "index-pack", ...) might be cleaner, since "git" is required to be in users' PATH even though git-* may be moved out of the PATH in later versions of git. I dunno...

In send-pack.c, I wonder why you didn't do a setup_exec_path() at the beginning of main() instead of having two calls close to exec*() call site.

The same comment applies for run-command.c; you do it once for each child, but calling it once at the beginning of receive-pack would be good enough. The same thing for daemon.c.

I suspect you are trying to limit the extent of damage, but I do not think of a downside if we just call setup_exec_path() once at the beginning of main(). $GIT_EXEC_PATH _could_ have a private copy of broken "diff" to confuse diff-* family, but you cannot say "git diff" in such a setup anyway because "git" does the PATH prefixing already, so it would be a moot point.

Here is the list my "nm | grep ' exec[vlpe]*$$\|@@$'" found that use some variant of exec* family (except "git-diff-*"):

	clone-pack
        daemon
        fetch-pack
        merge-index
        peek-remote
        receive-pack
        send-pack
        shell
        ssh-fetch/ssh-pull
        ssh-upload/ssh-push
        upload-pack

I do not care too much about ssh-* commit walkers (users can say e.g. GIT_SSH_PUSH themselves).

Anyway, thanks for starting this. I need a bit more thought and a bit of list discussion to convince myself this is a good change.