From b1b0b3de570ade986b50389c6b04119044154753 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 07:12:32 +0000 Subject: [PATCH] Fix round-four regressions: save fallback, replace memory, regex skip - Save's in-place fallbacks (no temp file; no hard-link copy) now apply only when the directory refuses a new name (EACCES, EPERM, ENAMETOOLONG). On ENOSPC, EDQUOT or EIO the in-place write truncated the file and then failed, leaving it corrupt; now the save fails and the file is untouched (checked on a full tmpfs). - replace_rows_text/replace_buffer_text delete and insert only the span between the old and new text's common prefix and suffix (kept on UTF-8 boundaries). Replace-all recorded the whole buffer twice in the undo tree per run. - Regex search skips long lines only while typing (limit lowered to 2,000 bytes, as for renderer highlighting; '.*q' on a 20 KB line took 12 s), reports how many were skipped, and next/previous match search every line. Previously matches on long lines could not be found. Co-Authored-By: Claude Opus 5.5 Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB --- Buffer.cc | 39 ++++++-- Command.cc | 125 +++++++++++++++--------- RegexGuard.h | 11 ++- docs/audits/claude-findings-20260923.md | 15 +++ tests/test_audit_regressions.cc | 98 +++++++++++++++++++ 5 files changed, 230 insertions(+), 58 deletions(-) diff --git a/Buffer.cc b/Buffer.cc index 48e8a68..0715673 100644 --- a/Buffer.cc +++ b/Buffer.cc @@ -173,13 +173,15 @@ write_in_place(const std::string &path, const char *data, std::size_t len, std:: // Create a new file named from `tmpl` (a mkstemp template, which receives // the chosen name) holding the content, fsynced. static bool -write_new_file(std::string &tmpl, const char *data, std::size_t len, std::string &err) +write_new_file(std::string &tmpl, const char *data, std::size_t len, std::string &err, int &create_errno) { + create_errno = 0; std::vector name(tmpl.begin(), tmpl.end()); name.push_back('\0'); const int fd = kte::syscall::Mkstemp(name.data()); if (fd < 0) { - err = "Failed to create " + tmpl + ": " + std::strerror(errno); + create_errno = errno; + err = "Failed to create " + tmpl + ": " + std::strerror(create_errno); return false; } tmpl.assign(name.data()); @@ -196,6 +198,18 @@ write_new_file(std::string &tmpl, const char *data, std::size_t len, std::string } +// Whether a failure to create a file beside the target means only that the +// directory will not take a new name (not writable, name too long), so the +// target itself may still be rewritten in place. Anything else (ENOSPC, +// EDQUOT, EIO, ...) would most likely hit the in-place write too, after it +// had already truncated the file. +static bool +may_write_in_place_after(int create_errno) +{ + return create_errno == EACCES || create_errno == EPERM || create_errno == ENAMETOOLONG; +} + + static bool atomic_write_file(const std::string &path_in, const char *data, std::size_t len, std::string &err) { @@ -222,11 +236,17 @@ atomic_write_file(const std::string &path_in, const char *data, std::size_t len, if (dst_exists && S_ISREG(dst_st.st_mode) && dst_st.st_nlink > 1) { // Truncating in place risks the only copy if the write then fails // (ENOSPC, EIO, crash), so first write and sync a copy beside it. If - // no copy can be made (directory not writable, name too long), write - // in place anyway rather than refuse to save. + // the directory will not take a new file (not writable, name too + // long), write in place anyway rather than refuse to save; any + // other failure (out of space, I/O error) refuses the save. std::string copy = path + ".kte-save.XXXXXX"; std::string copy_err; - const bool have_copy = write_new_file(copy, data, len, copy_err); + int copy_errno = 0; + const bool have_copy = write_new_file(copy, data, len, copy_err, copy_errno); + if (!have_copy && !may_write_in_place_after(copy_errno)) { + err = "Save failed, file left unchanged: " + copy_err; + return false; + } if (!write_in_place(path, data, len, err)) { if (have_copy) err += " (the new content is in " + copy + ")"; @@ -261,10 +281,11 @@ atomic_write_file(const std::string &path_in, const char *data, std::size_t len, if (!kte::RetryOnTransientError(mkstemp_fn, kte::RetryPolicy::Aggressive(), err)) { const int saved_errno = errno; - // No temp file in the directory (e.g. the file is writable but its - // directory is not): an existing regular file can still be written in - // place, as editors traditionally do. - if (fd < 0 && dst_exists && S_ISREG(dst_st.st_mode)) { + // No temp file in the directory because the directory is not + // writable (the file is): an existing regular file can still be + // written in place, as editors traditionally do. Not on ENOSPC and + // the like: the in-place write would truncate the file and then fail. + if (fd < 0 && dst_exists && S_ISREG(dst_st.st_mode) && may_write_in_place_after(saved_errno)) { err.clear(); return write_in_place(path, data, len, err); } diff --git a/Command.cc b/Command.cc index 677cd46..b876e6a 100644 --- a/Command.cc +++ b/Command.cc @@ -629,58 +629,78 @@ buffer_text(const Buffer &buf) } -// Replace the whole buffer's text as one undo group (one delete, one insert). +// Replace the text of rows [y, y + old_text lines) in place with new_text, +// where old_text is those rows joined by '\n' (no trailing newline). Unlike +// delete_row/insert_row this never adds or removes the newline after the +// last replaced row, so a file without a trailing newline keeps that shape. +// +// Only the span between the texts' common prefix and suffix is deleted and +// inserted: recording the whole text kept two more copies of it in the undo +// tree (and the piece table and journal) for every replace, however small. static void -replace_buffer_text(Buffer &buf, const std::string &old_text, const std::string &new_text, UndoSystem *u) +replace_rows_text(Buffer &buf, std::size_t y, const std::string &old_text, const std::string &new_text, + UndoSystem *u) { - UndoGroupGuard group(u); - if (!old_text.empty()) { - buf.delete_text(0, 0, old_text.size()); + const std::size_t common = std::min(old_text.size(), new_text.size()); + std::size_t pre = 0; + while (pre < common && old_text[pre] == new_text[pre]) + ++pre; + std::size_t suf = 0; + while (suf < common - pre && old_text[old_text.size() - 1 - suf] == new_text[new_text.size() - 1 - suf]) + ++suf; + // Keep UTF-8 sequences whole on both sides of the edit. + auto is_cont = [](char c) { + return (static_cast(c) & 0xC0) == 0x80; + }; + while (pre > 0 && ((pre < old_text.size() && is_cont(old_text[pre])) || + (pre < new_text.size() && is_cont(new_text[pre])))) + --pre; + while (suf > 0 && (is_cont(old_text[old_text.size() - suf]) || is_cont(new_text[new_text.size() - suf]))) + --suf; + if (pre + suf > old_text.size() || pre + suf > new_text.size()) + suf = std::min(old_text.size(), new_text.size()) - pre; + const std::size_t del_len = old_text.size() - pre - suf; + const std::size_t ins_len = new_text.size() - pre - suf; + if (del_len == 0 && ins_len == 0) + return; + + // Row and column of the first changed byte. + std::size_t row = y, col = pre; + const std::size_t last_nl = pre == 0 ? std::string::npos : old_text.rfind('\n', pre - 1); + if (last_nl != std::string::npos) { + row += static_cast(std::count(old_text.begin(), + old_text.begin() + static_cast(pre), '\n')); + col = pre - last_nl - 1; + } + + if (del_len > 0) { + buf.delete_text(static_cast(row), static_cast(col), del_len); if (u) { - buf.SetCursor(0, 0); + buf.SetCursor(col, row); u->Begin(UndoType::Delete); - u->Append(std::string_view(old_text)); + u->Append(std::string_view(old_text).substr(pre, del_len)); u->commit(); } } - if (!new_text.empty()) { - buf.insert_text(0, 0, new_text); + if (ins_len > 0) { + const std::string_view ins = std::string_view(new_text).substr(pre, ins_len); + buf.insert_text(static_cast(row), static_cast(col), ins); if (u) { - buf.SetCursor(0, 0); + buf.SetCursor(col, row); u->Begin(UndoType::Insert); - u->Append(std::string_view(new_text)); + u->Append(ins); u->commit(); } } } -// Replace the text of rows [y, y + old_text lines) in place with new_text, -// where old_text is those rows joined by '\n' (no trailing newline). Unlike -// delete_row/insert_row this never adds or removes the newline after the -// last replaced row, so a file without a trailing newline keeps that shape. +// Replace the whole buffer's text as one undo group. static void -replace_rows_text(Buffer &buf, std::size_t y, const std::string &old_text, const std::string &new_text, - UndoSystem *u) +replace_buffer_text(Buffer &buf, const std::string &old_text, const std::string &new_text, UndoSystem *u) { - if (!old_text.empty()) { - buf.delete_text(static_cast(y), 0, old_text.size()); - if (u) { - buf.SetCursor(0, y); - u->Begin(UndoType::Delete); - u->Append(std::string_view(old_text)); - u->commit(); - } - } - if (!new_text.empty()) { - buf.insert_text(static_cast(y), 0, new_text); - if (u) { - buf.SetCursor(0, y); - u->Begin(UndoType::Insert); - u->Append(std::string_view(new_text)); - u->commit(); - } - } + UndoGroupGuard group(u); + replace_rows_text(buf, 0, old_text, new_text, u); } @@ -848,8 +868,11 @@ struct RegexMatch { static std::vector -search_compute_matches_regex(const Buffer &buf, const std::string &pattern, std::string &err_out) +search_compute_matches_regex(const Buffer &buf, const std::string &pattern, std::string &err_out, + std::size_t line_limit = static_cast(-1), std::size_t *skipped = nullptr) { + if (skipped) + *skipped = 0; std::vector out; err_out.clear(); if (pattern.empty()) @@ -860,13 +883,16 @@ search_compute_matches_regex(const Buffer &buf, const std::string &pattern, std: // std::regex recurses per matched character; see RegexGuard.h. kte::RunWithLargeStack([&] { for (std::size_t y = 0; y < rows.size(); ++y) { - // This runs on every prompt keystroke, and std::regex can be - // quadratic in line length for ordinary patterns (".*q" on a - // 40 KB minified line took over a minute, uninterruptibly), so - // incremental search skips very long lines. Regex replace, an - // explicit one-shot command, still processes every line. - if (rows[y].size() > kte::kRegexIncrementalLineLimit) + // std::regex can be quadratic in line length for ordinary + // patterns (".*q" on a 40 KB minified line took over a + // minute, uninterruptibly), so the per-keystroke caller + // passes a limit; moving to the next/previous match + // (Left/Right in the prompt) searches every line. + if (rows[y].size() > line_limit) { + if (skipped) + ++*skipped; continue; + } std::string line = static_cast(rows[y]); for (auto it = std::sregex_iterator(line.begin(), line.end(), rx); it != std::sregex_iterator(); ++it) { @@ -2411,7 +2437,9 @@ cmd_insert_text(CommandContext &ctx) if (ctx.editor.CurrentPromptKind() == Editor::PromptKind::RegexSearch || ctx.editor.CurrentPromptKind() == Editor::PromptKind::RegexReplaceFind) { std::string err; - auto rmatches = search_compute_matches_regex(*buf, ctx.editor.SearchQuery(), err); + std::size_t skipped = 0; + auto rmatches = search_compute_matches_regex(*buf, ctx.editor.SearchQuery(), err, + kte::kRegexIncrementalLineLimit, &skipped); if (!err.empty()) { ctx.editor.SetStatus( std::string("Regex: ") + ctx.editor.PromptText() + " [error: " + err + @@ -2420,6 +2448,9 @@ cmd_insert_text(CommandContext &ctx) if (ctx.editor.SearchIndex() >= static_cast(rmatches.size())) ctx.editor.SetSearchIndex(rmatches.empty() ? -1 : 0); search_apply_match_regex(ctx.editor, *buf, rmatches); + if (skipped > 0 && err.empty()) + ctx.editor.SetStatus(ctx.editor.Status() + " [" + std::to_string(skipped) + + " long line(s) skipped; Left/Right search all]"); } else { auto matches = search_compute_matches(*buf, ctx.editor.SearchQuery()); // Keep index stable unless out of range @@ -3571,13 +3602,19 @@ cmd_backspace(CommandContext &ctx) if (ctx.editor.CurrentPromptKind() == Editor::PromptKind::RegexSearch || ctx.editor.CurrentPromptKind() == Editor::PromptKind::RegexReplaceFind) { std::string err; - auto rm = search_compute_matches_regex(*buf2, ctx.editor.SearchQuery(), err); + std::size_t skipped = 0; + auto rm = search_compute_matches_regex( + *buf2, ctx.editor.SearchQuery(), err, kte::kRegexIncrementalLineLimit, + &skipped); if (!err.empty()) { ctx.editor.SetStatus( std::string("Regex: ") + ctx.editor.PromptText() + " [error: " + err + "]"); } search_apply_match_regex(ctx.editor, *buf2, rm); + if (skipped > 0 && err.empty()) + ctx.editor.SetStatus(ctx.editor.Status() + " [" + std::to_string(skipped) + + " long line(s) skipped; Left/Right search all]"); } else { auto matches = search_compute_matches(*buf2, ctx.editor.SearchQuery()); search_apply_match(ctx.editor, *buf2, matches); diff --git a/RegexGuard.h b/RegexGuard.h index 522fcee..b37bb09 100644 --- a/RegexGuard.h +++ b/RegexGuard.h @@ -13,11 +13,12 @@ namespace kte { // Renderers highlight regex matches only on lines up to this many bytes, and -// incremental regex search skips longer lines: std::regex can take time -// quadratic in line length even for ordinary patterns, with no way to -// interrupt it, and these run on every frame or keystroke. -constexpr std::size_t kRegexRenderLineLimit = 10000; -constexpr std::size_t kRegexIncrementalLineLimit = 20000; +// search-as-you-type skips longer lines (an explicit search does not): +// std::regex can take time quadratic in line length even for ordinary +// patterns (".*q" on a 20 KB line: 12 s), with no way to interrupt it, and +// these run on every frame or keystroke. +constexpr std::size_t kRegexRenderLineLimit = 2000; +constexpr std::size_t kRegexIncrementalLineLimit = 2000; // Run fn to completion on a thread with a large stack (address space is // reserved, pages are committed only as used), so regex work over long lines diff --git a/docs/audits/claude-findings-20260923.md b/docs/audits/claude-findings-20260923.md index c6d1dad..0379656 100644 --- a/docs/audits/claude-findings-20260923.md +++ b/docs/audits/claude-findings-20260923.md @@ -410,6 +410,21 @@ before the fix (unless noted). after a close; a reload confirmation survived other commands; a swap recovery prompt from a deferred open was not drawn until a key. +**Fifth round (review of the fourth round's changes)** +- The save fallbacks added in round four (write in place when no temp + file or hard-link copy can be made) also fired on ENOSPC/EDQUOT/EIO, + truncating the file and then failing: the file on disk was left + corrupt. They now apply only when the directory refuses a new name + (EACCES, EPERM, ENAMETOOLONG); otherwise the save fails and the file + is untouched. +- Whole-buffer replace recorded the entire old and new text for undo on + every run (about 150 MB more per replace-all on a 50 MB file); only + the span between the common prefix and suffix is now recorded. +- Regex search silently never found matches on lines over 20,000 bytes; + the skip now applies only to search-as-you-type (at 2,000 bytes, where + the worst case is well under a second), says so in the status line, + and moving to the next/previous match searches every line. + **Known limitations (not fixed)** - Catastrophic regex backtracking (e.g. `(a*)*b`) can still take very long; std::regex has no time limit. diff --git a/tests/test_audit_regressions.cc b/tests/test_audit_regressions.cc index 4bda202..dd90711 100644 --- a/tests/test_audit_regressions.cc +++ b/tests/test_audit_regressions.cc @@ -1239,3 +1239,101 @@ TEST(Audit_BufferId_StableAcrossMovesUniqueForCopies) f = e; ASSERT_TRUE(f.Id() != id); } + + +// Replace-all records only the changed span in the undo tree (it recorded +// the whole buffer twice per run), and undo/redo still restore the text, +// including when the change sits next to multibyte characters. +TEST(Audit_ReplaceAll_RecordsOnlyChangedSpan) +{ + TestHarness h; + const std::string big(100000, 'x'); + const std::string before = big + "\nh\xC3\xA9llo w\xC3\xB8rld\n" + big + "\n"; + h.Buf().insert_text(0, 0, before); + h.Buf().SetCursor(0, 0); + regex_replace_all(h, "\xC3\xB8", "\xC3\xA5"); + const std::string after = big + "\nh\xC3\xA9llo w\xC3\xA5rld\n" + big + "\n"; + ASSERT_EQ(h.Text(), after); + const UndoNode *cur = h.Buf().Undo()->TreeForTests().current; + ASSERT_TRUE(cur != nullptr); + ASSERT_TRUE(cur->text.size() < 16); + ASSERT_TRUE(cur->parent != nullptr && cur->parent->text.size() < 16); + ASSERT_TRUE(h.Undo()); + ASSERT_EQ(h.Text(), before); + ASSERT_TRUE(h.Redo()); + ASSERT_EQ(h.Text(), after); + + // Growing, and splitting lines. + regex_replace_all(h, "rld", "rld!\n"); + regex_replace_all(h, "o w", "o\nw"); + const std::string after2 = big + "\nh\xC3\xA9llo\nw\xC3\xA5rld!\n\n" + big + "\n"; + ASSERT_EQ(h.Text(), after2); + ASSERT_TRUE(h.Undo()); + ASSERT_TRUE(h.Undo()); + ASSERT_EQ(h.Text(), after); + ASSERT_TRUE(h.Redo()); + ASSERT_TRUE(h.Redo()); + ASSERT_EQ(h.Text(), after2); +} + + +// Search-as-you-type skips very long lines (std::regex can be quadratic in +// line length) but says so, and moving to the next match searches them. +TEST(Audit_RegexSearch_LongLineFoundByNext) +{ + TestHarness h; + Editor &ed = h.EditorRef(); + h.Buf().insert_text(0, 0, "short\n" + std::string(25000, 'a') + "NEEDLE\nend\n"); + h.Buf().SetCursor(0, 0); + ASSERT_TRUE(h.Exec(CommandId::RegexFindStart)); + ASSERT_TRUE(h.Exec(CommandId::InsertText, "NEE+DLE")); + ASSERT_TRUE(ed.Status().find("long line") != std::string::npos); + ASSERT_TRUE(h.Exec(CommandId::MoveRight)); + ASSERT_EQ(h.Buf().Cury(), (std::size_t) 1); + ASSERT_EQ(h.Buf().Curx(), (std::size_t) 25000); +} + + +// Replace-all against a per-line std::regex_replace model, with overlapping +// common prefixes and suffixes; undo restores the original each time. +TEST(Audit_ReplaceAll_TrimmedEditMatchesModel) +{ + const char *texts[] = {"aaa\nbab\n", "abc", "\xC3\xA9\xC3\xA9x\xC3\xA9\n\n", "aXa\naa\n", "\n\n"}; + const std::pair reps[] = { + {"a", ""}, {"a", "aa"}, {"b", "a"}, {"^", "a"}, {"X", "aXa"}, {"a$", ""}, {"x", "\xC3\xA9"}, + {"\xC3\xA9", "e"}, {"aa", "a"}, {"^$", "a"} + }; + for (const char *t: texts) { + for (const auto &[find, with]: reps) { + TestHarness h; + h.Buf().insert_text(0, 0, t); + h.Buf().SetCursor(0, 0); + std::string expect; + const std::regex rx(find); + std::string src(t); + std::size_t start = 0; + while (true) { + const std::size_t nl = src.find('\n', start); + if (start == src.size() && start > 0) + break; // no line after a trailing newline + const std::string line = src.substr(start, nl == std::string::npos ? std::string::npos : nl - start); + expect += std::regex_replace(line, rx, with); + if (nl == std::string::npos) + break; + expect += '\n'; + start = nl + 1; + } + regex_replace_all(h, find, with); + if (h.Text() != expect) + fprintf(stderr, "text=[%s] find=[%s] with=[%s] got=[%s] want=[%s]\n", t, find, with, + h.Text().c_str(), expect.c_str()); + ASSERT_EQ(h.Text(), expect); + if (expect != t) { + ASSERT_TRUE(h.Undo()); + ASSERT_EQ(h.Text(), std::string(t)); + ASSERT_TRUE(h.Redo()); + ASSERT_EQ(h.Text(), expect); + } + } + } +}