threads / discuss / 39073

PATH modifications for git-hook processes

Subject: PATH modifications for git-hook processes

## tl;dr

12 messages between Apr 14, 2015 and Apr 22, 2015.

replies: 11people: 6as markdown or json

Matthew Rothenberg· Apr 14, 2015, 16:04 UTC · lore

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!
Junio C Hamano· Apr 14, 2015, 17:17 UTC · re: Matthew Rothenberg · lore

Re: PATH modifications for git-hook processes

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.

Matthew Rothenberg· Apr 15, 2015, 15:00 UTC · re: Junio C Hamano · lore

Re: PATH modifications for git-hook processes

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:
Show 24 quoted lines
> 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.
Jeff King· Apr 16, 2015, 06:17 UTC · re: Matthew Rothenberg · lore

Re: PATH modifications for git-hook processes

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.

Show 8 quoted lines
>   - 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
Jeff King· Apr 16, 2015, 06:31 UTC · re: Jeff King · lore

Re: PATH modifications for git-hook processes

On Thu, Apr 16, 2015 at 02:17:32AM -0400, Jeff King wrote:
Show 11 quoted lines
> 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
David Rodríguez· Apr 22, 2015, 00:39 UTC · re: Jeff King · lore

Re: PATH modifications for git-hook processes

Jeff King <peff <at> peff.net> writes:
Show 7 quoted lines
> 
> 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.

Junio C Hamano· Apr 22, 2015, 17:46 UTC · re: Jeff King · lore

Re: PATH modifications for git-hook processes

Jeff King <peff@peff.net> writes:
Show 13 quoted lines
>   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.

Jeff King· Apr 22, 2015, 18:14 UTC · re: Junio C Hamano · lore

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

On Wed, Apr 22, 2015 at 10:46:57AM -0700, Junio C Hamano wrote:
Show 10 quoted lines
> > 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
Eric Sunshine· Apr 22, 2015, 18:23 UTC · re: Jeff King · lore

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

On Wed, Apr 22, 2015 at 2:14 PM, Jeff King <peff@peff.net> wrote:
Show 8 quoted lines
> 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/
Show 22 quoted lines
> 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
Jeff King· Apr 22, 2015, 18:36 UTC · re: Eric Sunshine · lore

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

On Wed, Apr 22, 2015 at 02:23:27PM -0400, Eric Sunshine wrote:
Show 11 quoted lines
> 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
Jonathan Nieder· Apr 22, 2015, 19:23 UTC · re: Jeff King · lore

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

Hi,
Jeff King wrote:
Show 13 quoted lines
>                                         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>
[...]
Show 5 quoted lines
>                                          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

Jeff King· Apr 22, 2015, 20:00 UTC · re: Jonathan Nieder · lore

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

On Wed, Apr 22, 2015 at 12:23:17PM -0700, Jonathan Nieder wrote:
Show 7 quoted lines
> >                                                          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

← back to recent threads