Skip to content

sed: implement \\U \\L \\u \\l \\E case-conversion escapes in s/// - #557

Open
dedsec-terminal wants to merge 8 commits into
uutils:mainfrom
dedsec-terminal:fix-540-case-conversion-escapes
Open

dedsec-terminal wants to merge 8 commits into
uutils:mainfrom
dedsec-terminal:fix-540-case-conversion-escapes

Conversation

@dedsec-terminal

@dedsec-terminal dedsec-terminal commented Sep 15, 2026 •

Copy link
Copy Markdown

Fixes #540.

Implements GNU s/// replacement case-conversion escapes:

  • \U uppercase until \L/\E, \L lowercase until \U/\E
  • \u/\l one-shot next-character conversion
  • \E ends persistent conversion and clears pending one-shot

Details:

  • New ReplacementPart::{Upper,Lower,UpperFirst,LowerFirst,End} in command.rs; compile_replacement parses them before parse_char_escape so \u/\U in replacements are case-conversion directives rather than \uXXXX/\UXXXXXXXX Unicode escapes (matches GNU sed behavior; README updated accordingly). Escaped delimiter still wins (e.g. sU...U\UU).
  • Runtime append_with_case applies persistent + one-shot to literals, &, \1..\9. Empty matches leave one-shot pending (GNU s/(b?)-/\u\1x/g carry), any produced char consumes it. Fresh state per substituted occurrence so g does not propagate.
  • Factored out case selection into take_case(single, persistent) helper.
  • UTF-8 mode is Unicode-aware with invalid-byte passthrough; byte mode is ASCII-only.

Verified against GNU sed 4.9 including the issue table (ABC DEF, ABc def) plus \U&\E, \U\1-\2, empty-group g cases, \u\l/\l\u last-wins, \U\l/\L\u combos.

Tests: cargo test --lib 385 passed, cargo test --test tests 268 passed, cargo clippy --lib clean, cargo fmt --check clean.

Copilot AI lite review requested due to automatic review settings September 15, 2026 19:32

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 65 / PASSED: 15 / FAILED: 42 / SKIPPED: 8
  Reference: TOTAL: 65 / PASSED: 14 / FAILED: 43 / SKIPPED: 8

Changes from main branch:
  TOTAL: +0
  PASSED: +1
  FAILED: -1

Test improvements (1):
  + utf8-ru

@codecov

codecov Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 97.71689% with 15 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.03%. Comparing base (f7fb0d7) to head (58b6c03).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/sed/command.rs 98.58% 8 Missing ⚠️
src/sed/processor.rs 0.00% 6 Missing ⚠️
src/sed/compiler.rs 98.80% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #557      +/-   ##
==========================================
+ Coverage   83.86%   85.03%   +1.16%     
==========================================
  Files          14       14              
  Lines        7203     7824     +621     
  Branches      424      448      +24     
==========================================
+ Hits         6041     6653     +612     
- Misses       1157     1166       +9     
  Partials        5        5              
Flag Coverage Δ
macos_latest 86.10% <97.71%> (+1.11%) ⬆️
ubuntu_latest 86.25% <97.71%> (+1.07%) ⬆️
windows_latest 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@codspeed

codspeed Bot commented Sep 16, 2026 •

Copy link
Copy Markdown

Merging this PR will improve performance by 5.87%

⚠️ Different runtime environments detected

Some benchmarks with significant performance changes were compared across different runtime environments,
which may affect the accuracy of the results.

Open the report in CodSpeed to investigate

⚡ 1 improved benchmark
✅ 10 untouched benchmarks

Performance Changes

Benchmark BASE HEAD Efficiency
⚡ number_fix 1.2 s 1.2 s +5.87%

Tip

Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.


Comparing dedsec-terminal:fix-540-case-conversion-escapes (58b6c03) with main (0554c50)

Open in CodSpeed

Copilot AI review requested due to automatic review settings September 16, 2026 17:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Copilot AI review requested due to automatic review settings September 16, 2026 20:19

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@dedsec-terminal

Copy link
Copy Markdown
Author

Hi @dspinellis — friendly review request on this s/// case-conversion implementation (fixes #540; GNU suite +1, utf8-ru). Just pushed a perf follow-up (fast paths when no conversion directives; 265 tests green, fmt+clippy clean) to address the Codspeed delta. Let me know if you need anything else. Thanks!

@github-actions

Copy link
Copy Markdown

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 65 / PASSED: 15 / FAILED: 42 / SKIPPED: 8
  Reference: TOTAL: 65 / PASSED: 14 / FAILED: 43 / SKIPPED: 8

Changes from main branch:
  TOTAL: +0
  PASSED: +1
  FAILED: -1

Test improvements (1):
  + utf8-ru

Copilot AI review requested due to automatic review settings September 17, 2026 18:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@dedsec-terminal

Copy link
Copy Markdown
Author

Follow-up pushed (23b7709): patch coverage for the fast paths - unit tests for unmatched-group/single-literal fast paths, flag-sync contract panics, LowerFirst invalid-byte handling, compiler case-directive parsing + flag, and end-to-end substitution tests; plus removal of two dead None arms preempted by the all-None early return. Full suite green (lib + integration), fmt/clippy clean. Remaining reds are not from this change: Android jobs fail in sdkmanager setup, and CodSpeed shows -3.75% on number_fix only (genome/access_log improved; likely noise, happy to dig if you want).

@github-actions

Copy link
Copy Markdown

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 65 / PASSED: 15 / FAILED: 42 / SKIPPED: 8
  Reference: TOTAL: 65 / PASSED: 14 / FAILED: 43 / SKIPPED: 8

Changes from main branch:
  TOTAL: +0
  PASSED: +1
  FAILED: -1

Test improvements (1):
  + utf8-ru

Comment thread .github/workflows/GnuComment.yml Outdated
Comment thread src/sed/compiler.rs Outdated
Comment thread src/sed/compiler.rs Outdated
Comment thread src/sed/command.rs Outdated
Comment thread src/sed/command.rs
Copilot AI review requested due to automatic review settings September 22, 2026 06:10

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 65 / PASSED: 15 / FAILED: 42 / SKIPPED: 8
  Reference: TOTAL: 65 / PASSED: 14 / FAILED: 43 / SKIPPED: 8

Changes from main branch:
  TOTAL: +0
  PASSED: +1
  FAILED: -1

Test improvements (1):
  + utf8-ru

Copilot AI review requested due to automatic review settings September 22, 2026 08:03

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

Copy link
Copy Markdown

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 65 / PASSED: 15 / FAILED: 42 / SKIPPED: 8
  Reference: TOTAL: 65 / PASSED: 14 / FAILED: 43 / SKIPPED: 8

Changes from main branch:
  TOTAL: +0
  PASSED: +1
  FAILED: -1

Test improvements (1):
  + utf8-ru

Copilot AI review requested due to automatic review settings September 22, 2026 09:26

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

Comment thread src/sed/command.rs Outdated
@github-actions

Copy link
Copy Markdown

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 65 / PASSED: 15 / FAILED: 42 / SKIPPED: 8
  Reference: TOTAL: 65 / PASSED: 14 / FAILED: 43 / SKIPPED: 8

Changes from main branch:
  TOTAL: +0
  PASSED: +1
  FAILED: -1

Test improvements (1):
  + utf8-ru

Comment thread src/sed/command.rs
dedsec-terminal and others added 7 commits October 1, 2026 02:40
Addresses Codspeed -7.9pct regression: common case (no U/L/u/l/E)
now plain memcpy via has_case_conversion flag; byte-mode hoists the
branch; UTF-8 path validates once per segment; ASCII case uses
to_ascii_* fast path. All 265 tests pass; fmt+clippy clean.
Add unit tests for the no-conversion fast paths (unmatched groups, single literals, flag-sync contracts), LowerFirst invalid-byte handling, compiler case-directive parsing and the has_case_conversion flag, plus end-to-end substitution coverage. Collapse two provably-dead None arms preempted by the all-None early return. Deduplicate GNU testsuite bot comments.
- revert unrelated GnuComment workflow modifications
- update README to note \u/\U replacement behavior
- consolidate case escape mapping in compiler
- factor out take_case helper and shorten comments in command.rs
@dedsec-terminal
dedsec-terminal force-pushed the fix-540-case-conversion-escapes branch from 626ce4f to 7fefe2b Compare September 30, 2026 23:57
Copilot AI lite review requested due to automatic review settings September 30, 2026 23:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 65 / PASSED: 19 / FAILED: 38 / SKIPPED: 8
  Reference: TOTAL: 65 / PASSED: 17 / FAILED: 40 / SKIPPED: 8

Changes from main branch:
  TOTAL: +0
  PASSED: +2
  FAILED: -2

Test improvements (2):
  + subst-replacement
  + utf8-ru

@dedsec-terminal
dedsec-terminal force-pushed the fix-540-case-conversion-escapes branch from 7fefe2b to ecd940c Compare October 1, 2026 07:33
Copilot AI lite review requested due to automatic review settings October 1, 2026 07:33
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 65 / PASSED: 19 / FAILED: 38 / SKIPPED: 8
  Reference: TOTAL: 65 / PASSED: 17 / FAILED: 40 / SKIPPED: 8

Changes from main branch:
  TOTAL: +0
  PASSED: +2
  FAILED: -2

Test improvements (2):
  + subst-replacement
  + utf8-ru

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Fix the Unicode lowercasing mismatch and document all five directives and their semantics.

Review effort: Lite
Findings: 1 Medium severity · 1 Low severity

Open (2)

Comment thread src/sed/command.rs
Comment thread docs/src/extensions.md Outdated
Copilot AI lite review requested due to automatic review settings October 1, 2026 07:57
@dedsec-terminal
dedsec-terminal force-pushed the fix-540-case-conversion-escapes branch from ecd940c to 75865ff Compare October 1, 2026 07:57

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟡 Changes recommended

Address malformed UTF-8 handling and POSIX Unicode escape parsing.

Review effort: Lite
Findings: 1 Medium severity

Open (1)
Resolved since last review (2)

Comment thread src/sed/compiler.rs
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 65 / PASSED: 19 / FAILED: 38 / SKIPPED: 8
  Reference: TOTAL: 65 / PASSED: 17 / FAILED: 40 / SKIPPED: 8

Changes from main branch:
  TOTAL: +0
  PASSED: +2
  FAILED: -2

Test improvements (2):
  + subst-replacement
  + utf8-ru

- Encapsulate ReplacementTemplate.has_case_conversion with a private field and public getter

- Replace panic arm in render_parts with unreachable!() and remove artificial panic tests

- Drop backslash for unrecognized escapes in replacement strings to match GNU sed

- Disable GNU case-conversion escapes in replacement strings under POSIX mode

- Add unit and integration regression tests for unrecognized escapes and POSIX mode
@dedsec-terminal
dedsec-terminal force-pushed the fix-540-case-conversion-escapes branch from 75865ff to 58b6c03 Compare October 1, 2026 08:19
Copilot AI lite review requested due to automatic review settings October 1, 2026 08:19
@github-actions

github-actions Bot commented Oct 1, 2026

Copy link
Copy Markdown

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 65 / PASSED: 19 / FAILED: 38 / SKIPPED: 8
  Reference: TOTAL: 65 / PASSED: 17 / FAILED: 40 / SKIPPED: 8

Changes from main branch:
  TOTAL: +0
  PASSED: +2
  FAILED: -2

Test improvements (2):
  + subst-replacement
  + utf8-ru

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Copilot review overview

🟢 Approval recommended

The reviewed changes are fully covered by the supplied compatibility tests and documentation.

Review effort: Lite
Findings: None

Resolved since last review (1)

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.

s/// replacement: implement the \U \L \u \l \E case-conversion escapes

3 participants