Skip to content

perf(fish): migrate FORGIT_ vars without spawning subprocesses#543

Open
AllanZyne wants to merge 1 commit into
wfxr:mainfrom
AllanZyne:perf/fish-native-var-migration
Open

perf(fish): migrate FORGIT_ vars without spawning subprocesses#543
AllanZyne wants to merge 1 commit into
wfxr:mainfrom
AllanZyne:perf/fish-native-var-migration

Conversation

@AllanZyne

@AllanZyne AllanZyne commented Jul 7, 2026

Copy link
Copy Markdown

What

The backwards-compatibility block in conf.d/forgit.plugin.fish that exports unexported FORGIT_ config variables used a shell pipeline:

set | awk -F ' ' '{ print $1 }' | grep FORGIT_ | while read var
    if not set -x | grep -q "^$var\b"

This forks awk and grep (once for the list, then a grep per variable) on every interactive fish startup. I replaced it with fish's built-in set -n / set -xn + string match, which run in-process:

for var in (set -n | string match -er '^FORGIT_')
    if not set -xn | string match -qr "^$var\$"

Why

Pure startup-speed win — no external processes spawned for this block anymore. On my machine the block's cost dropped from ~2.5ms to ~1.5ms; the real benefit is eliminating the per-variable subprocess fork, which scales with how many FORGIT_ vars a user has set.

Behavior

Unchanged. Verified:

  • Unexported FORGIT_ variables are still detected, warned about, and exported.
  • Nothing is emitted when all FORGIT_ variables are already exported.

The anchored ^FORGIT_ match on set -n output (one name per line) is equivalent to the previous grep FORGIT_, just without the substring ambiguity.

Testing

  • fish conf.d/forgit.plugin.fish — parses clean
  • Manual equivalence tests for both the "all exported" (silent) and "unexported var" (warns + migrates) paths
  • OS: Linux; shell: fish 4.6.0

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved handling of user-defined FORGIT_ environment settings so they are exported more reliably.
    • Streamlined the check for already-exported settings, which should make startup behavior more consistent.

The backwards-compatibility block that exports unexported FORGIT_ config
variables ran `set | awk | grep` plus a `set -x | grep` for every variable,
forking multiple external processes on every interactive fish startup.

Replace the pipeline with fish's built-in `set -n`/`set -xn` and
`string match`, which run in-process. Behavior is unchanged: unexported
FORGIT_ variables are still detected, warned about, and exported; nothing
is emitted when all variables are already exported.

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@coderabbitai

coderabbitai Bot commented Jul 7, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: bfb97f6c-73ab-4e1c-ba20-5ab9586b49ac

📥 Commits

Reviewing files that changed from the base of the PR and between e0c7b84 and f147179.

📒 Files selected for processing (1)
  • conf.d/forgit.plugin.fish

📝 Walkthrough

Walkthrough

The FORGIT_ variable export-detection logic in the Fish plugin config was rewritten to use native Fish built-ins (set -n, set -xn, string match -qr) instead of an external pipeline of set, awk, grep, and while read.

Changes

Variable Export Refactor

Layer / File(s) Summary
Fish-native FORGIT_ variable export check
conf.d/forgit.plugin.fish
Replaced pipeline-based enumeration (set | awk | grep | while read) and export check (set -x | grep -q) with Fish built-ins: set -n filtered by regex ^FORGIT_, and set -xn combined with string match -qr to detect already-exported variables.

Estimated code review effort: 2 (Simple) | ~10 minutes

Compact metadata: Lines changed: +2/-2 | Base: e0c7b84 | Head: f147179

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed It clearly summarizes the fish startup optimization and subprocess removal.
Description check ✅ Passed It covers the change, motivation, behavior, and testing, but omits the template's checklist and type-of-change fields.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

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