Skip to content

Fix fish prompt hook never skipping redundant activate calls - #528

Open
ethanleifer wants to merge 1 commit into
pyenv:masterfrom
ethanleifer:fix-fish-hook-mtime-check
Open

ethanleifer wants to merge 1 commit into
pyenv:masterfrom
ethanleifer:fix-fish-hook-mtime-check

Conversation

@ethanleifer

@ethanleifer ethanleifer commented Oct 1, 2026 •

Copy link
Copy Markdown

In fish, the prompt hook ran pyenv activate --quiet before every prompt. The cache from #523 is supposed to skip that call when nothing has changed, but its fish version never matched.

Cause

The fish hook compared the stored mtimes against a quoted string:

-a "(stat -L -f %m $_PYENV_VH_PATHS 2>/dev/null)" = "$_PYENV_VH_MTIMES"

Fish does not run command substitution inside double quotes, so this compared _PYENV_VH_MTIMES against the literal text (stat -L -f %m ...). The comparison was always false. The bash and zsh hooks use "$(stat ...)", which does run, so only fish was affected.

Fix

Run stat into a local list, then compare it quoted:

if test "$PWD" = "$_PYENV_VH_PWD"
  set -l mtimes (stat -L -f %m $_PYENV_VH_PATHS 2>/dev/null)
  if test "$mtimes" = "$_PYENV_VH_MTIMES"
    return $ret
  end
end

_PYENV_VH_MTIMES is stored as a list from (stat ...). Quoting both lists joins them with spaces the same way, so they compare correctly with one or many paths.

I avoided "$(...)" for two reasons. It needs fish 3.4 or later, and it keeps newlines, so it wouldn't match the stored list. As before, stat only runs when $PWD is unchanged.

Testing

  • The bats suite passes with Bats 1.10.0, as in CI: 111 of 111 on macOS. test/init.bats "outputs fish-specific syntax" is updated for the new output. It fails against the old hook code.
  • I tested the generated hook in fish 4.9.3 with a stub pyenv that counts activate calls:
Step Calls on master Calls with this fix
First prompt 1 1
Second and third prompt in the same directory 3 1
After cd .., then another prompt 5 2
New .python-version in a parent, then another prompt 7 3
Edited .python-version 8 4
Local file removed and global version edited 9 5
PYENV_VERSION set 10 6

With the fix, fish behaves like the bash and zsh hooks. It reruns activate after a directory change, a version file change, or a PYENV_VERSION change, and skips it otherwise. On my machine, each skipped call saves about 250–290 ms per prompt.

🤖 Generated with Claude Code


Summary by cubic

Fixes the fish prompt hook so it no longer runs pyenv activate --quiet before every prompt, restoring the mtime-based deduplication that already worked for bash and zsh.

Bug Fixes

  • Fish does not run command substitution inside double quotes, so the old check always compared _PYENV_VH_MTIMES against the literal text and never matched.
  • stat now runs into a local list and compares quoted, matching how _PYENV_VH_MTIMES is stored, and still only runs when $PWD is unchanged.
  • The expected fish hook output in test/init.bats is updated to match.

Written for commit 820457b. Summary will update on new commits.

Review in cubic

The fish hook compared _PYENV_VH_MTIMES against "(stat ...)". Inside double
quotes fish does not run command substitution, so the comparison was against
literal text and always failed. The hook then ran `pyenv activate --quiet`
before every prompt instead of only when the directory or a version file
changed.

Run stat into a local list and compare it quoted, the same way
_PYENV_VH_MTIMES is stored. stat still only runs when $PWD is unchanged.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

@cubic-dev-ai cubic-dev-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 issue found across 2 files

Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="test/init.bats">

<violation number="1" location="test/init.bats:117">
P1: This comparison still fails whenever `stat` emits more than one line, which is the common case: `_PYENV_VH_PATHS` holds one entry per directory level between PWD and `/` (plus `${PYENV_ROOT}/version`) whenever the `.python-version` is not directly in PWD, and `stat -c %Y`/`-f %m` prints one line per file. `set -l mtimes (stat ...)` then creates a multi-element list, and `"$mtimes" = "$_PYENV_VH_MTIMES"` expands to >3 arguments, so fish `test` fails with "too many arguments" (status 2): the `if` is false, stderr is spammed on every prompt, and `pyenv activate --quiet` still runs on every prompt — the exact bug this PR is meant to fix. Collapse each list to a single string before comparing, e.g. `if test (string join " " $mtimes) = (string join " " $_PYENV_VH_MTIMES)` (mtime elements are pure digits, so the delimiter is safe, and `string` is already a requirement of this generated hook via `string replace`).</violation>
</file>

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread test/init.bats
return \$ret
if test "\$PWD" = "\$_PYENV_VH_PWD"
set -l mtimes (stat ${_stat_fmt} \$_PYENV_VH_PATHS 2>/dev/null)
if test "\$mtimes" = "\$_PYENV_VH_MTIMES"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: This comparison still fails whenever stat emits more than one line, which is the common case: _PYENV_VH_PATHS holds one entry per directory level between PWD and / (plus ${PYENV_ROOT}/version) whenever the .python-version is not directly in PWD, and stat -c %Y/-f %m prints one line per file. set -l mtimes (stat ...) then creates a multi-element list, and "$mtimes" = "$_PYENV_VH_MTIMES" expands to >3 arguments, so fish test fails with "too many arguments" (status 2): the if is false, stderr is spammed on every prompt, and pyenv activate --quiet still runs on every prompt — the exact bug this PR is meant to fix. Collapse each list to a single string before comparing, e.g. if test (string join " " $mtimes) = (string join " " $_PYENV_VH_MTIMES) (mtime elements are pure digits, so the delimiter is safe, and string is already a requirement of this generated hook via string replace).

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. At test/init.bats, line 117:

<comment>This comparison still fails whenever `stat` emits more than one line, which is the common case: `_PYENV_VH_PATHS` holds one entry per directory level between PWD and `/` (plus `${PYENV_ROOT}/version`) whenever the `.python-version` is not directly in PWD, and `stat -c %Y`/`-f %m` prints one line per file. `set -l mtimes (stat ...)` then creates a multi-element list, and `"$mtimes" = "$_PYENV_VH_MTIMES"` expands to >3 arguments, so fish `test` fails with "too many arguments" (status 2): the `if` is false, stderr is spammed on every prompt, and `pyenv activate --quiet` still runs on every prompt — the exact bug this PR is meant to fix. Collapse each list to a single string before comparing, e.g. `if test (string join " " $mtimes) = (string join " " $_PYENV_VH_MTIMES)` (mtime elements are pure digits, so the delimiter is safe, and `string` is already a requirement of this generated hook via `string replace`).</comment>

<file context>
@@ -112,9 +112,11 @@ function _pyenv_virtualenv_hook --on-event fish_prompt;
-      return \$ret
+    if test "\$PWD" = "\$_PYENV_VH_PWD"
+      set -l mtimes (stat ${_stat_fmt} \$_PYENV_VH_PATHS 2>/dev/null)
+      if test "\$mtimes" = "\$_PYENV_VH_MTIMES"
+        return \$ret
+      end
</file context>
Suggested change
if test "\$mtimes" = "\$_PYENV_VH_MTIMES"
if test (string join " " $mtimes) = (string join " " $_PYENV_VH_MTIMES)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant