fix(purge): restore cursor and temp files on any exit path (#915)#928
Merged
Conversation
mo purge hid the cursor before perform_purge and restored it via a single trailing show_cursor. Any abort path (set -e, uncaught error, unhandled signal) skipped that line, leaving the terminal with the cursor invisible until the user ran tput cnorm. The in-main INT/TERM trap also overwrote the file-scope cleanup_temp_files trap, so a Ctrl-C during arg parsing would have leaked temp files. Fold show_cursor into the file-scope cleanup function alongside cleanup_temp_files, and reinstall the trap chain in the installer.sh shape (EXIT plus an INT/TERM trap that explicitly calls cleanup before exit 130). The EXIT trap is the catch-all, so every exit path — normal, set -e abort, or signal — restores the cursor and clears temp files exactly once.
Owner
|
@sebastianbreguel Thanks for the careful fix. Merged into main; this covers #915 and also keeps temp cleanup intact on exit paths. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #915 (
mo purgeleaves the terminal cursor hidden after running).bin/purge.shhid the cursor beforeperform_purgeand restored it via a single trailingshow_cursor. Underset -euo pipefail, any abort path other thanINT/TERM(e.g. an uncaught error insideperform_purge, a non-zero return,SIGHUPfrom closing the terminal) skipped that line, so the terminal stayed with the cursor invisible until the user rantput cnormthemselves.While fixing that I noticed the
main()-scopedtrap 'show_cursor; exit 130' INT TERMoverwrote the existing file-scopetrap cleanup_temp_files EXIT INT TERM. So a Ctrl-C during arg parsing (beforeperform_purgereinstalled its own trap) would also leak temp files — addressed as part of the same change.Fix
Fold
show_cursorinto the file-scopecleanupfunction alongsidecleanup_temp_files, and reinstall the traps in the same shapebin/installer.shalready uses (trap cleanup EXIT+ anINT/TERMtrap that explicitly callscleanupbeforeexit 130). TheEXITtrap is the catch-all, so every exit path — normal,set -eabort, or signal — runs cleanup exactly once.Diff is +7 / −6 in a single file.
…and the corresponding deletions inside
main()(the redundant in-main()INT/TERM trap and the trailingshow_cursor).Why mirror
installer.shrather thanclean.shThe repo has three cleanup shapes today (
installer.sh,clean.sh,uninstall.sh).installer.shis the closest cousin topurge.sh— interactive UI, cursor hide/show, temp files, no need to pass the signal name to cleanup. Matching it keeps the variance between files lower.Test plan
shellcheck bin/purge.sh— cleanbats tests/purge.bats tests/purge_config_paths.bats— 76/76 passbash bin/purge.sh --help— parsesperform_purge→handle_interruptrunsshow_cursorandexit 130→exitfires the file-scopeEXITtrap →cleanupcallsshow_cursor(idempotent) andcleanup_temp_files. Both windows (before and duringperform_purge) are covered.Not in scope
bin/clean.shandbin/uninstall.shalready wrapshow_cursorinside a file-scopecleanup, so they are not affected by this bug. Keeping this PR narrow to #915.