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

>
> "D. Ben Knoble" <ben.knoble+github@gmail.com> 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?

>
> > @@ -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?