diff --git a/builtin/runtime/dired.lua b/builtin/runtime/dired.lua index 78ef3b4..ceaf3d5 100644 --- a/builtin/runtime/dired.lua +++ b/builtin/runtime/dired.lua @@ -224,6 +224,11 @@ end -- `rwxr-xr-x`, without the leading kind char (rendered separately so a -- symlink shows `l` and a directory `d`). Arithmetic rather than bit -- ops: this file has to run on LuaJIT (5.1) as well as Lua 5.4. +-- +-- The nine basic bits only: setuid / setgid / sticky are deliberately +-- not surfaced as Emacs's `s` / `t`, matching the M8.3 fixture's +-- `parse_perm_string`, which edits exactly these nine. Rendering a bit +-- Stage 3 could not accept back would be worse than omitting it. local function fmt_perms(mode) local function tri(bits) local r = (bits >= 4) and "r" or "-" @@ -244,8 +249,33 @@ local function kind_char(kind) end end +-- Exact bytes while they fit the column; a magnitude past that. +-- +-- `%10d` holds ten digits, so a file of 10 GB or more (VM images, core +-- dumps --- ordinary things) widens the field and shifts mtime and name +-- right on that line alone. That is only cosmetic today, but +-- `_layout.NAME_START` is exported as a contract and Stage 3's +-- column-classifying intercept is planned against these constants, so a +-- line that violates them now is a Stage 3 trap. Same discipline as +-- `fmt_mtime`: the width is the invariant, and precision yields to it. +-- +-- This is NOT the deferred human-readable size column (§13): the exact +-- byte count is still what a listing shows, right up to the point where +-- it cannot be shown at all. +local SIZE_UNITS = { "K", "M", "G", "T", "P", "E" } + local function fmt_size(n) - return string.format("%" .. SIZE_BYTES .. "d", n) + local exact = string.format("%" .. SIZE_BYTES .. "d", n) + if #exact <= SIZE_BYTES then return exact end + local value, unit = n, SIZE_UNITS[#SIZE_UNITS] + for _, suffix in ipairs(SIZE_UNITS) do + value = value / 1024 + unit = suffix + if value < 1024 then break end + end + local scaled = string.format("%.1f%s", value, unit) + if #scaled > SIZE_BYTES then scaled = scaled:sub(1, SIZE_BYTES) end + return string.rep(" ", SIZE_BYTES - #scaled) .. scaled end local function fmt_mtime(secs) @@ -348,6 +378,13 @@ end -- Re-seat by BASENAME (Q#DR9), falling back to the nearest surviving -- line. Every repaint is wholesale, so without this a revert, a sort, -- or any Stage 2 operation would drop the cursor to the header. +-- +-- `move_to_line` is AMBIENT --- it moves the active window's cursor, not +-- `handle.buf`'s --- so every caller that can run after an `:await()` +-- has to check that dired is still the active buffer first. Painting is +-- safe either way (it names the buffer); seating is not. Callers that +-- activate the buffer themselves (an open, which displays first) are +-- unconditionally in the right place. local function seat_cursor(handle, name, fallback_line) local count = #handle.entries if count == 0 then @@ -416,7 +453,8 @@ end -- Buffer ownership -- --------------------------------------------------------------------------- -local READ_ONLY_LIMIT = 99 +-- How far the `<2>`, `<3>`, ... disambiguation walks before giving up. +local NAME_VARIANT_LIMIT = 99 -- `pmacs.buffer.create` takes any caller-chosen name, so a foreign -- buffer may already be called `*dired:/tmp*`. Painting into it through @@ -435,7 +473,7 @@ local function claim_handle(path) local name = buffer_name(path) if buffer_named(name) then local unique = nil - for i = 2, READ_ONLY_LIMIT do + for i = 2, NAME_VARIANT_LIMIT do local candidate = string.format("%s<%d>", name, i) if buffer_named(candidate) == nil then unique = candidate @@ -680,19 +718,20 @@ pmacs.command.define { return end if entry.kind == "symlink" then - -- `read_dir`/`stat` are lstat-based, so the only way to learn - -- whether a link points at a directory is to try to list it. A - -- symlinked directory is an ordinary thing to walk into, and the - -- probe costs one syscall on symlink lines only. + -- `read_dir` and `stat` are both lstat-based, so nothing in the + -- entry says whether the link points at a directory --- the only + -- way to find out is to try to list it. A symlinked directory is + -- an ordinary thing to walk into, so try the descent and fall back + -- to a file visit. + -- + -- `open_directory` is the try: it reads before touching any editor + -- state and raises having changed nothing (acceptance 15), so its + -- failure IS the "not a directory" answer. An explicit probe + -- followed by the real open would list the whole directory TWICE + -- --- opendir plus one lstat per child, each time. pmacs.async(function() - local ok = pcall(function() - return pmacs.fs.read_dir(target, { tolerant = true }):await() - end) - if ok then - local descended, err = pcall(open_directory, target, nil, handle) - if not descended then report("dired", err) end - return - end + local descended = pcall(open_directory, target, nil, handle) + if descended then return end local visited, err = pcall(pmacs.window.display_file, target, { select = true }) if not visited then report("dired", err) end end) @@ -742,7 +781,14 @@ pmacs.command.define { handle.entries = entries handle.errors = errors paint(handle) - seat_cursor(handle, name, line) + -- The re-read settles a tick or more later, and the user may have + -- left (a buffer switch, or `q`) in the meantime. The paint names + -- its buffer and is safe; seating is ambient, so a stale seat here + -- would move an unrelated buffer's cursor to a line index that + -- only means something in this listing. + if pmacs.window.buffer() == handle.buf then + seat_cursor(handle, name, line) + end end) end, } diff --git a/src/fs.rs b/src/fs.rs index 1767234..0e05cbc 100644 --- a/src/fs.rs +++ b/src/fs.rs @@ -42,6 +42,28 @@ use crate::worker::CancellationToken; /// directories. const READDIR_CANCEL_POLL_EVERY: usize = 32; +/// How many *consecutive* `readdir` iterator errors a tolerant listing +/// records before giving up and failing (dired Q#DR6). +/// +/// [`std::fs::ReadDir`] is not obliged to terminate after yielding an +/// `Err`: a directory pulled out from under a stalled network mount can +/// keep producing them. Tolerant mode records-and-continues, so without +/// a bound that is an unbounded error vector on a worker thread. +/// +/// Cancellation is **not** an adequate backstop here, which is the +/// reason this constant exists rather than a comment saying it is: a +/// dired listing carries no supersede key and nothing cancels it, so the +/// only thing that would stop the loop is the directory itself. A +/// directory whose iterator produces nothing but errors has no partial +/// answer worth rendering, so the listing fails with the last error the +/// way an unopenable directory does. +/// +/// Deliberately untested: forcing a real `readdir` to yield errors +/// repeatedly is not portable, and faking it would need the walk to be +/// generic over its iterator — a refactor with no other consumer. The +/// counter resets on any entry that materializes. +const READDIR_MAX_CONSECUTIVE_ENTRY_ERRORS: usize = 1024; + /// One directory entry as returned by [`read_dir_blocking`]. /// /// The shape is what `dired` / `magit-class` / `outline-class` @@ -277,6 +299,7 @@ pub fn read_dir_blocking( let mut errors: Option> = matches!(tolerance, ReadDirTolerance::PerEntry).then(Vec::new); let parent_str = path.display().to_string(); + let mut consecutive_entry_errors = 0usize; for (i, entry_result) in iter.enumerate() { if i % READDIR_CANCEL_POLL_EVERY == 0 && cancel.is_cancelled() { return Err(FsError::Cancelled); @@ -286,17 +309,19 @@ pub fn read_dir_blocking( Err(source) => { // R2-2: the entry never materialized, so there is no // name to report and the error names the parent. - record_entry_error( - &mut errors, - None, - FsError::Io { - path: parent_str.clone(), - source, - }, - )?; + let error = FsError::Io { + path: parent_str.clone(), + source, + }; + consecutive_entry_errors += 1; + if consecutive_entry_errors > READDIR_MAX_CONSECUTIVE_ENTRY_ERRORS { + return Err(error); + } + record_entry_error(&mut errors, None, error)?; continue; } }; + consecutive_entry_errors = 0; let entry_path = entry.path(); // Resolved first so a later per-entry failure can name it. let name = path_to_utf8_string(&entry.file_name(), &parent_str)?; diff --git a/src/lua_bindings/mod.rs b/src/lua_bindings/mod.rs index 4314e18..d689f3f 100644 --- a/src/lua_bindings/mod.rs +++ b/src/lua_bindings/mod.rs @@ -3574,6 +3574,13 @@ impl UserData for AnsiParserLua { /// an edge (`//tmp`, `~` with `HOME` unset, a `..` that would escape /// root) would mint two buffers for one directory with no error /// anywhere. +/// +/// The result crosses the boundary through `to_string_lossy`, so a +/// non-UTF-8 `$HOME` (or a non-UTF-8 argument) can yield a Lua string +/// that no longer names the `PathBuf` the registry keys on. That is the +/// same limit `pmacs.fs` already documents — byte-preserving paths are +/// post-v0.1 work that widens every path in the API — and it is recorded +/// here so this binding is not read as an exception to it. fn install_path_module(lua: &Lua) -> mlua::Result { let path = lua.create_table()?; path.set( diff --git a/src/workers_buffer.rs b/src/workers_buffer.rs index 863f4ed..6a6eeb4 100644 --- a/src/workers_buffer.rs +++ b/src/workers_buffer.rs @@ -207,10 +207,10 @@ fn format_outcome(outcome: &JobOutcome) -> String { // tolerant listing that dropped half a directory is not the // same observable outcome as a clean one. match listing.errors.as_deref() { - Some([_, ..]) => format!( + Some(errors @ [_, ..]) => format!( "ok ({} entries, {} unreadable)", listing.entries.len(), - listing.errors.as_ref().map_or(0, Vec::len) + errors.len() ), _ => format!("ok ({} entries)", listing.entries.len()), } diff --git a/tests/dired_acceptance.rs b/tests/dired_acceptance.rs index 73b06f8..7d0f2c5 100644 --- a/tests/dired_acceptance.rs +++ b/tests/dired_acceptance.rs @@ -417,6 +417,75 @@ fn dired_renders_a_header_and_one_line_per_entry() { ); } +/// The columns are a CONTRACT, not a formatting preference: `_layout` is +/// exported and Stage 3's column-classifying intercept is planned +/// against it. A size that does not fit ten digits (10 GB and up — VM +/// images, core dumps) must therefore yield precision rather than width, +/// the way `fmt_mtime` already does. Without that, one line's mtime and +/// name shift right and nothing notices until Stage 3. +#[test] +fn dired_keeps_its_columns_when_a_size_exceeds_the_field() { + let td = tempfile::tempdir().expect("tempdir"); + std::fs::write(td.path().join("small.txt"), b"x").expect("write small"); + let huge = td.path().join("huge.img"); + // Sparse: `set_len` allocates nothing on any filesystem pmacs + // supports. If one refuses, the premise cannot be established. + let file = std::fs::File::create(&huge).expect("create huge"); + if file.set_len(12_000_000_000).is_err() { + eprintln!("filesystem refused a sparse 12 GB file; skipping"); + return; + } + drop(file); + let reported = std::fs::metadata(&huge).expect("stat huge").len(); + assert!( + reported > 9_999_999_999, + "fixture premise: the size must exceed ten digits, got {reported}" + ); + + let mut s = editor(); + open_ok(&mut s, td.path(), "nil"); + let size_start = layout(&s, "SIZE_START"); + let mtime_start = layout(&s, "MTIME_START"); + let name_start = layout(&s, "NAME_START"); + + let lines = active_lines(&s); + for name in ["huge.img", "small.txt"] { + let line = &lines[line_of(&s, name)]; + let size = &line[size_start..mtime_start - 1]; + assert_eq!( + size.len(), + 10, + "the size field must stay ten columns wide: {line:?}" + ); + let stamp = &line[mtime_start..name_start - 1]; + assert!( + stamp.starts_with("20") && stamp.contains(':'), + "so the mtime still starts where the layout says: {line:?}" + ); + assert_eq!( + line_name(&s, line), + name, + "and the name still starts at NAME_START" + ); + } + + // The oversized value degrades to a magnitude rather than a + // placeholder, so the listing still says how big the file is. + let huge_line = &lines[line_of(&s, "huge.img")]; + let size = huge_line[size_start..mtime_start - 1].trim(); + assert!( + size.ends_with('G') || size.ends_with('T'), + "an oversized size keeps its magnitude: {size:?}" + ); + // A size that DOES fit stays exact. + let small_line = &lines[line_of(&s, "small.txt")]; + assert_eq!( + small_line[size_start..mtime_start - 1].trim(), + "1", + "a size that fits is still the exact byte count" + ); +} + // --------------------------------------------------------------------------- // 2 --- visit dispatches on kind, through the panel-safe primitive // --------------------------------------------------------------------------- @@ -474,6 +543,54 @@ fn dired_visit_dispatches_on_entry_kind() { ); } +/// A symlink's kind is `"symlink"` in both `read_dir` and `stat` (both +/// are lstat-based), so nothing in the entry says what it points at. +/// `RET` therefore tries the descent and falls back to a file visit — +/// one read, since `open_directory` reads before touching any editor +/// state and its failure *is* the "not a directory" answer. +#[test] +fn dired_visit_follows_a_symlink_to_the_kind_of_its_target() { + let td = fixture_dir(); + std::os::unix::fs::symlink("subdir", td.path().join("linkdir")).expect("symlink to dir"); + + let mut s = editor(); + open_ok(&mut s, td.path(), "nil"); + + // A symlink to a directory descends. The path is NOT resolved + // (canonicalization is lexical), so the buffer names the way the user + // navigated — Emacs parity. + seat_on(&s, "linkdir"); + press(&mut s, KeyCode::Enter); + pump(&mut s); + assert_eq!( + active_name(&s), + format!("*dired:{}*", canon(&td.path().join("linkdir"))), + "a symlinked directory descends under the path we walked" + ); + assert_eq!( + line_name(&s, &active_lines(&s)[1]), + "inner.txt", + "and shows the target directory's contents" + ); + + // A symlink to a file opens the file. + type_char(&mut s, '^'); + pump(&mut s); + seat_on(&s, "link"); + press(&mut s, KeyCode::Enter); + pump(&mut s); + let path = active_path(&s).expect("a file must be open"); + assert!( + path.ends_with("/link"), + "the visit keeps the link's own path; got {path}" + ); + assert_eq!( + eval::(&s, "return pmacs.window.buffer():slice(0, 5)"), + "hello", + "with the target's contents" + ); +} + /// The panel case, which is the real assertion (Q#DR10): with dired /// displayed as a panel, `RET` on a file leaves the dired panel alive /// and puts the file in the document window. Falsified by swapping @@ -1018,6 +1135,68 @@ fn dired_revert_reseats_the_cursor_by_basename() { ); } +/// A revert settles a tick or more later, and the user may have left in +/// the meantime. `pmacs.editor.move_to_line` is **ambient** — it moves +/// whatever window is active — so an unguarded re-seat moves an +/// unrelated buffer's cursor to a line index that only means something +/// in the dired listing. The paint is safe either way because it names +/// its buffer; this pins the half that does not. +#[test] +fn dired_revert_does_not_seat_a_buffer_the_user_switched_to() { + let td = tempfile::tempdir().expect("tempdir"); + for name in ["a.txt", "b.txt", "c.txt", "d.txt", "e.txt"] { + std::fs::write(td.path().join(name), b"x").expect("write"); + } + let notes = td.path().join("notes.txt"); + std::fs::write(¬es, b"one\ntwo\nthree\nfour\nfive\nsix\n").expect("write notes"); + + let mut s = editor(); + open_ok(&mut s, td.path(), "nil"); + exec(&s, "_G.DIRED_BUF = pmacs.window.buffer()"); + // A late line, so a stale seat would be visible in the other buffer. + seat_on(&s, "e.txt"); + let dired_line = cursor_line(&s); + assert!( + dired_line >= 4, + "fixture premise: a late line, got {dired_line}" + ); + + // Start the revert, then leave BEFORE the read settles. + type_char(&mut s, 'g'); + exec( + &s, + &format!( + "pmacs.buffer.find_or_open({:?})", + notes.display().to_string() + ), + ); + assert_eq!(cursor_line(&s), 0, "a freshly opened file starts at line 0"); + pump(&mut s); + + assert_eq!( + active_path(&s).map(PathBuf::from), + Some(PathBuf::from(canon(¬es))), + "the switch stands: the revert must not pull the user back" + ); + assert_eq!( + cursor_line(&s), + 0, + "and it must not move the cursor of the buffer they moved to" + ); + + // The revert itself still happened: the dired buffer is repainted, + // and returning to it seats normally on the next command. + std::fs::write(td.path().join("f.txt"), b"x").expect("write f"); + exec(&s, "pmacs.window.switch_buffer(_G.DIRED_BUF)"); + type_char(&mut s, 'g'); + pump(&mut s); + assert!( + active_text(&s).contains("f.txt"), + "the dired buffer still reverts when it is the active one: {:?}", + active_text(&s) + ); +} + // --------------------------------------------------------------------------- // 8 --- sort modes // ---------------------------------------------------------------------------