Fix address range difference for line number command #260

LoukasPap · 2026-01-12T15:32:52Z

Increase to 2 supported addresses for line number command (=)
Add 2 integration tests

UPDATE
The PR now solves the problem where invalid POSIX commands with address range were being allowed when they shouldn't. These commands are: a, i, =, l, q, r (maybe there are some more I missed).

github-actions · 2026-01-12T15:35:47Z

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 64 / PASSED: 8 / FAILED: 56 / SKIPPED: 0
  Reference: TOTAL: 64 / PASSED: 8 / FAILED: 56 / SKIPPED: 0

codecov · 2026-01-12T15:36:49Z

Codecov Report

❌ Patch coverage is 64.70588% with 6 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.04%. Comparing base (db58849) to head (dd23a6f).
⚠️ Report is 18 commits behind head on main.

Files with missing lines	Patch %	Lines
src/sed/compiler.rs	57.14%	5 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #260      +/-   ##
==========================================
- Coverage   82.15%   82.04%   -0.11%     
==========================================
  Files          13       13              
  Lines        5324     5337      +13     
  Branches      293      295       +2     
==========================================
+ Hits         4374     4379       +5     
- Misses        948      955       +7     
- Partials        2        3       +1

Flag	Coverage Δ
macos_latest	`82.48% <64.70%> (-0.12%)`	⬇️
ubuntu_latest	`82.60% <64.70%> (-0.11%)`	⬇️
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 Sentry.
📢 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.

github-actions · 2026-01-16T08:23:06Z

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 64 / PASSED: 8 / FAILED: 56 / SKIPPED: 0
  Reference: TOTAL: 64 / PASSED: 8 / FAILED: 56 / SKIPPED: 0

dspinellis · 2026-01-20T14:28:13Z

Thank you! This is a GNU extension, so the feature should not be available under --posix. Please also squash all commits into one.

LoukasPap · 2026-01-20T15:51:15Z

Got it! I'll fix these and push again.

LoukasPap · 2026-01-20T17:32:25Z

Found out that the problem happens with the l (list) command too (we allow address range with --posix enabled but it should not be allowed). Should I fix it here or open a different issue for this?

dspinellis · 2026-01-20T17:56:14Z

I'd suggest one commit for all commands. Try to unify the check for POSIX.

github-actions · 2026-01-21T16:10:01Z

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 64 / PASSED: 15 / FAILED: 49 / SKIPPED: 0
  Reference: TOTAL: 64 / PASSED: 15 / FAILED: 49 / SKIPPED: 0

github-actions · 2026-01-21T16:22:38Z

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 64 / PASSED: 15 / FAILED: 49 / SKIPPED: 0
  Reference: TOTAL: 64 / PASSED: 15 / FAILED: 49 / SKIPPED: 0

github-actions · 2026-01-21T16:30:55Z

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 64 / PASSED: 15 / FAILED: 49 / SKIPPED: 0
  Reference: TOTAL: 64 / PASSED: 15 / FAILED: 49 / SKIPPED: 0

LoukasPap · 2026-01-21T17:11:30Z

Squashed all my commits having removed the merge commit which was not mine.

The POSIX check now happens in one place. I will update the README.md too.

LoukasPap · 2026-01-21T17:19:33Z

src/sed/compiler.rs

        );
    }

+    if context.is_posix_enabled() && is_gnu_extension(ch, n_addr) {


Now that I see it, maybe it would be more readable if we extracted this in a function named e.g. is_invalid_posix_cmd(...) or violates_posix(...).

Consider storing in cmd_spec n_addr_posix in addition to n_addr.

github-actions · 2026-01-21T17:30:33Z

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 64 / PASSED: 15 / FAILED: 49 / SKIPPED: 0
  Reference: TOTAL: 64 / PASSED: 15 / FAILED: 49 / SKIPPED: 0

…flag usage with GNU only extensions - Add a context parameter in get_verified_cmd_spec function and update the relative test cases - Add integration tests for address range

github-actions · 2026-01-22T17:32:19Z

GNU sed testsuite comparison:

Test results comparison:
  Current:   TOTAL: 64 / PASSED: 15 / FAILED: 49 / SKIPPED: 0
  Reference: TOTAL: 64 / PASSED: 15 / FAILED: 49 / SKIPPED: 0

dspinellis · 2026-01-22T17:37:27Z

I see you're progressing! Try running the various CI tasks locally to avoid CI failures. Please also close the PRs that you've folded into this one.

codspeed-hq · 2026-01-22T17:37:58Z

CodSpeed Performance Report

Merging this PR will degrade performance by 3.62%

_{Comparing LoukasPap:main (a73f56d) with main (398ee76)}

Summary

⚡ 1 improved benchmark
❌ 1 regressed benchmark
✅ 9 untouched benchmarks

⚠️ Please fix the performance issues or acknowledge them on CodSpeed.

Performance Changes

	Benchmark	`BASE`	`HEAD`	Efficiency
⚡	`access_log_subst`	1.2 s	1.1 s	+6.17%
❌	`access_log_translit`	784.3 ms	813.7 ms	-3.62%

LoukasPap · 2026-01-22T17:57:21Z

These two tests are failing because the supported addresses are now 2 and not 1 (like the tests assert):

test_lookup_text_command

sed/src/sed/compiler.rs

Lines 1309 to 1314 in 398ee76

    
           #[test] 
        
           fn test_lookup_text_command() { 
        
               let (lines, line) = make_providers("123abc"); 
        
               let cmd = get_cmd_spec(&lines, &line, 'a').unwrap(); 
        
               assert_eq!(cmd.n_addr, 1); 
        
           }

test_valid_command_spec

sed/src/sed/compiler.rs

Lines 1416 to 1425 in 398ee76

    
           #[test] 
        
           fn test_valid_command_spec() { 
        
               let lines = ScriptLineProvider::with_active_state("input.sed", 4); 
        
               let line = char_provider_from("a"); // valid command 
        
               let result = get_verified_cmd_spec(&lines, &line, 1); 
        
               assert!(result.is_ok()); 
        
               let spec = result.unwrap(); 
        
               assert_eq!(spec.n_addr, 1); 
        
           }

Would it be "correct" to change the tests to check for 2 addresses; What I mean is, do these tests represent the standard sed behavior (1 address) and not the 'GNU-extended' behavior (with 2 addresses) and thus changing them would be wrong?

I hope I am clear :)

dspinellis · 2026-01-22T17:59:34Z

src/sed/command.rs


+impl ProcessingContext {
+    /// Check if POSIX extension is enabled
+    pub fn is_posix_enabled(&self) -> bool {


Such trivial accessors are avoided in Rust code.

dspinellis · 2026-01-22T18:04:36Z

I would expect that rather than storing / returning a number you would store

These two tests are failing because the supported addresses are now 2 and not 1 (like the tests assert):
1. `test_lookup_text_command`
   https://github.com/uutils/sed/blob/398ee76b79ba52768c1df214d516b2ad5c173882/src/sed/compiler.rs#L1309-L1314

2. `test_valid_command_spec`
   https://github.com/uutils/sed/blob/398ee76b79ba52768c1df214d516b2ad5c173882/src/sed/compiler.rs#L1416-L1425
Would it be "correct" to change the tests to check for 2 addresses; What I mean is, do these tests represent the standard sed behavior (1 address) and not the 'GNU-extended' behavior (with 2 addresses) and thus changing them would be wrong?

I hope I am clear :)

I suggest that you also store and process n_addr_posix and add corresponding tests.

dspinellis

Please consider the initial comments I made on the code and fix the CI errors. Will then follow-up with a more complete review.

dspinellis · 2026-01-22T18:03:00Z

src/sed/compiler.rs

        );
    }

+    if context.is_posix_enabled() && is_gnu_extension(ch, n_addr) {


Consider storing in cmd_spec n_addr_posix in addition to n_addr.

LoukasPap force-pushed the main branch 2 times, most recently from 81e0901 to 698f2b7 Compare January 21, 2026 17:07

LoukasPap commented Jan 21, 2026

View reviewed changes

LoukasPap force-pushed the main branch from 698f2b7 to dd23a6f Compare January 21, 2026 17:23

LoukasPap mentioned this pull request Jan 22, 2026

Fix address ranges for i and a commands #261

Open

LoukasPap force-pushed the main branch 2 times, most recently from a25ecc9 to d980254 Compare January 22, 2026 17:12

Add address range support for a, i and = and check for invalid POSIX …

a73f56d

…flag usage with GNU only extensions - Add a context parameter in get_verified_cmd_spec function and update the relative test cases - Add integration tests for address range

LoukasPap force-pushed the main branch from d980254 to a73f56d Compare January 22, 2026 17:18

dspinellis reviewed Jan 22, 2026

View reviewed changes

dspinellis requested changes Jan 22, 2026

View reviewed changes

Fix address range difference for line number command #260

Are you sure you want to change the base?

Fix address range difference for line number command #260

Uh oh!

Conversation

LoukasPap commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Jan 12, 2026

Uh oh!

codecov bot commented Jan 12, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Jan 16, 2026

Uh oh!

dspinellis commented Jan 20, 2026

Uh oh!

LoukasPap commented Jan 20, 2026

Uh oh!

LoukasPap commented Jan 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dspinellis commented Jan 20, 2026

Uh oh!

github-actions bot commented Jan 21, 2026

Uh oh!

github-actions bot commented Jan 21, 2026

Uh oh!

github-actions bot commented Jan 21, 2026

Uh oh!

LoukasPap commented Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

LoukasPap Jan 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dspinellis Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Jan 21, 2026

Uh oh!

github-actions bot commented Jan 22, 2026

Uh oh!

dspinellis commented Jan 22, 2026

Uh oh!

codspeed-hq bot commented Jan 22, 2026

CodSpeed Performance Report

Merging this PR will degrade performance by 3.62%

Summary

Performance Changes

Uh oh!

LoukasPap commented Jan 22, 2026

Uh oh!

dspinellis Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

dspinellis commented Jan 22, 2026

Uh oh!

dspinellis left a comment

Choose a reason for hiding this comment

Uh oh!

dspinellis Jan 22, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

LoukasPap commented Jan 12, 2026 •

edited

Loading

codecov bot commented Jan 12, 2026 •

edited

Loading

LoukasPap commented Jan 20, 2026 •

edited

Loading

LoukasPap commented Jan 21, 2026 •

edited

Loading

LoukasPap Jan 21, 2026 •

edited

Loading