PATH modifications for git-hook processes

PATH modifications for git-hook processes

[Matthew Rothenberg]

Using git 2.3.5 (on darwin, installed via homebrew), when executing a
script via the commit-msg githook, the following gets *prepended* to
the $PATH for the subprocess:

  /usr/local/Cellar/git/2.3.5/libexec/git-core:/usr/local/Cellar/git/2.3.5/libexec/git-core:/usr/local/bin:{rest
of path...}

I can't find any documentation about this path modification behavior
in the git docs, and I'm not familiar enough with the source code to
locate it there either.

In our case, the prepending of /usr/local/bin is particularly
problematic, as it effectively reorders our chosen PATH hierarchy (in
which user-space versions of commands are picked up prior to system
installed ones).

(It's also curious that the git-core directory gets prepended twice,
but that doesn't cause us any particular issues.)

My questions:
 - what is the expected PATH modification behavior for subprocesses of
git-hooks? Is this documented anywhere?
 - what would be causing /usr/local/bin to be prepended here, and can
it be adjusted via preference?

Thanks!

Re: PATH modifications for git-hook processes

[Junio C Hamano]

Matthew Rothenberg <mroth@khanacademy.org> writes:

>  - what is the expected PATH modification behavior for subprocesses of
> git-hooks? Is this documented anywhere?
>  - what would be causing /usr/local/bin to be prepended here, and can
> it be adjusted via preference?

This is not limited to hooks and very much deliberate, I think.  In
order to make sure anything that is called from "git" wrapper gets
picked up from GIT_EXEC_PATH so that the matching version of the git
subprogram is used, "git <cmd>" does this before running "git-<cmd>"
(in git.c):

	/*
	 * We use PATH to find git commands, but we prepend some higher
	 * precedence paths: the "--exec-path" option, the GIT_EXEC_PATH
	 * environment, and the $(gitexecdir) from the Makefile at build
	 * time.
	 */
	setup_path();

And setup_path() is defined in exec_cmd.c to start the new PATH with
git-exec-path, and then the path to the running "git" itself, and
then finally the external PATH.

Re: PATH modifications for git-hook processes

[Matthew Rothenberg]

Hmm, this all makes sense as to why it's happening, thank you.  In my
case the ` /usr/local/Cellar/git/2.3.5/libexec/git-core` (git
--exec-path) does give all the proper binaries and sub-binaries. It
shows up twice because the GIT_EXEC_PATH environment variable is used
too (which is the same in my case since it hasn't been overriden).
The /usr/local/bin therefore comes from **the path to the running
"git" itself**.

There still seems to be a potential issue I can't figure out how to
work around with this.  If **the path to the running "git" itself** is
in /usr/local/bin or some other common area, then that would still
always get prepended prior to external PATH -- which means **other**
external programs will inherit precedence overriding the system PATH
preferences.

For example, in our case, many scripts run in our specific Python
environment, which ala virtualenv is located in a user-specific path
(e.g. ~/.virtualenv/foo/bin/python), which appears earlier in the user
PATH so it affects all shell processes using `#!/usr/bin/env python`.
When a git-exec prepends /usr/local/bin, the system installed Python
is used instead.

There are other use cases I can think of that would cause this issue
as well -- user provides more recent version of "bazfoo" program in
~/bin which they prepend of their system PATH, git-exec then prepends
shared path of a system binary directory which also contains older
version of bazfoo, older version then gets used instead.

So, I guess what I'm looking for is:
  - A way to prevent the **path to the running "git" itself** portion
of setup_path from firing, (OR)
  - A way to specify (via env var?) paths that must remain in high
precedence even during a git-exec, e.g.:
      NEWPATH = [git --exec-path] : [GIT_EXEC_PATH] :
[$PROPOSED_HIGH_PRECENDENCE_PATH] : ['git itself' path] : [$PATH] (OR)
  - A way to refine git-exec default behavior to avoid this edge case
(my C is a little rusty but I'm happy to try to help out if we can
think of a good logic), (OR)
  - Something else clever I am not aware of.

Thanks so much for your assistance.

On Tue, Apr 14, 2015 at 1:17 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Matthew Rothenberg <mroth@khanacademy.org> writes:
>
>>  - what is the expected PATH modification behavior for subprocesses of
>> git-hooks? Is this documented anywhere?
>>  - what would be causing /usr/local/bin to be prepended here, and can
>> it be adjusted via preference?
>
> This is not limited to hooks and very much deliberate, I think.  In
> order to make sure anything that is called from "git" wrapper gets
> picked up from GIT_EXEC_PATH so that the matching version of the git
> subprogram is used, "git <cmd>" does this before running "git-<cmd>"
> (in git.c):
>
>         /*
>          * We use PATH to find git commands, but we prepend some higher
>          * precedence paths: the "--exec-path" option, the GIT_EXEC_PATH
>          * environment, and the $(gitexecdir) from the Makefile at build
>          * time.
>          */
>         setup_path();
>
> And setup_path() is defined in exec_cmd.c to start the new PATH with
> git-exec-path, and then the path to the running "git" itself, and
> then finally the external PATH.

Re: PATH modifications for git-hook processes

[Jeff King]

On Wed, Apr 15, 2015 at 11:00:20AM -0400, Matthew Rothenberg wrote:

> So, I guess what I'm looking for is:
>   - A way to prevent the **path to the running "git" itself** portion
> of setup_path from firing, (OR)

Yeah, this behavior leaves me somewhat confused. It is obviously
dangerous (since we have no clue what else is in the directory along
with git, as opposed to the exec-path, which contains only git
programs), and that is biting you here. But is it actually doing any
good?

Surely we don't need to find the "git" executable from that directory;
it should already be in the exec-path itself. Ditto for any dashed
commands (built-ins or external commands) that git ships.

The only case I could come up with (that works now, but would not work
if we stopped prepending the argv0 directory to the PATH) is:

  mkdir foo
  cp $(which git) foo
  cat >foo/git-bar <<\EOF
  #!/bin/sh
  echo this external command is not in your PATH!
  EOF
  chmod +x foo/git-bar
  foo/git bar

but I am not sure that is altogether sane.

I tried to find some reasoning for this behavior in the history, and
this is what I found.

We originally started treating git programs as magical in 77cb17e (Exec
git programs without using PATH., 2006-01-10), but it specifically warns
against this:

  Modifying PATH is not desirable as it result in behavior differing
  from the user's intentions, as we may end up prepending "/usr/bin" to
  PATH.

So it uses a special git-specific version of exec, and doesn't touch the
PATH. Later, we did 231af83 (Teach the "git" command to handle some
commands internally, 2006-02-26), which says at the end:

   There's one other change: the search order for external programs is
   modified slightly, so that the first entry remains GIT_EXEC_DIR, but
   the second entry is the same directory as the git wrapper itself was
   executed out of - if we can figure it out from argv[0], of course.

There was some question of whether this would be a problem[1], but we
realized it is OK due to 77cb17e, mentioned above. The use case
described by Linus is more or less the "not altogether sane" scenario I
described above:

  IOW, you can do things like

        alias git=/opt/my-git/git

  and all the "git" commands will automatically work fine, even if you
  didn't know at compile time where you would install them, and you didn't
  set GIT_EXEC_DIR at run-time. It will still first look in GIT_EXEC_DIR,
  but if that fails, it will take the git commands from /opt/my-git/ instead
  of from /usr/bin or whatever.

I think this is pretty much a non-issue for stock git commands (we do
not even put them into /opt/my-git these days, so it cannot be helping
there). It's really about finding third-party commands you have added.

Later we did 511707d (use only the $PATH for exec'ing git commands,
2007-10-28) which went back to munging the PATH, and dropping the
protection we relied on when doing 231af83 (and causing the breakage
you're seeing today).

So I dunno. I find the current behavior quite questionable. I guess I
can see it coming in handy, but as you note it can also cause problems.
And "put the git-* programs somewhere in your PATH or git's exec-dir if
you want to use them" does not seem like that bad a thing to require
people to do.

>   - A way to specify (via env var?) paths that must remain in high
> precedence even during a git-exec, e.g.:
>       NEWPATH = [git --exec-path] : [GIT_EXEC_PATH] :
> [$PROPOSED_HIGH_PRECENDENCE_PATH] : ['git itself' path] : [$PATH] (OR)
>   - A way to refine git-exec default behavior to avoid this edge case
> (my C is a little rusty but I'm happy to try to help out if we can
> think of a good logic), (OR)
>   - Something else clever I am not aware of.

If we can get away with just dropping this element from the PATH, I'd
much rather do that than try to implement a complicated path-precedence
scheme.

-Peff

Re: PATH modifications for git-hook processes

[Jeff King]

On Thu, Apr 16, 2015 at 02:17:32AM -0400, Jeff King wrote:

> So it uses a special git-specific version of exec, and doesn't touch the
> PATH. Later, we did 231af83 (Teach the "git" command to handle some
> commands internally, 2006-02-26), which says at the end:
> 
>    There's one other change: the search order for external programs is
>    modified slightly, so that the first entry remains GIT_EXEC_DIR, but
>    the second entry is the same directory as the git wrapper itself was
>    executed out of - if we can figure it out from argv[0], of course.
> 
> There was some question of whether this would be a problem[1], but we
> realized it is OK due to 77cb17e, mentioned above.

I forgot to include my footnote, which was a link to the thread:

  http://thread.gmane.org/gmane.comp.version-control.git/16798

-Peff

Re: PATH modifications for git-hook processes

[David Rodríguez]

Jeff King <peff <at> peff.net> writes:
> 
> If we can get away with just dropping this element from the PATH, I'd
> much rather do that than try to implement a complicated path-precedence
> scheme.
> 
> -Peff
> 

I agree, GIT_EXEC_DIR should be enough and this surprising behavior would be
avoided.

Re: PATH modifications for git-hook processes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>   IOW, you can do things like
>
>         alias git=/opt/my-git/git
>
>   and all the "git" commands will automatically work fine, even if you
>   didn't know at compile time where you would install them, and you didn't
>   set GIT_EXEC_DIR at run-time. It will still first look in GIT_EXEC_DIR,
>   but if that fails, it will take the git commands from /opt/my-git/ instead
>   of from /usr/bin or whatever.
>
> If we can get away with just dropping this element from the PATH, I'd
> much rather do that than try to implement a complicated path-precedence
> scheme.

I am OK with dropping it at a major version boundary with
deprecation notice in the release note.  Unlike older days, by now,
Git has become so essential to users' everyday life, and there is
not much reason for people to keep the installation of Git they
built outside their $PATH, and "alias git=/opt/git/bin/git" has lost
much of its value, I would think.

[PATCH] stop putting argv[0] dirname at front of PATH

[Jeff King]

On Wed, Apr 22, 2015 at 10:46:57AM -0700, Junio C Hamano wrote:

> > If we can get away with just dropping this element from the PATH, I'd
> > much rather do that than try to implement a complicated path-precedence
> > scheme.
> 
> I am OK with dropping it at a major version boundary with
> deprecation notice in the release note.  Unlike older days, by now,
> Git has become so essential to users' everyday life, and there is
> not much reason for people to keep the installation of Git they
> built outside their $PATH, and "alias git=/opt/git/bin/git" has lost
> much of its value, I would think.

Actually, on looking further into it, I am not sure that the use-case
originally presented hasn't simply been broken since 2007. The patch
below is trivial, but I've tried to summarize the situation in the
commit message.

If we are really breaking workflows, I agree on the deprecation. But I
think we may not be, in which case it would be fine (IMHO) to skip the
deprecation notice.

-- >8 --
Subject: stop putting argv[0] dirname at front of PATH

When the "git" wrapper is invoked, we prepend the baked-in
exec-path to our PATH, so that any sub-processes we exec
will all find the git-foo commands that match the wrapper
version.

If you invoke git with an absolute path, like:

  /usr/bin/git foo

we also prepend "/usr/bin" to the PATH. This was added long
ago by by 231af83 (Teach the "git" command to handle some
commands internally, 2006-02-26), with the intent that
things would just work if you did something like:

  cd /opt
  tar xzf premade-git-package.tar.gz
  alias git=/opt/git/bin/git

as we would then find all of the related external commands
in /opt/git/bin. I.e., it made git runtime-relocatable,
since at the time of 231af83, we installed all of the git
commands into $(bindir). But these days, that is not enough.
Since f28ac70 (Move all dashed-form commands to libexecdir,
2007-11-28), we do not put commands into $(bindir), and you
actually need to convert "/usr/bin" into "/usr/libexec". And
not just for finding binaries; we want to find $(sharedir),
etc, the same way.  The RUNTIME_PREFIX build knob does this
the right way, by assuming a sane hierarchy rooted at
"$prefix" when we run "$prefix/bin/git", and inferring
"$prefix/libexec/git-core", etc.

So this feature (prepending the argv[0] dirname to the PATH)
is broken for providing a runtime prefix, and has been for
many years. Does it do anything for other cases?

For the "git" wrapper itself, as well as any commands
shipped by "git", the answer is no. Those are already in
git's exec-path, which is consulted first. For third-party
commands which you've dropped into the same directory, it
does include them. So if you do

  cd /opt
  tar xzf git-built-specifically-for-opt-git.tar.gz
  cp third-party/git-foo /opt/git/bin/git-foo
  alias git=/opt/git/bin/git

it does mean that we will find the third-party "git-foo",
even if you do not put /opt/git/bin into your $PATH. But
the flipside of this is that we will bump the precedence of
_other_ third-party tools that happen to be in the same
directory as git. For example, consider this setup:

  1. Git is installed by the system in /usr/bin. There are
     other system utilities in /usr/bin. E.g., a system
     "vi".

  2. The user installs tools they prefer in /usr/local/bin.
     E.g., vim with a "vi" symlink. They set their PATH to
     /usr/local/bin:/usr/bin to prefer their custom tools.

  3. Running /usr/bin/git puts "/usr/bin" at the front of
     their PATH. When git invokes the editor on behalf of
     the user, they get the system vi, not their normal vim.

There are other variants of this, including overriding
system ruby and python (which is quite common using tools
like "rvm" and "virtualenv", which use relocatable
hierarchies and $PATH settings to get a consistent
environment).

Given that the main motivation for git pulling the argv[0]
dirname into the PATH has been broken for years, that the
remaining cases are obscure and unlikely (and easily fixed
by the user just setting up their $PATH sanely), and that
the behavior is hurting real, reasonably common use cases,
it's not worth continuing to do so.

Signed-off-by: Jeff King <peff@peff.net>
---
If people _are_ interested in relocatable binary packages, I think
RUNTIME_PREFIX is the right way forward. But note that you can't just
flip on RUNTIME_PREFIX on non-Windows systems, as some invocations will
get the full path to the executable, and others see just "git". You'd
need to convert that into an absolute path (either by searching the
$PATH, or doing something system-specific like looking in /proc/$$/exe).

 exec_cmd.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/exec_cmd.c b/exec_cmd.c
index 8ab37b5..e85f0fd 100644
--- a/exec_cmd.c
+++ b/exec_cmd.c
@@ -96,7 +96,6 @@ void setup_path(void)
 	struct strbuf new_path = STRBUF_INIT;
 
 	add_path(&new_path, git_exec_path());
-	add_path(&new_path, argv0_path);
 
 	if (old_path)
 		strbuf_addstr(&new_path, old_path);
-- 
2.4.0.rc2.498.g02440db

Re: [PATCH] stop putting argv[0] dirname at front of PATH

[Eric Sunshine]

On Wed, Apr 22, 2015 at 2:14 PM, Jeff King <peff@peff.net> wrote:
> Subject: stop putting argv[0] dirname at front of PATH
>
> When the "git" wrapper is invoked, we prepend the baked-in
> exec-path to our PATH, so that any sub-processes we exec
> will all find the git-foo commands that match the wrapper
> version.
> [...]
> Given that the main motivation for git pulling the argv[0]

s/pulling/putting/

> dirname into the PATH has been broken for years, that the
> remaining cases are obscure and unlikely (and easily fixed
> by the user just setting up their $PATH sanely), and that
> the behavior is hurting real, reasonably common use cases,
> it's not worth continuing to do so.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> diff --git a/exec_cmd.c b/exec_cmd.c
> index 8ab37b5..e85f0fd 100644
> --- a/exec_cmd.c
> +++ b/exec_cmd.c
> @@ -96,7 +96,6 @@ void setup_path(void)
>         struct strbuf new_path = STRBUF_INIT;
>
>         add_path(&new_path, git_exec_path());
> -       add_path(&new_path, argv0_path);
>
>         if (old_path)
>                 strbuf_addstr(&new_path, old_path);
> --
> 2.4.0.rc2.498.g02440db

Re: [PATCH] stop putting argv[0] dirname at front of PATH

[Jeff King]

On Wed, Apr 22, 2015 at 02:23:27PM -0400, Eric Sunshine wrote:

> On Wed, Apr 22, 2015 at 2:14 PM, Jeff King <peff@peff.net> wrote:
> > Subject: stop putting argv[0] dirname at front of PATH
> >
> > When the "git" wrapper is invoked, we prepend the baked-in
> > exec-path to our PATH, so that any sub-processes we exec
> > will all find the git-foo commands that match the wrapper
> > version.
> > [...]
> > Given that the main motivation for git pulling the argv[0]
> 
> s/pulling/putting/

Heh. Too much git for me, I think.

-Peff

Re: [PATCH] stop putting argv[0] dirname at front of PATH

[Jonathan Nieder]

Hi,

Jeff King wrote:

>                                         This was added long
> ago by by 231af83 (Teach the "git" command to handle some
> commands internally, 2006-02-26), with the intent that
> things would just work if you did something like:
>
>   cd /opt
>   tar xzf premade-git-package.tar.gz
>   alias git=/opt/git/bin/git
>
> as we would then find all of the related external commands
> in /opt/git/bin. I.e., it made git runtime-relocatable,
> since at the time of 231af83, we installed all of the git
> commands into $(bindir).
[...]
>                                                          And
> not just for finding binaries; we want to find $(sharedir),
> etc, the same way.  The RUNTIME_PREFIX build knob does this
> the right way

Makes sense.  For the reason you say (templatedir, etc) I am surprised
to hear that that was the motivation, but I can't find any other.

[...]
> Signed-off-by: Jeff King <peff@peff.net>

For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[...]
>                                          But note that you can't just
> flip on RUNTIME_PREFIX on non-Windows systems, as some invocations will
> get the full path to the executable, and others see just "git". You'd
> need to convert that into an absolute path (either by searching the
> $PATH, or doing something system-specific like looking in /proc/$$/exe).

Yep --- /proc/self/exe should work okay if someone wants "portable
git" to work on Linux.

Thanks,
Jonathan

Re: [PATCH] stop putting argv[0] dirname at front of PATH

[Jeff King]

On Wed, Apr 22, 2015 at 12:23:17PM -0700, Jonathan Nieder wrote:

> >                                                          And
> > not just for finding binaries; we want to find $(sharedir),
> > etc, the same way.  The RUNTIME_PREFIX build knob does this
> > the right way
> 
> Makes sense.  For the reason you say (templatedir, etc) I am surprised
> to hear that that was the motivation, but I can't find any other.

The Makefile was much less mature, then, so I wondered if maybe those
things came later (certainly the munging of $PATH that introduced the
problems came later, as at the time we had a special "we are exec-ing a
git command" custom version of execvp).

But no, the template directory did indeed exist then. I think it was
always a half-baked idea (and got much worse when we stopped putting
git-foo into $bindir).

The relevant thread (which I think I linked the other day) is:

  http://article.gmane.org/gmane.comp.version-control.git/16798

-Peff