Re: PATH modifications for git-hook processes

2015-04-22 Thread Junio C Hamano
Jeff King  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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: PATH modifications for git-hook processes

2015-04-21 Thread David Rodríguez
Jeff King  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.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: PATH modifications for git-hook processes

2015-04-15 Thread 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: PATH modifications for git-hook processes

2015-04-15 Thread 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: PATH modifications for git-hook processes

2015-04-15 Thread 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  wrote:
> Matthew Rothenberg  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 " does this before running "git-"
> (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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: PATH modifications for git-hook processes

2015-04-14 Thread Junio C Hamano
Matthew Rothenberg  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 " does this before running "git-"
(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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html