perf(vcs_info): apply the %-quoting patch on first use - #14072
robbyrussell wants to merge 4 commits into
Conversation
lib/vcs_info.zsh loaded VCS_INFO_formats and regexp-replace and patched the function body on every startup, although only themes that call vcs_info ever need it. Define a VCS_INFO_formats wrapper that loads and patches the real function the first time it's called, then replaces itself with it. `autoload` doesn't override an existing function, so the wrapper survives a theme's `autoload -Uz vcs_info` and vcs_info's own autoload of VCS_INFO_*. Verified that a branch named `evil%n%m` still renders literally after prompt expansion, as with the eager patch (CVE-2021-45444 mitigation). Measured on macOS arm64, zsh 5.9: 1.6 ms -> 0.1 ms per interactive start. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
🟡 Changes recommended
The wrapper overwrites preloaded or user-defined VCS_INFO_formats implementations.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Defers the vcs_info security patch until first use to reduce shell startup time.
Changes:
- Adds a lazy
VCS_INFO_formatswrapper. - Loads, patches, and replaces the function on first invocation.
File summaries
| File | Description |
|---|---|
lib/vcs_info.zsh |
Implements lazy loading and patching. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # vcs_info. `autoload` doesn't replace an already defined function, so this | ||
| # wrapper survives a later `autoload -Uz vcs_info` in a theme or .zshrc, and | ||
| # vcs_info's own `autoload -Uz VCS_INFO_formats`. | ||
| function VCS_INFO_formats { |
There was a problem hiding this comment.
Fixed in 61f12e0. VCS_INFO_formats is now patched in place when already loaded, and the lazy path only installs/uses the wrapper when it is not defined yet.
Co-authored-by: robbyrussell <257+robbyrussell@users.noreply.github.com>
Co-authored-by: robbyrussell <257+robbyrussell@users.noreply.github.com>
Co-authored-by: robbyrussell <257+robbyrussell@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
The current “already defined” detection doesn’t account for autoload stubs (risking an unpatched real function later) and the already-loaded branch can return a non-zero status when sourcing the file, which may break shells with errexit/ERR_RETURN.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
| # vcs_info. `autoload` doesn't replace an already defined function, so this | ||
| # wrapper survives a later `autoload -Uz vcs_info` in a theme or .zshrc, and | ||
| # vcs_info's own `autoload -Uz VCS_INFO_formats`. | ||
| if (( $+functions[VCS_INFO_formats] )); then |
| () { | ||
| autoload -Uz +X regexp-replace 2>/dev/null || return 1 | ||
|
|
Standards checklist:
Changes:
lib/vcs_info.zshloadedVCS_INFO_formatsandregexp-replaceand patched the function body on every startup, although only themes that callvcs_infoever need it.VCS_INFO_formatswrapper that loads and patches the real function the first time it's called, then replaces itself with it. The patch itself is unchanged.autoloaddoesn't override an existing function, so the wrapper survives a theme'sautoload -Uz vcs_info(before or after Oh My Zsh loads) and vcs_info's ownautoload -Uz VCS_INFO_*.Other comments:
Part of a series of small startup-time PRs. Measured on macOS arm64, zsh 5.9: 1.6 ms to 0.1 ms per interactive start.
Since this is the CVE-2021-45444 mitigation, verified through a full Oh My Zsh startup with
autoload -Uz vcs_infoin the zshrc: the wrapper is in place at startup, the firstvcs_infocall applies the patch and the wrapper is gone afterwards, a branch namedevil%n%mrenders literally after prompt expansion (same as with the eager patch), and re-runningautoload -Uz vcs_infoafterwards keeps the patched function.🤖 Co-authored with Claude Code