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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: May 22, 2025, 13:21 UTC
Message-ID: <aa1f960a-e7a5-4ada-84c0-fbf86a6c873f@gmail.com>
In-Reply-To: <20250520193506.95199-1-ben.knoble+github@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

Show 35 quoted lines

> 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