Fix truncated pattern space problem

[LoukasPap]

Fix issue 253

• If the hold space is empty, newline is being used
• Also fix problem with --separate
• Add test cases

The solution fixes the problem for the swap x command but also for replace g and append-replace G commands.

[github-actions]

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]

Please fix the failing tests, ensuring the output is compatible with existing sed output. Also ensure and test that the behavior is correct under --separate.

Probably best to wait for the integration of this PR and then add the code in the following block.

        if index == 0 || context.separate {
            context.line_number = 0;
            reset_latched_address_ranges(&mut context.range_commands);
        }

[LoukasPap]

Yes, I'll get back on this when the PR is accepted.

[sylvestre]

@LoukasPap merged!

[LoukasPap]

@sylvestre Thanks for the update! I'll look at it.

[codspeed-hq]

Merging this PR will not alter performance

✅ 11 untouched benchmarks

---

_{Comparing LoukasPap:issue253 (c212acd) with main (ad1f3d2)}

[LoukasPap]

Hello and sorry for the inactivity these days. I am trying to install act so as to run CI checks locally, but I am facing issues with the installation. The problem is in my OS (Windows) where something is not working correctly. I am trying to find a solution to that. I will update you as soon as I start working on the issue again.

Thank you for your patience!

[dspinellis]

@LoukasPap Consider focusing on the test failures, not to replicate the entire CI.

[LoukasPap]

@dspinellis I had tried running some tests alone but I was getting different error messages from the ones in the CI console. For example cargo test pi gives different errors locally and in the GitHub CI. Do I do something wrong?

[dspinellis]

Ensure your files don't contain carriage returns (\r). Consider installing WSL and trying the tests there.

[LoukasPap]

I have WSL. I'll try there.

[sylvestre]

could you please add a test to cover this? thanks

[LoukasPap]

Yes, I'll add today.

[LoukasPap]

Moved to WSL and the tests are showing the correct error messages! I've added a test case and now I will try to fix the failing tests before pushing.

[LoukasPap]

Finally corrected the problem with the newline. I also fixed a same-case problem with g and G commands. I am creating a few tests casing and make the last push. Should I squash the commits?

[dspinellis]

@LoukasPap Please squash related commits together. So each commit should address one case.

[LoukasPap]

Just checked with --separate and is not working correctly so I'll try fix that. The rest are OK I believe.

[codecov]

Codecov Report

❌ Patch coverage is 0% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.95%. Comparing base (ad1f3d2) to head (c212acd).

Files with missing lines	Patch %	Lines
src/sed/processor.rs	0.00%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #263      +/-   ##
==========================================
- Coverage   82.07%   81.95%   -0.13%     
==========================================
  Files          13       13              
  Lines        5445     5453       +8     
  Branches      293      296       +3     
==========================================
  Hits         4469     4469              
- Misses        974      982       +8     
  Partials        2        2              

Flag	Coverage Δ
macos_latest	82.39% <0.00%> (-0.13%)	⬇️
ubuntu_latest	82.51% <0.00%> (-0.13%)	⬇️
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.

[LoukasPap]

Fixed --separate (added test case too). I don't know if it should be moved to another issue & PR. Anyway, I think this PR is now ready for review 🙂

[LoukasPap]

Hello! I resolved conflicts with main branch. Is this OK to merge? (I see we don't have unit tests to cover processor.rs so I didn't add any).

[dspinellis]

Almost there! Please see the one comment and squash the commits into one rebased atop main.

[dspinellis]

Could there two lines be merged to the body of the condition above?

[LoukasPap]

Of course:

if index == 0 || context.separate {
    context.line_number = 0;
    reset_latched_address_ranges(&mut context.range_commands);

    if index > 0 {
        context.hold.content.clear();
        context.hold.has_newline = false;
    }
}

[LoukasPap]

Assuming that index will never be negative, we can remove the index > 0 condition. Deleting this inner condition doesn't affect any passing test.