Skip to content

Cache hook state to skip redundant pyenv sh-activate calls #523

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jakelodwick wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Cache hook state to skip redundant pyenv sh-activate calls #523

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
bb9a634
Cache prompt hook state to skip redundant pyenv sh-activate calls
jakelodwick Mar 14, 2026
1266527
Revise hook cache: stat-based mtime comparison
jakelodwick Mar 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
54 changes: 54 additions & 0 deletions bin/pyenv-virtualenv-init
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,13 @@
set -e
[ -n "$PYENV_DEBUG" ] && set -x

Copy link
Member

@native-api native-api Mar 22, 2026

Choose a reason for hiding this comment

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

As my analysis in the summary shows, we'll have to allow for pyenv version-name hooks in the cache check.
I reckon it's reasonable to check for hooks presence only once during init and thus require the user to restart their sessions if they make drastic changes to their Pyenv installation like installing a new plugin.

# Detect stat format for mtime: GNU uses -c %Y, BSD uses -f %m
if stat -c %Y / >/dev/null 2>&1; then
_stat_fmt="-c %Y"
else
_stat_fmt="-f %m"
fi

Copy link
Member

@native-api native-api Mar 22, 2026

Choose a reason for hiding this comment

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

Quite an elegant solution! The comment is very on point, too! (conveys information required for the task that is not in the code itself)

resolve_link() {
$(type -p greadlink readlink | head -1) "$1"
}
Expand Down Expand Up @@ -106,11 +113,35 @@ fish )
cat <<EOS
function _pyenv_virtualenv_hook --on-event fish_prompt;
set -l ret \$status
if test -n "\$PYENV_VERSION" \\
-a "\$PYENV_VERSION" = "\$_PYENV_VH_VERSION" \\
-a "\$VIRTUAL_ENV" = "\$_PYENV_VH_VENV"
return \$ret
end
set -l d "\$PWD"
set -l paths
while true
set paths \$paths "\$d" "\$d/.python-version"
test "\$d" = "/"; and break
set d (string replace -r '/[^/]*\$' '' -- "\$d")
test -z "\$d"; and set d "/"
end
set paths \$paths "\$PYENV_ROOT/version"
if test "\$PWD" = "\$_PYENV_VH_PWD" \\
-a "\$PYENV_VERSION" = "\$_PYENV_VH_VERSION" \\
-a "\$VIRTUAL_ENV" = "\$_PYENV_VH_VENV" \\
-a "(stat ${_stat_fmt} \$paths 2>/dev/null)" = "\$_PYENV_VH_MTIMES"
return \$ret
end
if [ -n "\$VIRTUAL_ENV" ]
pyenv activate --quiet; or pyenv deactivate --quiet; or true
else
pyenv activate --quiet; or true
end
set -g _PYENV_VH_PWD "\$PWD"
set -g _PYENV_VH_VERSION "\$PYENV_VERSION"
set -g _PYENV_VH_VENV "\$VIRTUAL_ENV"
set -g _PYENV_VH_MTIMES (stat ${_stat_fmt} \$paths 2>/dev/null)
return \$ret
end
EOS
Expand All @@ -130,11 +161,34 @@ esac
if [[ "$shell" != "fish" ]]; then
cat <<EOS
local ret=\$?
if [ -n "\${PYENV_VERSION-}" ] \\
&& [ "\${PYENV_VERSION-}" = "\${_PYENV_VH_VERSION-}" ] \\
&& [ "\${VIRTUAL_ENV-}" = "\${_PYENV_VH_VENV-}" ]; then
return \$ret
fi
local _pvh_d="\${PWD}" _pvh_paths=""
while :; do
_pvh_paths="\${_pvh_paths} \${_pvh_d} \${_pvh_d}/.python-version"
[ "\${_pvh_d}" = "/" ] && break
_pvh_d="\${_pvh_d%/*}"
[ -z "\${_pvh_d}" ] && _pvh_d="/"
Comment on lines +171 to +174
Copy link
Member

@native-api native-api Mar 21, 2026

Choose a reason for hiding this comment

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

Do we need to construct _pvh_paths every time?
AFAICT it's enough to construct it once when saving the cache.

Besides, it's also sufficient to:

  • only continue up until the first .python-version found
    • but if there's a broken symlink named ".python-version" found on the way, it should be included, too: pyenv version will ignore it, but if the link's target is later created, it'll magically "appear" without the directory's mtime changing)
  • directories with a .python-version (including the broken symlinks) don't need to be checked: we check directories to catch that file being created later.

This will stat fewer entities every time and not unnecessarily reset the cache from entities that do not matter -- but constructing the list initially would take somewhat longer. Not sure if the economy is worth it.

done
_pvh_paths="\${_pvh_paths} \${PYENV_ROOT}/version"
Copy link
Member

@native-api native-api Mar 22, 2026

Choose a reason for hiding this comment

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

Like the above, the global file only needs checking if no local version is active. Same possible caveats apply.

if [ "\${PWD}" = "\${_PYENV_VH_PWD-}" ] \\
&& [ "\${PYENV_VERSION-}" = "\${_PYENV_VH_VERSION-}" ] \\
&& [ "\${VIRTUAL_ENV-}" = "\${_PYENV_VH_VENV-}" ] \\
Comment on lines +178 to +179
Copy link
Member

@native-api native-api Mar 21, 2026

Choose a reason for hiding this comment

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

We already checked PYENV_VERSION and VIRTUAL_ENV earlier

&& [ "\$(stat ${_stat_fmt} \${_pvh_paths} 2>/dev/null)" = "\${_PYENV_VH_MTIMES-}" ]; then
Copy link
Member

@native-api native-api Mar 22, 2026

Choose a reason for hiding this comment

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

  1. Should probably stat -L: if any of the dirs or .pyenv-version is a symlink, its mtime won't change if the target's contents change.
  • Alternatively, we can resolve links beforehand when composing the list -- but then we'd need to check both the link and the target 'cuz either can change. With -L, we assume that if a link changes, it's extremely unlikely that the new target will have exactly the same mtime as the old one.
  1. Now that we unpack _pvh_paths into an arglist, it shows that it has to be an array -- or paths with spaces will break the logic (e.g. I've seen spaces in home directory names in user reports here). Kinda sad 'cuz arrays can't be exported for pyenv version to potentially use (barring serialization) -- but that's not a present concern.

return \$ret
fi
if [ -n "\${VIRTUAL_ENV-}" ]; then
eval "\$(pyenv sh-activate --quiet || pyenv sh-deactivate --quiet || true)" || true
else
eval "\$(pyenv sh-activate --quiet || true)" || true
fi
_PYENV_VH_PWD="\${PWD}"
_PYENV_VH_VERSION="\${PYENV_VERSION-}"
_PYENV_VH_VENV="\${VIRTUAL_ENV-}"
_PYENV_VH_MTIMES="\$(stat ${_stat_fmt} \${_pvh_paths} 2>/dev/null)"
return \$ret
};
EOS
Expand Down
70 changes: 70 additions & 0 deletions test/init.bats
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,11 +54,34 @@ export PATH="${TMP}/pyenv/plugins/pyenv-virtualenv/shims:\${PATH}";
export PYENV_VIRTUALENV_INIT=1;
_pyenv_virtualenv_hook() {
local ret=\$?
if [ -n "\${PYENV_VERSION-}" ] \\
&& [ "\${PYENV_VERSION-}" = "\${_PYENV_VH_VERSION-}" ] \\
&& [ "\${VIRTUAL_ENV-}" = "\${_PYENV_VH_VENV-}" ]; then
return \$ret
fi
local _pvh_d="\${PWD}" _pvh_paths=""
while :; do
_pvh_paths="\${_pvh_paths} \${_pvh_d} \${_pvh_d}/.python-version"
[ "\${_pvh_d}" = "/" ] && break
_pvh_d="\${_pvh_d%/*}"
[ -z "\${_pvh_d}" ] && _pvh_d="/"
done
_pvh_paths="\${_pvh_paths} \${PYENV_ROOT}/version"
if [ "\${PWD}" = "\${_PYENV_VH_PWD-}" ] \\
&& [ "\${PYENV_VERSION-}" = "\${_PYENV_VH_VERSION-}" ] \\
&& [ "\${VIRTUAL_ENV-}" = "\${_PYENV_VH_VENV-}" ] \\
&& [ "\$(stat ${_stat_fmt} \${_pvh_paths} 2>/dev/null)" = "\${_PYENV_VH_MTIMES-}" ]; then
return \$ret
fi
if [ -n "\${VIRTUAL_ENV-}" ]; then
eval "\$(pyenv sh-activate --quiet || pyenv sh-deactivate --quiet || true)" || true
else
eval "\$(pyenv sh-activate --quiet || true)" || true
fi
_PYENV_VH_PWD="\${PWD}"
_PYENV_VH_VERSION="\${PYENV_VERSION-}"
_PYENV_VH_VENV="\${VIRTUAL_ENV-}"
_PYENV_VH_MTIMES="\$(stat ${_stat_fmt} \${_pvh_paths} 2>/dev/null)"
return \$ret
};
if ! [[ "\${PROMPT_COMMAND-}" =~ _pyenv_virtualenv_hook ]]; then
Expand All @@ -78,11 +101,35 @@ set -gx PATH '${TMP}/pyenv/plugins/pyenv-virtualenv/shims' \$PATH;
set -gx PYENV_VIRTUALENV_INIT 1;
function _pyenv_virtualenv_hook --on-event fish_prompt;
set -l ret \$status
if test -n "\$PYENV_VERSION" \\
-a "\$PYENV_VERSION" = "\$_PYENV_VH_VERSION" \\
-a "\$VIRTUAL_ENV" = "\$_PYENV_VH_VENV"
return \$ret
end
set -l d "\$PWD"
set -l paths
while true
set paths \$paths "\$d" "\$d/.python-version"
test "\$d" = "/"; and break
set d (string replace -r '/[^/]*\$' '' -- "\$d")
test -z "\$d"; and set d "/"
end
set paths \$paths "\$PYENV_ROOT/version"
if test "\$PWD" = "\$_PYENV_VH_PWD" \\
-a "\$PYENV_VERSION" = "\$_PYENV_VH_VERSION" \\
-a "\$VIRTUAL_ENV" = "\$_PYENV_VH_VENV" \\
-a "(stat ${_stat_fmt} \$paths 2>/dev/null)" = "\$_PYENV_VH_MTIMES"
return \$ret
end
if [ -n "\$VIRTUAL_ENV" ]
pyenv activate --quiet; or pyenv deactivate --quiet; or true
else
pyenv activate --quiet; or true
end
set -g _PYENV_VH_PWD "\$PWD"
set -g _PYENV_VH_VERSION "\$PYENV_VERSION"
set -g _PYENV_VH_VENV "\$VIRTUAL_ENV"
set -g _PYENV_VH_MTIMES (stat ${_stat_fmt} \$paths 2>/dev/null)
return \$ret
end
EOS
Expand All @@ -97,11 +144,34 @@ export PATH="${TMP}/pyenv/plugins/pyenv-virtualenv/shims:\${PATH}";
export PYENV_VIRTUALENV_INIT=1;
_pyenv_virtualenv_hook() {
local ret=\$?
if [ -n "\${PYENV_VERSION-}" ] \\
&& [ "\${PYENV_VERSION-}" = "\${_PYENV_VH_VERSION-}" ] \\
&& [ "\${VIRTUAL_ENV-}" = "\${_PYENV_VH_VENV-}" ]; then
return \$ret
fi
local _pvh_d="\${PWD}" _pvh_paths=""
while :; do
_pvh_paths="\${_pvh_paths} \${_pvh_d} \${_pvh_d}/.python-version"
[ "\${_pvh_d}" = "/" ] && break
_pvh_d="\${_pvh_d%/*}"
[ -z "\${_pvh_d}" ] && _pvh_d="/"
done
_pvh_paths="\${_pvh_paths} \${PYENV_ROOT}/version"
if [ "\${PWD}" = "\${_PYENV_VH_PWD-}" ] \\
&& [ "\${PYENV_VERSION-}" = "\${_PYENV_VH_VERSION-}" ] \\
&& [ "\${VIRTUAL_ENV-}" = "\${_PYENV_VH_VENV-}" ] \\
&& [ "\$(stat ${_stat_fmt} \${_pvh_paths} 2>/dev/null)" = "\${_PYENV_VH_MTIMES-}" ]; then
return \$ret
fi
if [ -n "\${VIRTUAL_ENV-}" ]; then
eval "\$(pyenv sh-activate --quiet || pyenv sh-deactivate --quiet || true)" || true
else
eval "\$(pyenv sh-activate --quiet || true)" || true
fi
_PYENV_VH_PWD="\${PWD}"
_PYENV_VH_VERSION="\${PYENV_VERSION-}"
_PYENV_VH_VENV="\${VIRTUAL_ENV-}"
_PYENV_VH_MTIMES="\$(stat ${_stat_fmt} \${_pvh_paths} 2>/dev/null)"
return \$ret
};
typeset -g -a precmd_functions
Expand Down
7 changes: 7 additions & 0 deletions test/test_helper.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
export TMP="$BATS_TEST_DIRNAME/tmp"

# Detect stat format for mtime: GNU uses -c %Y, BSD uses -f %m
if stat -c %Y / >/dev/null 2>&1; then
_stat_fmt="-c %Y"
else
_stat_fmt="-f %m"
fi
export PS4='+(${BASH_SOURCE}:${LINENO}): ${FUNCNAME[0]:+${FUNCNAME[0]}(): }'

PATH=/usr/bin:/usr/sbin:/bin:/sbin
Expand Down
Loading