git-extra(vimrc): adjust $PATH to remove Git's libexec #616

benknoble · 2025-05-13T21:51:03Z

As described on the Git mailing list 1 and elsewhere 2, Git adjusts
PATH before invoking programs, and those adjustments are inherited by
editors if they are invoked as subprocesses. In most cases this is
harmless, but it can create confusion when spawning subprocesses from
the editor (as is common in Vim) and when the user's shell looks at PATH
or the location of a "git" binary.

Include portable code to strip these entries from PATH on startup.

Signed-off-by: D. Ben Knoble [email protected]

This code comes from my own Dotfiles 3 via 2 authors, myself and
https://github.com/Konfekt, on their recommendation 4.

The only major user-facing change would be that :!git-commit and
similar probably no longer work, since libexec-style dir containing
those binaries wouldn't be on PATH anymore. But Git doesn't support that
usage officially, anyway?

I took the liberty of swapping const for let in this version to make
it a little more portable between versions of Vim; I'm not sure which
version Git for Windows ships, so if I need to convert the lambda
expressions into "v:val" strings, I'll do so.

benknoble · 2025-05-14T00:44:12Z

range-diff: also update sha256sum

1:  2b4b5cf5 ! 1:  f8b44295 git-extra(vimrc): adjust $PATH to remove Git's libexec
    @@ Commit message
     
         Signed-off-by: D. Ben Knoble <[email protected]>
     
    + ## git-extra/PKGBUILD ##
    +@@ git-extra/PKGBUILD: source=('inputrc'
    +         'git-askpass.h'
    +         'git-askpass.rc')
    + sha256sums=('8ed76d1cb069ac8568f21c431f5e23caebea502d932ab4cdff71396f4f0d5b72'
    +-            'e36a3b93b8a33b0a74619f2449ed6d56ed54e4e2938b97070bce4926f1f44054'
    ++            '7736786309a58112b7ac60627b3df049eb5eac3e0627c364939277477aaa03f1'
    +             '640d04d2a2da709419188a986d5e5550ad30df23c7ea9be1a389c37a6b917994'
    +             '17c90698d4dd52dd9f383e72aee54ecea0f62520baf56722de5c83285507c0d9'
    +             '3cd83627f1d20e1108533419fcf33c657cbcf777c3dc39fa7f13748b7d63858a'
    +
      ## git-extra/vimrc ##
     @@ git-extra/vimrc: if has("autocmd")
            autocmd Filetype diff

As described on the Git mailing list [1] and elsewhere [2], Git adjusts PATH before invoking programs, and those adjustments are inherited by editors if they are invoked as subprocesses. In most cases this is harmless, but it can create confusion when spawning subprocesses from the editor (as is common in Vim) and when the user's shell looks at PATH or the location of a "git" binary. Include portable code to strip these entries from PATH on startup. [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/ Signed-off-by: D. Ben Knoble <[email protected]>

benknoble · 2025-05-14T00:52:31Z

range-diff: and the pkgver? took this from CI; I'm probably missing the "workflow" here

1:  f8b44295 ! 1:  1bcec9ae git-extra(vimrc): adjust $PATH to remove Git's libexec
    @@ Commit message
         Signed-off-by: D. Ben Knoble <[email protected]>
     
      ## git-extra/PKGBUILD ##
    +@@ git-extra/PKGBUILD: pkgbase="mingw-w64-${_realname}"
    + pkgname=($_realname
    +         "${MINGW_PACKAGE_PREFIX}-${_realname}")
    + _ver_base=1.1
    +-pkgver=1.1.653.48e2403b3
    ++pkgver=1.1.654.aca5df34e
    + pkgrel=1
    + pkgdesc="Git for Windows extra files"
    + arch=('any')
     @@ git-extra/PKGBUILD: source=('inputrc'
              'git-askpass.h'
              'git-askpass.rc')

Konfekt · 2025-05-14T03:22:24Z

On a general side note, is an augroup for an VimEnter autocmd needed ? Though wrapping autocmd into augroups is considered best practice, this is meant to prevent double loading in the same Vim instance, but then VimEnter is only fired once anyway ?

dscho

Thank you for contributing!

I was a bit on the fence after I read the PR title (which didn't manage to convince me that this is a good change, it rather did the opposite: why on Earth should it be vim's job to clean up the PATH?), but can see that this might make sense.

Since I am unfamiliar with vim's arcane script language, the code in question is too hard to understand for me, and additional code comments would be much appreciated. For example, I wonder whether this code now adds a start-up penalty to all invocations of vim and not just those where GIT_EXEC_PATH is set.

Also, I am still uncertain that this fix is in the right layer: It works around when calling vim as Git editor, but only in Git for Windows! What about Linux, macOS? What about other editors?

In essence, I am beginning to suspect that the most appropriate place to fix the underlying issue is launch_specified_editor() instead. There's a slight complication there in that it already takes an env parameter and it is not a strvec but a fixed, constant array (but then, in all the callers, there is only one that actually passes a non-NULL env parameter, and it comically constructs a strvec already).

Of course, the most versatile way to implement this would not even be in launch_specified_editor(), but instead only set a (to-be-introduced) flag in the child_process struct (something like no_git_exec_path) and then handle that flag at the beginning of start_command(), something along these lines:

if (cmd->no_git_exec_path) {
  struct strbuf buf = STRBUF_INIT;
  const char *exec_path = git_exec_path();
  size_t len = strlen(exec_path);
  char *p;

  strbuf_addstr(&buf, getenv("PATH"));
  for (p = strstr(buf.buf, exec_path); p; p = strstr(p, exec_path)) {
    if ((p[len] && p[len] != PATH_SEP) || (p != buf.buf && p[-1] != PATH_SEP))
      p += len; /* false positive, skip */
    else {
      size_t offset = p - buf.buf, l = len;
      if (p != buf.buf) {
        /* include the preceding path separator */
        offset--;
        l++;
      } else if (p[len] == PATH_SEP) {
        /* include the path separator following GIT_EXEC_PATH */
        l++;
      }
      strbuf_splice(&buf, offset, l, "", 0);
    }
    /* now use this as `PATH` value in the child process' env */
  }
}

Of course, the devil lies in the details and it may well be better to integrate this logic into prep_childenv() instead (passing through the no_git_exec_path parameter).

benknoble · 2025-05-20T15:14:58Z

Thank you for contributing!

Thank you for your detailed reply!

I was a bit on the fence after I read the PR title (which didn't manage to convince me that this is a good change, it rather did the opposite: why on Earth should it be vim's job to clean up the PATH?), but can see that this might make sense.

Right: I wasn't sure this belonged here, anyway, but my old mail never generated any conversation :)

Since I am unfamiliar with vim's arcane script language, the code in question is too hard to understand for me, and additional code comments would be much appreciated. For example, I wonder whether this code now adds a start-up penalty to all invocations of vim and not just those where GIT_EXEC_PATH is set.

I can further explain, but based on below will try to upstream a different change first.

To answer the performance question: we take a minor hit when queuing and executing the callback, but we bail out of that as fast as we can when we don't find the exec path in PATH. If we can rely on GIT_EXEC_PATH to know when to adjust PATH (and even which value to adjust), we can probably do even better.

As a side note, you can profile with --startuptime <output> to Vim: my entire 600-line vimrc takes 23ms to execute. Unfortunately, I don't think VimEnter commands are included in this profile. Yet something like

vim --cmd 'profile start myprof' --cmd 'profile func fix_git_path'
# press "ZZ" once Vim opens
view myprof

does work to profile the function execution: when not launched by Git, this function executes for 0.000042000s (0.042ms = 42μs).

Also, I am still uncertain that this fix is in the right layer: It works around when calling vim as Git editor, but only in Git for Windows! What about Linux, macOS? What about other editors?

In essence, I am beginning to suspect that the most appropriate place to fix the underlying issue is launch_specified_editor() instead. There's a slight complication there in that it already takes an env parameter and it is not a strvec but a fixed, constant array (but then, in all the callers, there is only one that actually passes a non-NULL env parameter, and it comically constructs a strvec already).

Of course, the most versatile way to implement this would not even be in launch_specified_editor(), but instead only set a (to-be-introduced) flag in the child_process struct (something like no_git_exec_path) and then handle that flag at the beginning of start_command(), something along these lines:
if (cmd->no_git_exec_path) {
  struct strbuf buf = STRBUF_INIT;
  const char *exec_path = git_exec_path();
  size_t len = strlen(exec_path);
  char *p;

  strbuf_addstr(&buf, getenv("PATH"));
  for (p = strstr(buf.buf, exec_path); p; p = strstr(p, exec_path)) {
    if ((p[len] && p[len] != PATH_SEP) || (p != buf.buf && p[-1] != PATH_SEP))
      p += len; /* false positive, skip */
    else {
      size_t offset = p - buf.buf, l = len;
      if (p != buf.buf) {
        /* include the preceding path separator */
        offset--;
        l++;
      } else if (p[len] == PATH_SEP) {
        /* include the path separator following GIT_EXEC_PATH */
        l++;
      }
      strbuf_splice(&buf, offset, l, "", 0);
    }
    /* now use this as `PATH` value in the child process' env */
  }
}
Of course, the devil lies in the details and it may well be better to integrate this logic into prep_childenv() instead (passing through the no_git_exec_path parameter).

Aha! Now we're cooking ;) I'll take a look at upstreaming this.

benknoble · 2025-05-20T19:37:06Z

upstreaming

v1: https://lore.kernel.org/git/[email protected]/T/#m3c581e1e9fee9b9390c4c49e41255a7b206f2c69

benknoble mentioned this pull request May 13, 2025

git-core path benknoble/Dotfiles#143

Closed

benknoble force-pushed the vim-adjust-PATH branch from 2b4b5cf to f8b4429 Compare May 14, 2025 00:43

benknoble force-pushed the vim-adjust-PATH branch from f8b4429 to 1bcec9a Compare May 14, 2025 00:52

dscho reviewed May 14, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

git-extra(vimrc): adjust $PATH to remove Git's libexec #616

git-extra(vimrc): adjust $PATH to remove Git's libexec #616

Uh oh!

benknoble commented May 13, 2025

Uh oh!

benknoble commented May 14, 2025

Uh oh!

benknoble commented May 14, 2025

Uh oh!

Konfekt commented May 14, 2025

Uh oh!

dscho left a comment

Uh oh!

benknoble commented May 20, 2025

Uh oh!

benknoble commented May 20, 2025

Uh oh!

Uh oh!

git-extra(vimrc): adjust $PATH to remove Git's libexec #616

Are you sure you want to change the base?

git-extra(vimrc): adjust $PATH to remove Git's libexec #616

Uh oh!

Conversation

benknoble commented May 13, 2025

Uh oh!

benknoble commented May 14, 2025

Uh oh!

benknoble commented May 14, 2025

Uh oh!

Konfekt commented May 14, 2025

Uh oh!

dscho left a comment

Choose a reason for hiding this comment

Uh oh!

benknoble commented May 20, 2025

Uh oh!

benknoble commented May 20, 2025

Uh oh!

Uh oh!