Skip to content

sed: start a range whose numeric first address was already passed - #581

Open
sap1110 wants to merge 2 commits into
uutils:mainfrom
sap1110:fix-range-late-numeric-start
Open

sap1110 wants to merge 2 commits into
uutils:mainfrom
sap1110:fix-range-late-numeric-start

Conversation

@sap1110

@sap1110 sap1110 commented Sep 28, 2026

Copy link
Copy Markdown

Summary
Fixes sed range handling when the command first runs after the range’s starting address has already been passed.
What changed

  • Start the range when the current line is already past the first address.
  • Keep the range from restarting after it has finished.
  • Preserve cases where both the start and end addresses have already passed.
  • Add tests for nested address blocks and related range edge cases.
  • Align the behavior with GNU sed.

Closes #541

When a range command is first evaluated on a line after its numeric first
address (inside a block, or after d, n, N, c or a branch skipped it), start
the range on the current line, as GNU sed does, unless a numeric second
address has also been passed. The range can start this way only once.

Fixes uutils#541
@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):
  + cmd-R

@codecov

codecov Bot commented Sep 28, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 10.00000% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.62%. Comparing base (a05d743) to head (6efcc06).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/sed/processor.rs 0.00% 9 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #581      +/-   ##
==========================================
- Coverage   83.67%   83.62%   -0.05%     
==========================================
  Files          14       14              
  Lines        7067     7072       +5     
  Branches      413      415       +2     
==========================================
+ Hits         5913     5914       +1     
- Misses       1149     1153       +4     
  Partials        5        5              
Flag Coverage Δ
macos_latest 84.75% <10.00%> (-0.05%) ⬇️
ubuntu_latest 84.95% <10.00%> (-0.05%) ⬇️
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.

Comment thread src/sed/command.rs
pub addr2: Option<Address>, // End address
pub non_select: bool, // True if '!'
pub start_line: Option<usize>, // Start line number (or None if unlatched)
pub range_started: bool, // True once the range has been entered

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.

start_line + range_started could be a small enum (inactive / active(start) / closed), no?
not blocking

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I would be happy to change it now if you prefer

Comment thread src/sed/processor.rs Outdated
// See if latch must start.
if match_address(addr1, reader, pattern, context, &command.location)? {
let starts = match addr1 {
// A numeric first address may already have been passed

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.

5 lines is a lot here, could you please make it shorter?
something like "numeric start already passed (block, d, n, branch): start once here"

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

It's done, the comment is now one line.

Comment thread src/sed/processor.rs
// Check for address-spec line 0 pre-latch extension.
let pre_latched = matches!(cmd.addr1, Some(Address::Line(0)));
cmd.start_line = if pre_latched { Some(0) } else { None };
cmd.range_started = pre_latched;

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.

please add a test where this reset matters, e.g. with -s and two files, so range_started is cleared for the second file

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I added the test it runs the same range on two files with -s and checks that both files print lines 3 and 4, the same as GNU sed

@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):
  + cmd-R

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.

Ranges: a numeric first address that has already been passed never latches

2 participants