Adding Patrick to CC
On Tue, May 20, 2025 at 4:33 PM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> "D. Ben Knoble" <ben.knoble+github@xxxxxxxxx> writes:
>
> > 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.
>
> I can understand why we want to do this for things like editors, but
> doing this in start_command() would affect running of things like
> hook scripts (which may in turn invoke "git" commands) and command
> running on the other side of the pipe that does a local transport
> (e.g., "git fetch --upload-pack cmd"), and other commands specified
> via the configuration (e.g., "trailer.<key>.cmd") that may in turn
> invoke "git" commands, wouldn't it?
It would affect those things, yes, but I'm not sure what /effect/ that
has: doesn't adding GIT_EXEC_PATH to PATH only matter if we use the
dashed forms ("git-*" in my original message), which is long
deprecated?
Patrick pointed out in a parallel thread that it probably matters for
". git-sh-setup", but that manual explicitly describes using
. "$(git --exec-path)/git-sh-setup"
So I would say the callers that don't do that are wrong ;)
>
> > @@ -746,7 +789,7 @@ int start_command(struct child_process *cmd)
> > if (cmd->close_object_store)
> > close_object_store(the_repository->objects);
> >
> > - childenv = prep_childenv(cmd->env.v);
> > + childenv = prep_childenv(cmd->env.v, cmd->git_cmd);
>
> Regardless of the answer to the above question, I wonder if the API
> change for this helper function should go the other direction, by
> passing the entire "child_process" structure to the function. That
> would give prep_childenv() more information to work with to decide
> when futzing with PATH is appropriate.
>
> Yes, I am assuming that the answer to the above question is "true,
> unconditionally futzing with PATH is a bad idea" and we need this
> fix to be more focused, e.g., limit to "launch_specified_editor()".
> If that is the case, we can
>
> - add a member "drop_git_exec_path" bit to "struct child_process";
>
> - update selected callers, e.g., launch_specified_editor(), that
> prepare child_process to set that bit;
>
> - update prep_childenv() to accept the entire "struct child_process",
> instead of cmd->env.v, and base the decision in your logic to
> look at the cmd->drop_git_exec_path bit;
>
> perhaps. It would give us a more surgical and focused update,
> hopefully?
Indeed, a more surgical approach is what Dscho originally suggested
(IIUC). Failing CI suggests there may be more that relies on
GIT_EXEC_PATH in PATH than I realized, so I can post an alternative
series that should only affect editors (requiring an extra patch for
contrib/git-jump, since it invokes an editor), but it is likely to be
"incomplete" in some sense:
- no Windows support (see replies to 3/4)
- won't affect all editor invocations, since those launched by a
script after retrieving "git var GIT_EDITOR" won't necessarily know to
rollback PATH.
I need to reroll anyway, so I think what I'll do is post a v2 of this
series along with a v3 that refocuses on editors? That way we have
both around for interested folks in the future.
