Fix review findings in the regex engine, search count and journal v2

- PCRE2: compile with the ANYCRLF newline convention and DOLLAR_ENDONLY,
  as ECMAScript reads '.' and '$'. With PCRE2's default (LF only) '.'
  matched the CR of a CRLF line, so regex replace-all deleted CRs.
- Replacement $0 is the whole match, two digits are read greedily and a
  group the pattern lacks expands to nothing, as std::regex_replace
  (libstdc++) does; these cases are now compared in the tests.
- Plain search visits overlapping matches going forward too, and counts
  them, as the backward search found them ('aa' in 'aaa' read 2/1).
- The recovery prompt's replayed content is used on 'y' only if the
  journal is unchanged since; otherwise it is replayed again.
- A version 1 journal continued after recovery is rewritten as a
  compacted version 2 journal when it needs a chunked checkpoint,
  instead of receiving records an older kte would skip. (The version is
  read through a separate descriptor: the journal fd is write-only.)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015rcVQiiWyTzoNrsvLSaAfB
This commit is contained in:
Claude
2026-09-24 09:16:04 -07:00
committed by Kyle Isom
parent e782d24e32
commit 41e28a8ab7
9 changed files with 184 additions and 31 deletions
+4 -2
View File
@@ -1169,7 +1169,7 @@ search_count(const Buffer &buf, bool regex, const std::string &q, const RegexMat
else else
c.reached = true; c.reached = true;
++c.total; ++c.total;
p += q.size(); p += 1; // overlapping, as Next/Previous visit them
} }
if (p != std::string_view::npos && p >= cur_off) if (p != std::string_view::npos && p >= cur_off)
c.reached = true; c.reached = true;
@@ -1238,7 +1238,9 @@ run_search(Editor &ed, Buffer &buf, bool regex, SearchStep step, bool incrementa
x = ed.SearchOrigX(); x = ed.SearchOrigX();
} }
if (step == SearchStep::Next && have_cur) { if (step == SearchStep::Next && have_cur) {
const std::size_t len = std::max<std::size_t>(ed.SearchMatchLen(), 1); // Plain search visits overlapping matches ("aa" in "aaa" at 0 and 1),
// as the backward search does; a regex continues after its match.
const std::size_t len = regex ? std::max<std::size_t>(ed.SearchMatchLen(), 1) : 1;
x += len; x += len;
if (x > line_text_view(buf, y).size() && y + 1 < buf.Nrows()) { if (x > line_text_view(buf, y).size() && y + 1 < buf.Nrows()) {
++y; ++y;
+14 -2
View File
@@ -397,6 +397,7 @@ Editor::CancelRecoveryPrompt()
pending_recovery_replay_err_.clear(); pending_recovery_replay_err_.clear();
std::string().swap(pending_recovery_content_); std::string().swap(pending_recovery_content_);
pending_recovery_valid_bytes_ = 0; pending_recovery_valid_bytes_ = 0;
pending_recovery_swap_size_ = 0;
pending_recovery_have_content_ = false; pending_recovery_have_content_ = false;
} }
@@ -413,6 +414,7 @@ Editor::ResolveRecoveryPrompt(const bool yes)
std::string content = std::move(pending_recovery_content_); std::string content = std::move(pending_recovery_content_);
const bool have_content = pending_recovery_have_content_; const bool have_content = pending_recovery_have_content_;
std::uint64_t valid_bytes = pending_recovery_valid_bytes_; std::uint64_t valid_bytes = pending_recovery_valid_bytes_;
const std::uint64_t swap_size_then = pending_recovery_swap_size_;
CancelRecoveryPrompt(); CancelRecoveryPrompt();
std::string err; std::string err;
@@ -427,8 +429,13 @@ Editor::ResolveRecoveryPrompt(const bool yes)
SetStatus("Recovery failed: no buffer"); SetStatus("Recovery failed: no buffer");
return false; return false;
} }
if (have_content) { std::uint64_t swap_size_now = 0;
// The prompt already replayed the journal; install that. try {
swap_size_now = std::filesystem::file_size(swp);
} catch (...) {
}
if (have_content && swap_size_now == swap_size_then) {
// The prompt already replayed the journal (unchanged since); install that.
b->replace_all_bytes(content); b->replace_all_bytes(content);
std::string().swap(content); std::string().swap(content);
} else { } else {
@@ -607,6 +614,11 @@ Editor::process_pending_opens_()
pending_recovery_content_ = std::move(rec); pending_recovery_content_ = std::move(rec);
pending_recovery_valid_bytes_ = valid; pending_recovery_valid_bytes_ = valid;
pending_recovery_have_content_ = true; pending_recovery_have_content_ = true;
try {
pending_recovery_swap_size_ = std::filesystem::file_size(swp);
} catch (...) {
pending_recovery_have_content_ = false; // replay on "y"
}
StartPrompt(PromptKind::Confirm, "Recover", ""); StartPrompt(PromptKind::Confirm, "Recover", "");
SetStatus("Recover swap edits for " + req.path + "? (y/n, C-g cancel)"); SetStatus("Recover swap edits for " + req.path + "? (y/n, C-g cancel)");
return opened_any; return opened_any;
+1
View File
@@ -721,6 +721,7 @@ private:
// produced it: "y" installs it instead of replaying the journal again. // produced it: "y" installs it instead of replaying the journal again.
std::string pending_recovery_content_; std::string pending_recovery_content_;
std::uint64_t pending_recovery_valid_bytes_ = 0; std::uint64_t pending_recovery_valid_bytes_ = 0;
std::uint64_t pending_recovery_swap_size_ = 0; // the journal's size then
bool pending_recovery_have_content_ = false; bool pending_recovery_have_content_ = false;
// GUI-only state (safe no-op in terminal builds) // GUI-only state (safe no-op in terminal builds)
+19 -15
View File
@@ -38,24 +38,20 @@ append_format(std::string &out, std::string_view fmt, std::string_view subject,
out.append(subject.substr(me)); out.append(subject.substr(me));
++i; ++i;
} else if (n >= '0' && n <= '9') { } else if (n >= '0' && n <= '9') {
// $nn if that group exists, else $n; $0 is not a reference. // As libstdc++'s match_results::format: up to two digits, read
// greedily; $0 is the whole match; a group the pattern does not
// have expands to nothing.
std::size_t idx = static_cast<std::size_t>(n - '0'); std::size_t idx = static_cast<std::size_t>(n - '0');
std::size_t used = 1; ++i;
if (i + 2 < fmt.size() && fmt[i + 2] >= '0' && fmt[i + 2] <= '9') { if (i + 1 < fmt.size() && fmt[i + 1] >= '0' && fmt[i + 1] <= '9') {
const std::size_t two = idx * 10 + static_cast<std::size_t>(fmt[i + 2] - '0'); idx = idx * 10 + static_cast<std::size_t>(fmt[i + 1] - '0');
if (two >= 1 && two < ngroups) { ++i;
idx = two;
used = 2;
}
}
if (idx == 0 || idx >= ngroups) {
out.push_back(c); // not a group reference: literal
continue;
} }
std::size_t b = 0, e = 0; std::size_t b = 0, e = 0;
if (group(idx, b, e)) if (idx == 0)
out.append(subject.substr(ms, me - ms));
else if (idx < ngroups && group(idx, b, e))
out.append(subject.substr(b, e - b)); out.append(subject.substr(b, e - b));
i += used;
} else { } else {
out.push_back(c); out.push_back(c);
} }
@@ -121,8 +117,16 @@ Regex::Compile(std::string_view pattern, std::string &err)
int errcode = 0; int errcode = 0;
PCRE2_SIZE erroff = 0; PCRE2_SIZE erroff = 0;
static const char empty = '\0'; static const char empty = '\0';
// As in ECMAScript (std::regex): '.' matches neither CR nor LF, and '$'
// matches only at the end (PCRE2's default newline is LF alone, so '.'
// matched the CR of a CRLF line and replace-all deleted it).
pcre2_compile_context *cctx = pcre2_compile_context_create(nullptr);
if (cctx)
pcre2_set_newline(cctx, PCRE2_NEWLINE_ANYCRLF);
impl->code = pcre2_compile(reinterpret_cast<PCRE2_SPTR>(pattern.data() ? pattern.data() : &empty), impl->code = pcre2_compile(reinterpret_cast<PCRE2_SPTR>(pattern.data() ? pattern.data() : &empty),
pattern.size(), 0, &errcode, &erroff, nullptr); pattern.size(), PCRE2_DOLLAR_ENDONLY, &errcode, &erroff, cctx);
if (cctx)
pcre2_compile_context_free(cctx);
if (!impl->code) { if (!impl->code) {
PCRE2_UCHAR msg[256]; PCRE2_UCHAR msg[256];
pcre2_get_error_message(errcode, msg, sizeof(msg)); pcre2_get_error_message(errcode, msg, sizeof(msg));
+5 -2
View File
@@ -11,8 +11,11 @@
* PCRE2 additionally has lookbehind, possessive quantifiers and the like. * PCRE2 additionally has lookbehind, possessive quantifiers and the like.
* *
* Replacement strings use ECMAScript syntax with either engine, as * Replacement strings use ECMAScript syntax with either engine, as
* std::regex_replace reads it: $& (the match), $1..$99 (groups), $` (the * std::regex_replace (libstdc++) reads it: $& or $0 (the match), $1..$99
* text since the previous match), $' (the rest of the line), $$ (a dollar). * (groups, two digits read greedily; a group the pattern lacks expands to
* nothing), $` (the text since the previous match), $' (the rest of the
* line), $$ (a dollar sign). As in ECMAScript, '.' matches neither CR nor
* LF and '$' matches only at the end of the line's text.
*/ */
#pragma once #pragma once
+28 -3
View File
@@ -845,6 +845,7 @@ SwapManager::open_ctx(JournalCtx &ctx, const std::string &path, std::string &err
if (st.st_size == 0) { if (st.st_size == 0) {
ctx.header_ok = write_header(fd, ctx); ctx.header_ok = write_header(fd, ctx);
ctx.approx_size_bytes = ctx.header_ok ? 64 : 0; ctx.approx_size_bytes = ctx.header_ok ? 64 : 0;
ctx.header_version = VERSION;
if (!ctx.header_ok) { if (!ctx.header_ok) {
err = "Failed to write swap file header: " + path; err = "Failed to write swap file header: " + path;
// Drop any partial header and close, so the next attempt starts a // Drop any partial header and close, so the next attempt starts a
@@ -856,6 +857,20 @@ SwapManager::open_ctx(JournalCtx &ctx, const std::string &path, std::string &err
} else { } else {
ctx.header_ok = true; // stage 1: trust existing header ctx.header_ok = true; // stage 1: trust existing header
ctx.approx_size_bytes = static_cast<std::uint64_t>(st.st_size); ctx.approx_size_bytes = static_cast<std::uint64_t>(st.st_size);
// An existing journal (continued after recovery) may be version 1,
// which must not receive chunked checkpoints (see process_one).
// (The journal fd is write-only; read through another.)
std::uint8_t ver[4] = {0, 0, 0, 0};
ctx.header_version = 0;
const int rfd = kte::syscall::Open(path.c_str(), O_RDONLY | O_CLOEXEC, 0);
if (rfd >= 0) {
if (::pread(rfd, ver, sizeof(ver), 8) == static_cast<ssize_t>(sizeof(ver)))
ctx.header_version = static_cast<std::uint32_t>(ver[0]) |
(static_cast<std::uint32_t>(ver[1]) << 8) |
(static_cast<std::uint32_t>(ver[2]) << 16) |
(static_cast<std::uint32_t>(ver[3]) << 24);
kte::syscall::Close(rfd);
}
} }
return ctx.header_ok; return ctx.header_ok;
} }
@@ -1025,6 +1040,7 @@ SwapManager::compact_to_checkpoint(JournalCtx &ctx, const RecordFrames &chkpt_re
(void) ::fcntl(tfd, F_SETFL, fl | O_APPEND); (void) ::fcntl(tfd, F_SETFL, fl | O_APPEND);
ctx.fd = tfd; ctx.fd = tfd;
ctx.header_ok = true; ctx.header_ok = true;
ctx.header_version = VERSION;
// Best-effort: fsync parent dir to persist the rename. // Best-effort: fsync parent dir to persist the rename.
try { try {
@@ -1556,8 +1572,14 @@ SwapManager::process_one(const Pending &p)
// For a large buffer the thresholds scale with its size, so the cost // For a large buffer the thresholds scale with its size, so the cost
// of compacting stays proportional to what was journaled since. // of compacting stays proportional to what was journaled since.
const std::uint64_t compact_at = std::max<std::uint64_t>(compact_bytes, 2 * p.chkpt.size()); const std::uint64_t compact_at = std::max<std::uint64_t>(compact_bytes, 2 * p.chkpt.size());
if (p.type == SwapRecType::CHKPT && compact_bytes > 0 && // A chunked checkpoint may not be appended to a version 1 journal
ctxp->approx_size_bytes + rec.total >= compact_at) { // (an older kte would skip it and misapply what follows): write it
// as a compacted version 2 journal instead.
const bool chunked = p.type == SwapRecType::CHKPT && p.chkpt.size() > kMaxRecordPayload - 5;
const bool v1_journal = ctxp->header_version != 0 && ctxp->header_version < 2;
if (p.type == SwapRecType::CHKPT &&
((chunked && v1_journal) ||
(compact_bytes > 0 && ctxp->approx_size_bytes + rec.total >= compact_at))) {
std::string compact_err; std::string compact_err;
if (compact_to_checkpoint(*ctxp, rec, compact_err)) { if (compact_to_checkpoint(*ctxp, rec, compact_err)) {
std::lock_guard<std::mutex> lg(mtx_); std::lock_guard<std::mutex> lg(mtx_);
@@ -1566,7 +1588,10 @@ SwapManager::process_one(const Pending &p)
circuit_breaker_.RecordSuccess(); circuit_breaker_.RecordSuccess();
return; return;
} }
report_error(compact_err, p.buf); // fall back to appending report_error(compact_err, p.buf);
if (chunked && v1_journal)
return; // not appended; the records around it stay valid
// otherwise fall back to appending
} }
// Remember where this record starts so a partial write can be undone; // Remember where this record starts so a partial write can be undone;
+1
View File
@@ -237,6 +237,7 @@ private:
bool gap{false}; bool gap{false};
bool gap_notified{false}; // the user was told about the current gap bool gap_notified{false}; // the user was told about the current gap
bool chkpt_queued{false}; // a checkpoint is queued and not yet being written bool chkpt_queued{false}; // a checkpoint is queued and not yet being written
std::uint32_t header_version{0}; // of the open journal file (0: none yet)
bool lockout_notified{false}; // told about the lock; kept across ResetJournal bool lockout_notified{false}; // told about the lock; kept across ResetJournal
std::uint64_t gap_chkpt_request_ns{0}; std::uint64_t gap_chkpt_request_ns{0};
bool gap_unfixable_reported{false}; // gap on a buffer too large to checkpoint bool gap_unfixable_reported{false}; // gap on a buffer too large to checkpoint
+9
View File
@@ -558,6 +558,15 @@ before the fix (unless noted).
1.55 s -> 0.10 s per keystroke. 1.55 s -> 0.10 s per keystroke.
- Only one checkpoint per journal is queued at a time; whole-file edits - Only one checkpoint per journal is queued at a time; whole-file edits
in a row on a large buffer each queued a full copy of it. in a row on a large buffer each queued a full copy of it.
- Review of this work found, and these are fixed: with PCRE2, '.'
matched the CR of CRLF lines (regex replace-all deleted it) — PCRE2
now uses the ANYCRLF newline convention and $ matches only at the end;
$0, out-of-range $n and $nn in replacements now follow libstdc++;
plain search visits overlapping matches in both directions (the count
could read "2/1"); the recovery prompt replays again if the journal
changed while it was open; a version 1 journal continued after
recovery gets a chunked checkpoint only by being rewritten as
version 2.
**Known limitations (not fixed)** **Known limitations (not fixed)**
- Without PCRE2 (std::regex builds), catastrophic regex backtracking - Without PCRE2 (std::regex builds), catastrophic regex backtracking
+100 -4
View File
@@ -17,6 +17,7 @@
#include "syntax/LanguageHighlighter.h" #include "syntax/LanguageHighlighter.h"
#include <chrono> #include <chrono>
#include <cstring>
#include <filesystem> #include <filesystem>
#include <fstream> #include <fstream>
#include <fcntl.h> #include <fcntl.h>
@@ -2184,16 +2185,20 @@ TEST(Swap_DeferredBaseCrc)
} }
// kte::Regex replacements follow std::regex_replace (ECMAScript), whichever // kte::Regex replacements follow std::regex_replace (ECMAScript as libstdc++
// engine is built in. (References to groups the pattern does not have, like // reads it), whichever engine is built in.
// $3 with two groups, are implementation-defined and left out.)
TEST(RegexEngine_ReplaceMatchesStdRegex) TEST(RegexEngine_ReplaceMatchesStdRegex)
{ {
const char *subjects[] = {"", "baaac", "one two three", "a-b-c", "xyz", "aaa", " lead", "tail "}; const char *subjects[] = {"", "baaac", "one two three", "a-b-c", "xyz", "aaa", " lead", "tail "};
const std::pair<const char *, const char *> cases[] = { const std::pair<const char *, const char *> cases[] = {
{"a*", "X"}, {"a", "[$&]"}, {"(\\w+)", "<$1>"}, {"(o)(n)?", "$2$1"}, {"-", "$`|$'"}, {"a*", "X"}, {"a", "[$&]"}, {"(\\w+)", "<$1>"}, {"(o)(n)?", "$2$1"}, {"-", "$`|$'"},
{"\\s+", "$$"}, {"^", ">"}, {"$", "<"}, {"b|", "_"}, {"\\b", "|"}, {"\\s+", "$$"}, {"^", ">"}, {"$", "<"}, {"b|", "_"}, {"\\b", "|"},
{"([a-z])([a-z])", "$2$1"}, {"(a)(x)?", "[$2]"} {"([a-z])([a-z])", "$2$1"}, {"(a)(x)?", "[$2]"},
#if defined(__GLIBCXX__)
// Read as libstdc++ reads them (libc++ differs): $0 is the match,
// two digits are read greedily, missing groups expand to nothing.
{"b", "[$0]"}, {"(b?)", "<$10>"}, {"(a)", "[$2]"}, {"(a)(b)?", "$12"},
#endif
}; };
for (const char *subj: subjects) { for (const char *subj: subjects) {
for (const auto &[pat, fmt]: cases) { for (const auto &[pat, fmt]: cases) {
@@ -2228,3 +2233,94 @@ TEST(RegexEngine_CatastrophicPatternStops)
ASSERT_TRUE(h.EditorRef().Status().find("too complex") != std::string::npos); ASSERT_TRUE(h.EditorRef().Status().find("too complex") != std::string::npos);
ASSERT_TRUE(h.Exec(CommandId::Refresh)); ASSERT_TRUE(h.Exec(CommandId::Refresh));
} }
// '.' does not match the CR of a CRLF line (PCRE2's default newline
// convention let it, and regex replace-all deleted the CR).
TEST(RegexEngine_DotDoesNotMatchCR)
{
TestHarness h;
h.Buf().insert_text(0, 0, "abc\r\ndef\r\n");
regex_replace_all(h, "b.*", "X");
ASSERT_EQ(h.Text(), std::string("aX\r\ndef\r\n"));
kte::Regex rx;
std::string err;
ASSERT_TRUE(rx.Compile("c$", err));
std::size_t pos = 0, len = 0;
ASSERT_TRUE(!rx.Search("abc\r", 0, pos, len)); // '$' only at the very end
ASSERT_TRUE(rx.Search("abc", 0, pos, len));
}
// Plain search visits overlapping matches in both directions, and the count
// agrees (Previous found the overlapping one while Next and the count
// skipped it: "2/1").
TEST(Search_OverlappingMatchesConsistent)
{
TestHarness h;
Editor &ed = h.EditorRef();
h.Buf().insert_text(0, 0, "aaa");
h.Buf().SetCursor(0, 0);
ASSERT_TRUE(h.Exec(CommandId::FindStart));
ASSERT_TRUE(h.Exec(CommandId::InsertText, "aa"));
ASSERT_EQ(h.Buf().Curx(), (std::size_t) 0);
ASSERT_TRUE(ed.Status().find("1/2") != std::string::npos);
ASSERT_TRUE(h.Exec(CommandId::MoveRight));
ASSERT_EQ(h.Buf().Curx(), (std::size_t) 1);
ASSERT_TRUE(ed.Status().find("2/2") != std::string::npos);
ASSERT_TRUE(h.Exec(CommandId::MoveLeft));
ASSERT_EQ(h.Buf().Curx(), (std::size_t) 0);
ASSERT_TRUE(h.Exec(CommandId::MoveLeft)); // wraps
ASSERT_EQ(h.Buf().Curx(), (std::size_t) 1);
ASSERT_TRUE(ed.Status().find("2/2") != std::string::npos);
ASSERT_TRUE(h.Exec(CommandId::Refresh));
}
// A version 1 journal (continued after recovery) never receives chunked
// checkpoint records, which an older kte would skip: the checkpoint is
// written as a compacted version 2 journal instead.
TEST(Swap_V1JournalGetsNoChunkedRecords)
{
TempDir d("v1_chunked");
const std::string path = (d.path / "big.txt").string();
const std::string text = big_text();
{
std::ofstream o(path, std::ios::binary);
o << text;
}
Buffer b;
std::string err;
ASSERT_TRUE(b.OpenFromFile(path, err));
const std::string swp = kte::SwapManager::ComputeSwapPathForTests(b);
{
// A bare version 1 header (no base identity recorded).
std::string hdr(64, '\0');
std::memcpy(hdr.data(), "KTE_SWP", 8);
hdr[8] = 1;
std::ofstream o(swp, std::ios::binary | std::ios::trunc);
o << hdr;
}
kte::SwapManager sm;
sm.Attach(&b);
b.SetSwapRecorder(sm.RecorderFor(&b));
b.insert_text(0, 0, "X");
sm.Flush(&b);
sm.Checkpoint(&b);
sm.Flush(&b);
b.insert_text(0, 1, "Y");
sm.Flush(&b);
const std::string j = slurp(swp);
ASSERT_EQ(static_cast<unsigned char>(j[8]), 2u); // rewritten as version 2
const std::string copy = (d.path / "copy.swp").string();
{
std::ofstream o(copy, std::ios::binary | std::ios::trunc);
o << j;
}
Buffer r;
ASSERT_TRUE(r.OpenFromFile(path, err));
ASSERT_TRUE(kte::SwapManager::ReplayFile(r, copy, err));
ASSERT_TRUE(r.BytesForTests() == b.BytesForTests());
b.SetSwapRecorder(nullptr);
sm.Detach(&b, true);
}