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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB
This commit is contained in:
@@ -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<char> 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);
|
||||
}
|
||||
|
||||
+81
-44
@@ -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<unsigned char>(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::size_t>(std::count(old_text.begin(),
|
||||
old_text.begin() + static_cast<std::ptrdiff_t>(pre), '\n'));
|
||||
col = pre - last_nl - 1;
|
||||
}
|
||||
|
||||
if (del_len > 0) {
|
||||
buf.delete_text(static_cast<int>(row), static_cast<int>(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<int>(row), static_cast<int>(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<int>(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<int>(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<RegexMatch>
|
||||
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<std::size_t>(-1), std::size_t *skipped = nullptr)
|
||||
{
|
||||
if (skipped)
|
||||
*skipped = 0;
|
||||
std::vector<RegexMatch> 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<std::string>(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<int>(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);
|
||||
|
||||
+6
-5
@@ -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
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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<const char *, const char *> 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);
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user