Skip to content

Commit d90f98f

Browse files
DeusDataandis777
andcommitted
fix(store): keep ADR line endings intact when splicing a section
Every ADR fixture in the tree used \n, so nothing exercised a CRLF-stored document — and CI could not have found this, because the fixtures are LF whichever platform runs them. A red would only have appeared once a real user's CRLF ADR met this code. The exposure is specific to splicing. The old rebuild normalised everything on the way out, so line-ending confusion was invisible; a byte-span replacement can miscount a separator run, and byte-identity is the property the splice design exists to provide. Two of the three paths were already correct, and now say so in tests: - Replacing a section preserves CRLF exactly. The walk-back over a trailing break run steps across both '\r' and '\n', so it cannot cut mid-pair. - Heading matching is ending-agnostic: "## PURPOSE\r\n" locates the same as "## PURPOSE\n", because the scanner trims '\r' from the name. Trimming and matching were different claims and only trimming was pinned. Appending a new section was wrong. It counted trailing '\n' characters, so a document ending "...Foo\r\n" was read as having one break but then had a BARE '\n' appended, and the "## NAME" line it wrote used '\n' too — leaving a mixed document that neither the author nor the tool asked for. A CRLF file ending in a blank line also gained a spurious extra break. Appends now count a "\r\n" pair as one break, and write the separator and heading line using the document's own ending. The contract, now that a document can be mixed: - Bytes that already exist are never rewritten, so each line keeps whatever ending it had. This follows from splicing and is what the acceptance property already guaranteed. - Text this code writes itself — the separator and the "## NAME" line — follows the document's majority ending, LF on a tie. - A caller's body is inserted verbatim. Rewriting bytes a caller supplied would be the same silent modification this whole change set removes. Co-authored-by: andis777 <24672074+andis777@users.noreply.github.com> Signed-off-by: Martin Vogel <martin.vogel.tech@gmail.com>
1 parent d013555 commit d90f98f

2 files changed

Lines changed: 162 additions & 21 deletions

File tree

‎src/store/store.c‎

Lines changed: 62 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -8900,6 +8900,44 @@ static void adr_find_cb(void *ctx, const cbm_adr_heading_t *h) {
89008900
f->body_end = h->body_end;
89018901
}
89028902

8903+
/* The line ending this code should WRITE into `doc`: whichever of "\r\n" and
8904+
* "\n" the document already uses more often, LF on a tie or an empty document.
8905+
* Lines that already exist are never rewritten — this decides only the
8906+
* separator and the "## NAME" line an append adds, so a CRLF document does not
8907+
* acquire a stray bare newline nobody asked for. */
8908+
static const char *adr_dominant_eol(const char *doc, size_t doc_len) {
8909+
size_t crlf = 0;
8910+
size_t lf = 0;
8911+
for (size_t i = 0; i < doc_len; i++) {
8912+
if (doc[i] != '\n') {
8913+
continue;
8914+
}
8915+
if (i > 0 && doc[i - SKIP_ONE] == '\r') {
8916+
crlf++;
8917+
} else {
8918+
lf++;
8919+
}
8920+
}
8921+
return crlf > lf ? "\r\n" : "\n";
8922+
}
8923+
8924+
/* Trailing line breaks, counting a "\r\n" PAIR as ONE break. Counting raw '\n'
8925+
* characters gets the count right but loses the shape, and the separator that
8926+
* is then appended has to match the ending it is continuing. */
8927+
static size_t adr_trailing_breaks(const char *doc, size_t doc_len) {
8928+
size_t idx = doc_len;
8929+
size_t breaks = 0;
8930+
while (idx > 0 && (doc[idx - SKIP_ONE] == '\n' || doc[idx - SKIP_ONE] == '\r')) {
8931+
bool was_lf = doc[idx - SKIP_ONE] == '\n';
8932+
idx--;
8933+
if (was_lf && idx > 0 && doc[idx - SKIP_ONE] == '\r') {
8934+
idx--;
8935+
}
8936+
breaks++;
8937+
}
8938+
return breaks;
8939+
}
8940+
89038941
char *cbm_adr_splice_section(const char *content, const char *name, const char *body) {
89048942
if (!name || !body) {
89058943
return NULL;
@@ -8951,37 +8989,40 @@ char *cbm_adr_splice_section(const char *content, const char *name, const char *
89518989
return out;
89528990
}
89538991

8954-
/* Absent: append. Enough newlines are added to leave exactly one blank
8992+
/* Absent: append. Enough line breaks are added to leave exactly one blank
89558993
* line before the new heading — never fewer, and never rewriting the
8956-
* newlines the document already ends with. */
8957-
size_t trailing = 0;
8958-
while (trailing < doc_len && doc[doc_len - SKIP_ONE - trailing] == '\n') {
8959-
trailing++;
8960-
}
8994+
* breaks the document already ends with. Both the separator and the
8995+
* heading line use the document's OWN ending: counting raw '\n' here would
8996+
* drop a bare newline into a CRLF document, leaving it mixed. */
8997+
const char *eol = adr_dominant_eol(doc, doc_len);
8998+
size_t eol_len = strlen(eol);
8999+
size_t breaks = adr_trailing_breaks(doc, doc_len);
89619000
size_t sep = 0;
8962-
if (doc_len > 0 && trailing < PAIR_LEN) {
8963-
sep = PAIR_LEN - trailing;
9001+
if (doc_len > 0 && breaks < PAIR_LEN) {
9002+
sep = PAIR_LEN - breaks;
89649003
}
89659004
size_t name_len = strlen(name);
8966-
size_t total = doc_len + sep + ST_HEADER_PREFIX + name_len + SKIP_ONE + body_len;
9005+
size_t total = doc_len + (sep * eol_len) + ST_HEADER_PREFIX + name_len + eol_len + body_len;
89679006
out = malloc(total + SKIP_ONE);
89689007
if (!out) {
89699008
return NULL;
89709009
}
8971-
size_t at = 0;
9010+
size_t cursor = 0;
89729011
memcpy(out, doc, doc_len);
8973-
at += doc_len;
9012+
cursor += doc_len;
89749013
for (size_t i = 0; i < sep; i++) {
8975-
out[at++] = '\n';
8976-
}
8977-
memcpy(out + at, "## ", ST_HEADER_PREFIX);
8978-
at += ST_HEADER_PREFIX;
8979-
memcpy(out + at, name, name_len);
8980-
at += name_len;
8981-
out[at++] = '\n';
8982-
memcpy(out + at, body, body_len);
8983-
at += body_len;
8984-
out[at] = '\0';
9014+
memcpy(out + cursor, eol, eol_len);
9015+
cursor += eol_len;
9016+
}
9017+
memcpy(out + cursor, "## ", ST_HEADER_PREFIX);
9018+
cursor += ST_HEADER_PREFIX;
9019+
memcpy(out + cursor, name, name_len);
9020+
cursor += name_len;
9021+
memcpy(out + cursor, eol, eol_len);
9022+
cursor += eol_len;
9023+
memcpy(out + cursor, body, body_len);
9024+
cursor += body_len;
9025+
out[cursor] = '\0';
89859026
return out;
89869027
}
89879028

‎tests/test_store_arch.c‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1011,6 +1011,103 @@ TEST(adr_splice_preserves_untouched_bytes) {
10111011
PASS();
10121012
}
10131013

1014+
/* Line endings. Every other ADR fixture in this file uses \n, so nothing
1015+
* exercised a CRLF-stored document — and CI could not have caught that,
1016+
* because the fixtures are LF whichever platform runs them.
1017+
*
1018+
* The exposure is specific to splicing: the old rebuild normalised everything
1019+
* on the way out, so ending confusion was invisible. A byte-span replacement
1020+
* can cut mid-"\r\n" or miscount a separator run, and byte-identity is the
1021+
* property this design exists to provide.
1022+
*
1023+
* Contract: bytes that already exist are never rewritten, so each line keeps
1024+
* whatever ending it had. Text this code writes itself — the separator before
1025+
* an appended section and the "## NAME" line — follows the document's majority
1026+
* ending, LF on a tie. A caller's body is inserted verbatim; rewriting the
1027+
* bytes a caller supplied would be the same silent modification this whole
1028+
* change removes. */
1029+
TEST(adr_splice_preserves_line_endings) {
1030+
/* CRLF throughout: untouched bytes keep their \r\n. */
1031+
CHECK_SPLICE("## PURPOSE\r\nFoo\r\n\r\n## STACK\r\nBar", "STACK", "New bar",
1032+
"## PURPOSE\r\nFoo\r\n\r\n## STACK\r\nNew bar");
1033+
1034+
/* The separator run before the next heading survives as CRLF, not as the
1035+
* two bare newlines a naive walk-back would leave. */
1036+
CHECK_SPLICE("## A\r\nx\r\n\r\n## B\r\ny", "A", "z", "## A\r\nz\r\n\r\n## B\r\ny");
1037+
1038+
/* A trailing CRLF on the document is preserved as CRLF. */
1039+
CHECK_SPLICE("## A\r\nx\r\n", "A", "z", "## A\r\nz\r\n");
1040+
1041+
/* Appending to a CRLF document writes CRLF — a bare \n here would leave a
1042+
* mixed document that neither the author nor the tool asked for. */
1043+
CHECK_SPLICE("## PURPOSE\r\nFoo", "DECISIONS", "- New.",
1044+
"## PURPOSE\r\nFoo\r\n\r\n## DECISIONS\r\n- New.");
1045+
1046+
/* One existing trailing break means one more is added, counting \r\n as a
1047+
* single break rather than as two characters. */
1048+
CHECK_SPLICE("## PURPOSE\r\nFoo\r\n", "DECISIONS", "- New.",
1049+
"## PURPOSE\r\nFoo\r\n\r\n## DECISIONS\r\n- New.");
1050+
1051+
/* A document already ending in a CRLF blank line gains no extra break. */
1052+
CHECK_SPLICE("## PURPOSE\r\nFoo\r\n\r\n", "DECISIONS", "- New.",
1053+
"## PURPOSE\r\nFoo\r\n\r\n## DECISIONS\r\n- New.");
1054+
1055+
/* LF documents are unaffected by any of the above. */
1056+
CHECK_SPLICE("## PURPOSE\nFoo\n", "DECISIONS", "- New.",
1057+
"## PURPOSE\nFoo\n\n## DECISIONS\n- New.");
1058+
PASS();
1059+
}
1060+
1061+
/* Mixed endings are what real files become through editors and merges. Lines
1062+
* that already exist keep their own endings; the majority decides only what
1063+
* this code writes itself. */
1064+
TEST(adr_splice_mixed_line_endings) {
1065+
/* Majority CRLF (2 of 3) -> the appended block is CRLF, and the one bare
1066+
* LF line in the middle is left exactly as it was. */
1067+
CHECK_SPLICE("## PURPOSE\r\nFoo\nBar\r\n", "DECISIONS", "- New.",
1068+
"## PURPOSE\r\nFoo\nBar\r\n\r\n## DECISIONS\r\n- New.");
1069+
1070+
/* Majority LF (2 of 3) -> the appended block is LF, and the lone CRLF line
1071+
* survives untouched. */
1072+
CHECK_SPLICE("## PURPOSE\nFoo\r\nBar\n", "DECISIONS", "- New.",
1073+
"## PURPOSE\nFoo\r\nBar\n\n## DECISIONS\n- New.");
1074+
1075+
/* A tie falls to LF. */
1076+
CHECK_SPLICE("## PURPOSE\r\nFoo\n", "DECISIONS", "- New.",
1077+
"## PURPOSE\r\nFoo\n\n## DECISIONS\n- New.");
1078+
1079+
/* Replacing a section in a mixed document rewrites nothing around it. */
1080+
CHECK_SPLICE("## PURPOSE\r\nFoo\n\n## STACK\nBar\r\n", "STACK", "New",
1081+
"## PURPOSE\r\nFoo\n\n## STACK\nNew\r\n");
1082+
PASS();
1083+
}
1084+
1085+
/* "## PURPOSE\r\n" must locate exactly as "## PURPOSE\n" does. The scanner
1086+
* trims \r from heading names, but trimming and matching are different claims
1087+
* and only one of them was pinned. */
1088+
TEST(adr_splice_matches_headings_across_line_endings) {
1089+
adr_name_collect_t c;
1090+
memset(&c, 0, sizeof(c));
1091+
ASSERT_EQ(cbm_adr_scan_headings("## PURPOSE\r\nFoo\r\n\r\n## STACK\r\nBar",
1092+
adr_collect_names, &c),
1093+
CBM_STORE_OK);
1094+
ASSERT_STR_EQ(c.buf, "[PURPOSE][STACK]");
1095+
1096+
/* A fenced block with CRLF still hides its heading, and still closes. */
1097+
memset(&c, 0, sizeof(c));
1098+
ASSERT_EQ(cbm_adr_scan_headings("## PURPOSE\r\nFoo\r\n\r\n```md\r\n## Example\r\n```"
1099+
"\r\n\r\n## STACK\r\nBar",
1100+
adr_collect_names, &c),
1101+
CBM_STORE_OK);
1102+
ASSERT_STR_EQ(c.buf, "[PURPOSE][STACK]");
1103+
1104+
/* And an unterminated CRLF fence is still refused. */
1105+
char errbuf[256];
1106+
ASSERT_TRUE(cbm_adr_check_structure("## PURPOSE\r\nFoo\r\n\r\n```\r\nopen\r\n", errbuf,
1107+
sizeof(errbuf)) != CBM_STORE_OK);
1108+
PASS();
1109+
}
1110+
10141111
/* The original use case: add an entry under its own heading. */
10151112
TEST(adr_splice_appends_arbitrary_heading) {
10161113
CHECK_SPLICE("## PURPOSE\nFoo", "DECISIONS", "- Chose SQLite.",
@@ -1620,6 +1717,9 @@ SUITE(store_arch) {
16201717
RUN_TEST(adr_validate_keys_accepts_arbitrary_names);
16211718
RUN_TEST(adr_validate_keys_rejects_unroundtrippable);
16221719
RUN_TEST(adr_splice_preserves_untouched_bytes);
1720+
RUN_TEST(adr_splice_preserves_line_endings);
1721+
RUN_TEST(adr_splice_mixed_line_endings);
1722+
RUN_TEST(adr_splice_matches_headings_across_line_endings);
16231723
RUN_TEST(adr_splice_appends_arbitrary_heading);
16241724
RUN_TEST(adr_splice_is_idempotent);
16251725
RUN_TEST(adr_splice_matches_case_exactly);

0 commit comments

Comments
 (0)