From 0b0d5acd81b6b21a0984a938dac33961e6633d1b Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sat, 25 Jul 2026 11:33:46 -0400 Subject: [PATCH] fix(find-file): review round 1 -- name the real test, pin two gaps Three of the five review findings land here; the other two are recorded as named deferrals in the framing on the dired branch. Finding 1: the command comment cited "acc4", a name from a draft scheme that no test carries. It now names the real test, and the comment splits the shadowing consequence into the two cases that actually exist -- a new bare name that matches an entry (shadowed) versus one that matches nothing (creates normally) -- each pointing at its test. Finding 2: the everyday new-file flow had no test. Typing a bare name that is not a subsequence of any entry is the path users hit first, and the only route combining free text with a relative join; every existing new-file test used a name containing a separator. find_file_bare_new_name_creates_in_the_root covers it, asserting the parent is the prompt's root so the join itself is pinned. Finding 3: the failure arm was never exercised, and as the review noted, deleting the pcall would have passed the whole suite. Accepting a directory candidate reaches display_file, whose load fails because File::open on a directory succeeds and the read returns EISDIR; find_file_accepting_a_directory_reports_instead_of_raising pins that this surfaces as the command's status message, leaves the active buffer alone, and closes the prompt. Verified by manual revert: with the pcall replaced by a direct call, that test and only that test fails. scripts/bite could not isolate it, since the guard and its test have no separating commit. Finding 4 is documented at the command rather than left implicit: accepting on empty input opens the first-sorted candidate, because fuzzy_score returns Some(0) for an empty needle and filter_and_sort breaks the tie lexicographically, so dotfiles lead and a directory can lead. M-x and switch-buffer share the mechanism, so it is inherited rather than introduced, and it is listed in the framing beside the accept-semantics change that would close it. --- builtin/commands/default.lua | 21 ++++++++-- tests/find_file_acceptance.rs | 79 +++++++++++++++++++++++++++++++++++ 2 files changed, 96 insertions(+), 4 deletions(-) diff --git a/builtin/commands/default.lua b/builtin/commands/default.lua index bc04e91..2a13c21 100644 --- a/builtin/commands/default.lua +++ b/builtin/commands/default.lua @@ -637,10 +637,23 @@ cmd { name = "editor.switch-buffer", -- are basenames and the filter is a subsequence match, is exactly -- when the input contains a `/`. That makes the deeper-path case work -- (`sub/inner.txt` matches no basename, so it arrives verbatim) and --- leaves one documented hole: typing a NEW bare name that happens to --- be a subsequence of an existing entry opens the existing file --- instead of creating the new one. `acc4` pins that as a known --- behavior rather than letting it be an accident. +-- leaves TWO documented consequences, each pinned by a test rather +-- than left to be rediscovered: +-- +-- (a) typing a NEW bare name that happens to be a subsequence of an +-- existing entry opens the existing file instead of creating the +-- new one --- `find_file_selected_candidate_shadows_typed_text`. +-- A new bare name that matches nothing is unaffected and creates +-- normally (`find_file_bare_new_name_creates_in_the_root`). +-- (b) accepting on EMPTY input opens the first candidate in sort +-- order. `fuzzy_score` returns `Some(0)` for an empty needle, so +-- everything ties and `filter_and_sort` falls back to +-- lexicographic order --- which puts dotfiles first, and can put +-- a DIRECTORY first, in which case the open fails and reports. +-- This is the same mechanism `M-x` and `switch-buffer` already +-- have, so it is inherited rather than introduced; it is recorded +-- as decided, not overlooked, and listed in the framing's +-- deferrals beside the accept-semantics fix that would close it. -- -- The root is the active buffer's directory, or the process cwd when the -- buffer has no backing path (`source_root` defaults to "." Rust-side, diff --git a/tests/find_file_acceptance.rs b/tests/find_file_acceptance.rs index ad4c5b2..fb793e3 100644 --- a/tests/find_file_acceptance.rs +++ b/tests/find_file_acceptance.rs @@ -194,6 +194,85 @@ fn find_file_nonexistent_path_creates_a_new_file_buffer() { ); } +/// The everyday new-file flow: a BARE name, no separator, matching no +/// existing entry. The candidate list empties on its own, so the typed +/// text arrives and joins onto the root. This is the path users hit +/// first, and it is the only route through `find_file_resolve` that +/// combines free text with a relative join. +#[test] +fn find_file_bare_new_name_creates_in_the_root() { + let td = tempfile::tempdir().expect("tempdir"); + let fresh = td.path().join("zzz.txt"); + + let mut s = editor_in(td.path()); + open_prompt(&mut s); + // "zzz.txt" is not a subsequence of "anchor.txt" (no 'z' in it), so + // nothing survives the filter and the typed name is what accepts. + type_str(&mut s, "zzz.txt"); + assert!( + candidates(&s).is_empty(), + "fixture premise: a bare non-matching name must empty the list; got {:?}", + candidates(&s) + ); + + press(&mut s, KeyCode::Enter); + + let path = active_path(&s).expect("a buffer must be bound to the new path"); + assert_eq!( + std::path::Path::new(&path).parent(), + Some(td.path()), + "a bare name must join onto the prompt's root; got {path}" + ); + assert!( + path.ends_with("zzz.txt"), + "the buffer must carry the typed name; got {path}" + ); + let len: usize = eval(&s, "return pmacs.window.buffer():len()"); + assert_eq!(len, 0, "a new-file buffer starts empty"); + assert!(!fresh.exists(), "nothing is written to disk until save"); +} + +/// The failure arm. Accepting a DIRECTORY candidate reaches +/// `display_file`, whose load fails (opening a directory succeeds, the +/// read does not), and the command's `pcall` must turn that into a +/// status message rather than letting the error escape mid-dispatch. +/// Without the `pcall` this test fails, which is the point --- the +/// guard is pinned through the real accept path, not asserted directly. +#[test] +fn find_file_accepting_a_directory_reports_instead_of_raising() { + let td = tempfile::tempdir().expect("tempdir"); + std::fs::create_dir(td.path().join("sub")).expect("mkdir"); + + let mut s = editor_in(td.path()); + let before = active_path(&s).expect("the anchor must be open"); + + open_prompt(&mut s); + // Only the directory matches: "anchor.txt" contains no 's'. + type_str(&mut s, "sub"); + assert_eq!( + candidates(&s), + vec!["sub".to_string()], + "fixture premise: the directory must be the sole candidate" + ); + + press(&mut s, KeyCode::Enter); + + let line = status(&s); + assert!( + line.starts_with("find-file: "), + "the failure must surface as this command's status message; got {line:?}" + ); + assert_eq!( + active_path(&s).as_deref(), + Some(before.as_str()), + "a failed open must leave the active buffer alone" + ); + assert!( + !eval::(&s, "return pmacs.minibuffer.is_active()"), + "the prompt must have closed even though the open failed" + ); +} + /// 0d --- with no backing path, the prompt roots at the process cwd /// (`source_root` is omitted, and the Rust side defaults to "."). /// The test crate's cwd is the crate root, so `Cargo.toml` is a