Skip to content

fix(history): don't panic when a listing filters down to nothing - #634

Open
NgoQuocViet2001 wants to merge 1 commit into
kamiyaa:mainfrom
NgoQuocViet2001:fix-empty-listing-index-panic
Open

fix(history): don't panic when a listing filters down to nothing#634
NgoQuocViet2001 wants to merge 1 commit into
kamiyaa:mainfrom
NgoQuocViet2001:fix-empty-listing-index-panic

Conversation

@NgoQuocViet2001

Copy link
Copy Markdown

Likely the crash behind #614 — the reports there are exit code 101 after toggle_hidden, and a commenter narrowed it to dotfiles being present.

The panic

create_dirlist_with_history clamps every index carried over from the previous listing into the new one. The cursor index and the viewport index both check contents_len == 0 first. The visual-mode anchor does not:

let visual_mode_anchor_index = history.get(path).and_then(|dirlist| {
    dirlist.get_visual_mode_anchor_index().map(|old| {
        if old < contents_len { old } else { contents_len - 1 }   // 0usize - 1
    })
});

A directory whose entries are all filtered out gives contents_len == 0, and the subtraction overflows. Toggling hidden files off over a directory holding only dotfiles, with visual mode on, is enough to reach it:

thread 'history::tests::a_listing_that_filters_to_empty_keeps_its_visual_anchor_in_range'
panicked at src/history.rs:117:21:
attempt to subtract with overflow

An empty listing has nowhere for an anchor to point, so it returns None.

The index read next to it

The cursor branch reads &dirlist.contents[i], where i is the previous listing's stored index. Nothing keeps that index in step with its own contents — set_index takes an Option<usize> and does not bound it — so a stale one indexes out of range on a listing that is otherwise fine.

It now reads through .get(i). The entry is only used to look the old file name up in the new contents, and when that lookup is unavailable i is still returned, exactly as before — it has already passed the i >= contents_len arm above. No behaviour changes for an index that is in range.

Test plan

  • Ran: cargo test — 43 passed, 0 failed (41 pre-existing + 2 new).
  • Checked: the first new test panics on the parent commit with the attempt to subtract with overflow shown above, and passes with the change.
  • The second builds a history entry whose stored index points past its own (empty) contents and asserts the rebuild still resolves the cursor rather than panicking.
  • Built and tested on Linux (Ubuntu 24.04, cargo test); the allmytoes dependency does not compile on Windows.

Note on #614

I could not reproduce the reporter's exact keystroke sequence, so I would not close #614 on this alone — but this is a real panic on the same code path, with the same exit code, and reached by the same command. Happy to keep digging if it survives.

create_dirlist_with_history clamps every index carried over from the
previous listing into the new one, but the visual-mode anchor did it with
`contents_len - 1` and nothing guarded contents_len == 0. A directory whose
entries are all filtered out reaches exactly that, and usize subtraction
overflows:

  thread '...' panicked at src/history.rs:117:21:
  attempt to subtract with overflow

Toggling hidden files off over a directory that holds only dotfiles, with
visual mode on, is enough. An empty listing has no anchor, so return None.

The cursor index in the same block read `dirlist.contents[i]`, where `i` is
the previous listing's stored index. Nothing keeps that index in step with
its own contents — set_index does not bound it — so a stale one indexes out
of range. Read it with .get() instead; the name lookup it feeds is only a
hint, and without it `i` still lands in range, having passed the arm above.
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.

toggle_hidden after "h" crashes the app

1 participant