On Wed, May 21, 2025 at 4:27 AM Patrick Steinhardt <ps@xxxxxx> wrote: > > On Tue, May 20, 2025 at 03:34:58PM -0400, D. Ben Knoble wrote: > > We setup_path() with git_exec_path() unconditionally (8e3462837b (Modify > > setup_path() to only add git_exec_path() to PATH, 2009-01-18)) when Git > > starts; as a result, all child processes see Git's exec-path when run, > > including editors and other programs that don't need it [1]. This can > > cause confusion for such programs or shells, especially when they rely > > on finding "git" in PATH to locate other nearby directories, in a > > similar vein as a0b4507ef7 (stop putting argv[0] dirname at front of > > PATH, 2015-04-22) solved. > > > > Since we only need this for finding git-* subprocesses, drop it from > > child processes that aren't Git commands. > > I agree with what Junio mentioned in a parallel thread, especially > around Git hooks. The expectation there is that those may execute other > Git commands, and that should typically be using the same execution > environment as the original Git command that has been invoking the hook. > So refining this patch so that the mechanism is opt-in probably makes > sense. Ok, will do in a reroll. Though if you have time… see my message in that thread about only affecting uses of "git-*" dashed commands. > A slight tangent: I wonder whether it is even required nowadays to > adapt PATH at all anymore. As far as I understand this was a > requirement back when people still executed dashed binaries > directly. But nowadays scripts don't really do that anymore, but > instead use the git binary. And that one doesn't need PATH to be > adapted at all, as it knows to listen to GIT_EXEC_PATH and its > built-in path anyway. Good point. I haven't tried running the tests with such a change, and GitHub CI at least reports a number of failures on this branch (whether that's because 3/4 is wrong for Windows or not, I'm not sure, but I suspect it's actually because 4/4 violates some assumptions). > So I have to wonder whether this is something that we can deprecate > to finalize the migration away from dashed builtins. There probably > are edge cases though. Shell scripts for example still execute ". > git-sh-setup", which I think relies on PATH to resolve the script's > location. Those scripts are, at least according to the manual for git-sh-setup, wrong ;) > In any case, this is of course outside of the scope of this patch > series. Noted; I'll leave it for later to tug that thread. [snip: style and leak discussion] Acked
