Skip to content

fix(formats): keep the padding a txt selector write used to eat - #1190

Merged
BryanFRD merged 1 commit into
mainfrom
fix/selector-write-padding
Sep 24, 2026
Merged

BryanFRD merged 1 commit into
mainfrom
fix/selector-write-padding

Conversation

@BryanFRD

Copy link
Copy Markdown
Contributor

Closes #1186, and the capture-group nit from the review of #1189 that was folded into it.

One span, two callers

The read and the write disagreed about what capture group 1 means. The read trimmed it, the write replaced all of it, so a release through a padded selector ate the padding:

file:     version = 1.2.3
selector: (?m)^version =(.+)$
read:     "1.2.3"
write:    version =2.0.0

Nothing ever reported it, because the selector still matches afterwards. Three releases through v = 1.0.0 would have walked the file to v =1.3.0.

Rather than teaching the write to trim the same way, the two now share select_span, which returns the trimmed byte range. The read slices it, the write splices over it. They cannot drift again, which is the actual defect: the same capture had two meanings in two functions forty lines apart.

The write inherits three things it did not have: the whitespace-only rejection from #1187, the did not participate wording and the TXT_VERSION_NOT_FOUND code from #1189. That is the folded nit, and it came for free rather than as a second patch.

Tests

Five, each verified failing against the previous commit by restoring the old write body with the tests in place:

  • the padding survives a write
  • three consecutive releases do not erode it one character at a time
  • a three-byte whitespace character is not counted as one, so the span arithmetic is byte-correct rather than char-count-correct
  • a group that never participates fails with the code
  • a whitespace-only capture is refused and the file is left untouched

The third is the one the read could not cover. m.start() + leading has to land on a character boundary, and leading is a byte length; with \u{3000} padding, a char-count implementation slices mid-character and panics.

The file contents are built with writeln! and named constants rather than literals carrying newlines and trailing spaces. That is not style: my first draft put the padding at the end of a source line, where a whitespace-stripping hook would have silently weakened the test, which is exactly what the review of #1185 caught the last time I did it.

Not done

The issue asks for a write-side fuzz target. I left it out. Sharing select_span means the write's span is the read's span, and the read is already fuzzed through version_file at ~84k executions a run; a write target would add file I/O per iteration to re-cover arithmetic that is now common code. The part that was genuinely uncovered is the byte-boundary slicing, and that is pinned by a unit test above. If the write path ever grows logic of its own, the target earns its place then.

cargo test: 1083 + 1478. cargo clippy --all-targets -- -D warnings clean.

Docs: the selector description in both schemas, and the mirrored copies under docs/site/data/schema/ regenerated with scripts/embed-release-data.js.

@ferrfleet ferrfleet Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. Collapsing both callers onto select_span is the right shape: the drift was the defect, not the trimming.

Checked the span arithmetic. leading + trailing >= raw.len() is exactly the whitespace-only test: when the capture has a non-whitespace byte the two trimmed runs cannot overlap, so the sum is strictly below raw.len(); when it is all whitespace (or empty) it is 2 * len >= len, which also keeps the empty-capture rejection the old version.is_empty() had. m.start() + leading and m.end() - trailing are on char boundaries because trim_start/trim_end cut there, so the \u{3000} test pins the thing that would actually have panicked.

The write now picking up TXT_VERSION_NOT_FOUND on a non-participating group is a real fix on its own: the old cap.get(1) path returned a bare anyhow! with no code, so that failure was invisible to anything keying on codes.

Docs: I searched for other copies of the selector description and found only the four schema files, all four updated here.

Agreed on skipping the write-side fuzz target. With the span shared there is no write-only arithmetic left to fuzz, and the byte-boundary case is covered.

Nit: "Whitespace inside the capture is trimmed" reads as if internal whitespace is stripped too. "Leading and trailing whitespace inside the capture" would say it. Eight occurrences across four files, so not worth a suggestion block, and not worth a second push on its own.

@BryanFRD
BryanFRD merged commit cc495a2 into main Sep 24, 2026
33 checks passed
@BryanFRD
BryanFRD deleted the fix/selector-write-padding branch September 24, 2026 12:20
ferrflow Bot added a commit that referenced this pull request Sep 24, 2026
## [7.26.3] - 2026-09-24

### Bug Fixes

- fix(formats): keep the padding a txt selector write used to eat (#1190)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(formats): a txt selector write eats the padding the read now trims

1 participant