fix(history): emit diagnostic when HISTFILE is unset for -a/-w#1146
Open
dashitongzhi wants to merge 5 commits into
Open
fix(history): emit diagnostic when HISTFILE is unset for -a/-w#1146dashitongzhi wants to merge 5 commits into
dashitongzhi wants to merge 5 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
This PR aligns history -a and history -w behavior with bash by emitting a stderr diagnostic when no effective history file can be determined (i.e., HISTFILE unset and no explicit file argument), while preserving the bash-compatible success exit code.
Changes:
- Updated
history -aandhistory -wcode paths to handleNonefromget_effective_history_file_pathby printingHISTFILE: parameter null or not setto stderr. - Refactored the previous
if let Some(...)flow intomatchto make theNonehandling explicit.
Comment on lines
+132
to
+134
| None => { | ||
| writeln!(stderr, "brush: history: HISTFILE: parameter null or not set")?; | ||
| } |
Comment on lines
+120
to
+134
| match get_effective_history_file_path( | ||
| config.default_history_file_path, | ||
| append_option.as_ref(), | ||
| ) { | ||
| history.flush( | ||
| file_path, | ||
| true, /* append? */ | ||
| true, /* unsaved items only */ | ||
| config.time_format.is_some(), /* write timestamps? */ | ||
| )?; | ||
| Some(file_path) => { | ||
| history.flush( | ||
| file_path, | ||
| true, /* append? */ | ||
| true, /* unsaved items only */ | ||
| config.time_format.is_some(), /* write timestamps? */ | ||
| )?; | ||
| } | ||
| None => { | ||
| writeln!(stderr, "brush: history: HISTFILE: parameter null or not set")?; | ||
| } |
Comment on lines
119
to
137
| if let Some(append_option) = &self.append_session_to_file { | ||
| if let Some(file_path) = get_effective_history_file_path( | ||
| match get_effective_history_file_path( | ||
| config.default_history_file_path, | ||
| append_option.as_ref(), | ||
| ) { | ||
| history.flush( | ||
| file_path, | ||
| true, /* append? */ | ||
| true, /* unsaved items only */ | ||
| config.time_format.is_some(), /* write timestamps? */ | ||
| )?; | ||
| Some(file_path) => { | ||
| history.flush( | ||
| file_path, | ||
| true, /* append? */ | ||
| true, /* unsaved items only */ | ||
| config.time_format.is_some(), /* write timestamps? */ | ||
| )?; | ||
| } | ||
| None => { | ||
| writeln!(stderr, "brush: history: HISTFILE: parameter null or not set")?; | ||
| } | ||
| } | ||
|
|
||
| return Ok(ExecutionResult::success()); |
| )?; | ||
| } | ||
| None => { | ||
| writeln!(stderr, "brush: history: HISTFILE: parameter null or not set")?; |
Comment on lines
+149
to
+163
| match get_effective_history_file_path( | ||
| config.default_history_file_path, | ||
| write_option.as_ref(), | ||
| ) { | ||
| history.flush( | ||
| file_path, | ||
| false, /* append? */ | ||
| false, /* unsaved items only? */ | ||
| config.time_format.is_some(), /* write timestamps? */ | ||
| )?; | ||
| Some(file_path) => { | ||
| history.flush( | ||
| file_path, | ||
| false, /* append? */ | ||
| false, /* unsaved items only? */ | ||
| config.time_format.is_some(), /* write timestamps? */ | ||
| )?; | ||
| } | ||
| None => { | ||
| writeln!(stderr, "brush: history: HISTFILE: parameter null or not set")?; | ||
| } |
Owner
|
@dashitongzhi Thanks for the contribution! The change looks relatively straightforward. I've unblocked the PR checks to run and will take another look. |
Performance Benchmark Report
Code Coverage Report: Only Changed Files listed
Minimum allowed coverage is Test Summary: bash-completion test suite
|
reubeno
requested changes
May 18, 2026
reubeno
left a comment
reubeno
left a comment
Owner
There was a problem hiding this comment.
Thanks for the contribution, and for your patience. I left one comment that I'd like you to address; other than that it's looking good.
| None => { | ||
| writeln!( | ||
| stderr, | ||
| "brush: history: HISTFILE: parameter null or not set" |
Owner
There was a problem hiding this comment.
As copilot's auto-comments called out, brush-core is embeddable in differently name shell apps, so we shouldn't hard-code brush.
If you look elsewhere in the code base you can see examples of what other builtins do. (Most of them probably omit the shell name, although the shell name can be dynamically queried via .current_shell_name() on the shell object.)
Could you please update this and the other error below accordingly?
When is unset and no explicit file argument is provided,
and previously silently succeeded with exit 0.
Bash emits a diagnostic in this situation.
This change converts the pattern to for both
(append_session_to_file) and (write_session_to_file) code
paths, emitting the following diagnostic to stderr when the history
file path resolves to None:
brush: history: HISTFILE: parameter null or not set
This matches bash's exact wording for compat-script friendliness.
Fixes reubeno#1135
dashitongzhi
force-pushed
the
fix/history-histfile-diagnostic
branch
from
8a5e0d7 to
90c747b
Compare
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
When
HISTFILEis unset and no explicit file argument is provided,history -aandhistory -wpreviously silently succeeded with exit 0. Bash emits a diagnostic to stderr in the same situation.This PR fixes the issue by emitting:
to stderr when the history file path resolves to
None, matching bash's exact wording for compat-script friendliness.Changes
brush-builtins/src/history.rs: Convertedif let Sometomatchfor both-a(append_session_to_file) and-w(write_session_to_file) code paths, adding aNonearm that writes the diagnostic to stderr.Notes
-nand-ralready return "not yet implemented" errors, so they don't need the HISTFILE check yet.Fixes #1135