Re: [PATCH 4/4] drop git_exec_path() from non-Git commands' PATH

["D. Ben Knoble" <ben.knoble+github@gmail.com>]

On Wed, May 21, 2025 at 4:27 AM Patrick Steinhardt <ps@pks.im> 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