Re: [PATCH 0/4] Drop git-exec-path from non-Git child programs

[Phillip Wood <phillip.wood123@gmail.com>]

Hi Ben

On 20/05/2025 20:34, D. Ben Knoble wrote:
> This has caused trouble in the past [1] [2];

Another way of looking at this is that the trouble is caused by a script 
that makes incorrect assumptions about git.

The assumption that the scripts from contrib as installed at a fixed 
location relative to the git binary is false. Where they are installed 
and whether they are installed at all is down to the discretion of the 
distribution that you're using. Looking for "git jump" at a fixed offset 
from the git binary is no more portable than looking for it in a fixed 
location.

I think that the assumption that git should not change the environment 
when it runs the editor is unrealistic. "git commit file" will use a 
temporary index to create the commit and sets GIT_INDEX_FILE when 
running the editor. This means that if the editor wants to display the 
staged changes by running "git diff --cached HEAD" the diff will 
accurately represent the changes being committed. Adding GIT_EXEC_PATH 
to the beginning of PATH ensures that the diff will be created by the 
same version of git the the user ran which avoids subtle bugs where a 
sub-process of git runs a git command using an incompatible version of 
git. There are several other environment variables that may be set when 
running the editor such as GIT_DIR if the command is run from a linked 
wortree.

To create a clean environment when opening a terminal from your editor 
you can add

     PATH="${PATH#$GIT_EXEC_PATH:}"
     unset $(git rev-parse --local-env-vars)

to your shell setup script.

I think the first two patches are very welcome cleanups but I'm not 
convinced by the rationale for patches 3 & 4.

Best Wishes

Phillip

> after attempting to help
> Git-for-Windows avoid that problem in Vim [3] by recommendation [4], it
> was suggested that upstreaming the change instead would be a better
> solution. Indeed, this should work for more uses/editors/etc.
> 
> [1]: https://public-inbox.org/git/CALnO6CDtGRRav8zK2GKi1oHTZWrHFTxZNmnOWu64-ab+oY3_Lw@mail.gmail.com/
> [2]: https://benknoble.github.io/blog/2020/05/22/libexec-git-core-on-path/
> [3]: https://github.com/git-for-windows/build-extra/pull/616
> [4]: https://github.com/benknoble/Dotfiles/issues/143#issuecomment-2869525481
> 
> I haven't managed to test this on Windows, so any extra eyeballs there
> are greatly appreciated. I'd also appreciate suggestions to fix the
> memory leak.
> 
> Structure: patches 1 & 2 are cleanups. In particular, patch 1 is essential to
> the tests in patch 4. I don't think patch 2 is strictly needed. Patch 3
> refactors a little to make patch 4 take effect on all platforms. Patch 4 does
> the real work.
> 
> D. Ben Knoble (4):
>    t7005: sanitize test environment for subsequent tests
>    editor: use standard strvec API to receive environment for external
>      editors
>    run-command: prep_childenv on all platforms
>    drop git_exec_path() from non-Git commands' PATH
> 
>   builtin/commit.c  |  2 +-
>   editor.c          | 10 ++++-----
>   editor.h          |  7 +++---
>   run-command.c     | 55 ++++++++++++++++++++++++++++++++++++++++++-----
>   t/t7005-editor.sh | 18 +++++++++++++---
>   5 files changed, 75 insertions(+), 17 deletions(-)
> 
> 
> base-commit: 7a1d2bd0a596f42a8a7a68d55577967bb454fec0