fix: implement history -n and history -r#1136
Draft
melderan wants to merge 2 commits into
Draft
Conversation
`history -n` and `history -r` previously returned "not yet implemented"
errors. JMO's shell config calls `history -n` in `__history_sync` on
every prompt cycle, causing a stderr error on every prompt.
Changes:
brush-core/src/history.rs:
- Add `last_file_read_count: usize` field to `History` (serde-skipped so
it resets to 0 across any serialization boundary -- safe, as a
subsequent -n will just re-read all new entries from that baseline).
`import` sets it to `self.count()` after loading so that a fresh
session's -n call correctly skips already-loaded entries.
- Extract `parse_and_append_lines` from `import`. Items are always
constructed via direct `Item { dirty: false, ... }` struct literal
(never `Item::new()`, which sets dirty: true) because file-loaded
entries are already persisted.
- Add `read_new_from_file` (history -n): re-parses the full file into a
scratch History, then appends only the entries beyond
`last_file_read_count`. Truncation recovery: if
`scratch.count() < last_file_read_count`, the file was truncated
externally -- reset the baseline to `scratch.count()` without
appending so that future appends to the file remain visible.
Refuses files > 1 GiB (matches `load_history` guard).
- Add `reload_from_file` (history -r): appends the full file content to
the current in-memory history (bash -r semantics: append, not replace).
Sets `last_file_read_count = scratch.count()` so subsequent -n calls
know where to start.
- Update `clear` to also reset `last_file_read_count = 0`. NOTE: `next_id`
is intentionally not reset -- item IDs must remain monotonically
increasing across a clear/reload cycle for reedline stability.
brush-builtins/src/history.rs:
- Wire `-n` to `history.read_new_from_file(file_path)?` using the
existing `get_effective_history_file_path` helper (supports optional
explicit path, falls back to HISTFILE).
- Wire `-r` to `history.reload_from_file(file_path)?` with same pattern.
- Upgrade `-p` stub from `error::unimp(...)` to
`error::unimp_with_issue("history -p is not yet implemented", 100)`.
Issue reubeno#100 confirmed: "Implement history expansion (word designators,
event designators)" in reubeno/brush -- exactly the right tracker.
Reedline clone investigation (plan step 4): `brush-interactive/src/reedline/history.rs`
accesses `History` only through `MutexGuard` borrows (`get_shell_history` /
`get_shell_history_mut`). No `History::clone()` calls observed. The `Clone`
derive exists on the struct but is not exercised by the reedline integration.
`last_file_read_count` is therefore not reset mid-session by the reedline
layer.
Tests:
- 8 new compat test cases in brush-shell/tests/cases/compat/builtins/history.yaml.
All use `skip_oracle: true` + `expected_stdout`: bash non-interactive mode does
not load HISTFILE at startup (brush does), and bash records fewer stdin commands
in history in the test harness. Oracle comparison would fail on incidental
differences unrelated to the feature being tested.
- 2 new unit tests in brush-builtins/src/history.rs:
`test_parse_dash_n` and `test_parse_dash_r`, mirroring `test_parse_dash_a`.
Pre-commit gate: cargo xtask ci pre-commit passes. The sole failure
(run_in_bg_then_fg) is the pre-existing flake that also fails on main.
Assisted-by: Claude (Anthropic) (JMO's friend)
The 1 GiB history-file size guard was duplicated as a function-local const in three places: `load_history`, `read_new_from_file`, and `reload_from_file`. Lift to a single `pub(crate) const` at the top of `brush-core/src/history.rs` so the guard stays in lockstep across all import paths if the threshold is ever revisited. No behavior change. Assisted-by: Claude (Anthropic) (JMO's friend)
reubeno
reviewed
May 5, 2026
reubeno
left a comment
reubeno
left a comment
Owner
There was a problem hiding this comment.
Thanks for tackling these options!
I've left a few initial questions / comments.
|
|
||
| # Tests for history -r and history -n. | ||
| # | ||
| # These tests use skip_oracle + expected_stdout rather than comparing against bash's oracle, |
Owner
There was a problem hiding this comment.
I'm not sure I follow why the oracle testing approach can't be used? The other tests in this file are able to compare history loading/manipulation against bash's behavior.
Are you suggesting that there's something unique about the options targeted by this PR?
| /// Serde-skipped: not persisted across serialization boundaries; resets to 0 on | ||
| /// deserialization (safe — subsequent `-n` reads everything new from the file). | ||
| #[cfg_attr(feature = "serde", serde(skip))] | ||
| last_file_read_count: usize, |
Owner
There was a problem hiding this comment.
Is this captured independently of which file was last read? Do we have any new test cases that check behavior when subsequent invocations use different history file paths (or the history file has since been changed underneath us)?
| next_id: ItemId, | ||
| /// Tracks how many items were present in the history file the last time we read it. | ||
| /// Used by `read_new_from_file` (`history -n`) to skip already-loaded entries. | ||
| /// Serde-skipped: not persisted across serialization boundaries; resets to 0 on |
Owner
There was a problem hiding this comment.
What's the thinking of not persisting it? Is your concern that the history file has been changed in the intervening time since serialization?
| /// # Arguments | ||
| /// | ||
| /// * `path` - The path to the history file. | ||
| pub fn read_new_from_file(&mut self, path: impl AsRef<Path>) -> Result<(), error::Error> { |
Owner
There was a problem hiding this comment.
This and the function below are quite similar; any way to share logic to ensure they don't drift from each other?
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
2 participants
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
Implements
history -nandhistory -r, which previously returnederror: not yet implemented. Also retags thehistory -pstub viaunimp_with_issue(..., 100)so anyone hitting it lands on #100 (history expansion) directly.The motivating case: a
__history_syncfunction inPROMPT_COMMAND(a common pattern for keeping multi-terminal history coherent) callshistory -a(works) andhistory -n(errors today). With this fix, those configurations stop spamming stderr on every prompt cycle.Closes #1134.
Behavior — matches bash
history -n [filename]: reads all entries from the file that haven't already been read into this session, appending them to the in-memory history. Tracks a per-session "last read offset" via a newlast_file_read_countfield onHistory.history -r [filename]: appends the full file contents to the in-memory history. Note: bash's man page says "use them as the current history," but the actual bash implementation appends rather than replaces — verified empirically withbash --norc --noprofile -c 'history -s in-mem; history -r src.txt; history'. The tests follow the empirical behavior.$HISTFILE.-ncalls (log rotation, another session doinghistory -w),last_file_read_countis reset to the new (smaller) count so subsequent appends remain visible.-nand-r, matching the existingload_historybehavior. While we were there, the constant was lifted to a singlepub(crate) constso the threshold stays in lockstep across the three import paths.Design notes worth flagging
last_file_read_countisserde(skip): it's session-local state. Resetting to 0 across serialization boundaries is safe — a subsequent-njust re-reads everything from the file (the conservative outcome).clear()resetslast_file_read_countto 0 (sohistory -c; history -nreads everything from the file, matching bash).next_idis intentionally not reset — comment in code explains why (reedline ID stability across clear/reload cycles).Item { dirty: false, ... }literal, notItem::new()(which setsdirty: true). That ensureshistory -adoesn't double-write entries that came from the file.brush-interactive/src/reedline/history.rsaccessesHistoryexclusively throughMutexGuardborrows; noHistory::clone()calls. TheClonederive on the struct exists but is not exercised by the reedline integration, solast_file_read_countdoesn't get reset mid-session.Tests
brush-shell/tests/cases/compat/builtins/history.yamlcovering: load file, append-vs-replace, explicit file argument, incremental-nreads, idempotence, post-history -cbehavior, and truncation recovery. All useargs: ["-o", "history"]for deterministic non-interactive sequencing.test_parse_dash_n,test_parse_dash_r) mirroring the existingtest_parse_dash_a.The new tests use
skip_oracle: true+expected_stdoutrather than bash-oracle comparison. Reason: brush loads$HISTFILEat startup in non-interactive mode while bash doesn't, which causes oracle divergence on incidental output unrelated to the feature being tested. This is a pre-existing brush↔bash divergence, not introduced by this PR. Happy to switch to oracle comparison if the maintainer prefers; this seemed like the lower-risk choice.Known related gap (not in this PR)
When
HISTFILEis unset and no explicit file argument is given,-a/-w/-n/-rsilently no-op (exit 0). Bash emits a diagnostic to stderr in the same situation (also exit 0). Filed as #1135. Doesn't affect the motivating__history_synccase (whereHISTFILEis set), but worth tracking.Local verification
unset BASH_ENV && cargo xtask ci pre-commit— fmt, build, lint, deps, schemas, unit tests all green; integration tests pass except for the pre-existingbrush-interactive-tests::run_in_bg_then_fgflake unrelated to this change (also fails onmainon this dev machine; the local.bashrcreferencesmise/direnv/atuin/zoxidehook functions that aren't present in CI).brush -i <<< 'history -n; exit'now exits clean instead of printing an error.Two commits, one story
fix: implement history -n and history -r— the actual implementationrefactor: lift MAX_FILE_SIZE_FOR_HISTORY_IMPORT to module scope— small follow-on dedupe of the file-size constant across the three import pathsHappy to squash if the maintainer prefers a single commit.
Filed as a draft for early input.
Assisted-by: Claude (Anthropic) (JMO's friend)