Skip to content

pr: pr panics (Option::unwrap() on None) in merge mode -m on empty input #12995#13080

Merged
cakebaker merged 4 commits into
uutils:mainfrom
Devel08:fix-issue-12995
Jun 25, 2026
Merged

pr: pr panics (Option::unwrap() on None) in merge mode -m on empty input #12995#13080
cakebaker merged 4 commits into
uutils:mainfrom
Devel08:fix-issue-12995

Conversation

@Devel08

@Devel08 Devel08 commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

This PR resolves #12995
GNU pr exits with silently with code 0, whenever the input in merge mode is an empty file.

@codspeed-hq

codspeed-hq Bot commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown

Merging this PR will improve performance by 9.35%

⚡ 1 improved benchmark
✅ 322 untouched benchmarks
⏩ 46 skipped benchmarks1

Performance Changes

Mode Benchmark BASE HEAD Efficiency
⚡ Memory split_number_chunks 941.3 KB 860.8 KB +9.35%

Tip

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

Comparing Devel08:fix-issue-12995 (ad8d868) with main (cc60f5f)

Open in CodSpeed

Footnotes

  1. 46 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩

@github-actions

github-actions Bot commented Jun 25, 2026 •
edited
Loading

Copy link
Copy Markdown

GNU testsuite comparison:

Skipping an intermittent issue tests/misc/io-errors (passes in this run but fails in the 'main' branch)
Note: The gnu test tests/cut/bounded-memory is now being skipped but was previously passing.
Congrats! The gnu test tests/csplit/csplit-heap is now passing!
Congrats! The gnu test tests/printf/printf-surprise is now passing!

cakebaker
cakebaker reviewed Jun 25, 2026
View reviewed changes
Comment thread src/uu/pr/src/pr.rs Outdated
}
}
}
result.push((current_key.unwrap(), current_group));

@cakebaker cakebaker Jun 25, 2026

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.

The condition on line 1106 is done because of the .unwrap() here. So I would wrap this line with a condition and remove the other condition. Something like:

    if let Some(k) = current_key {
        result.push((k, current_group));
    }

@Devel08 Devel08 Jun 25, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah, looks better, thanks!

cakebaker
cakebaker reviewed Jun 25, 2026
View reviewed changes
Comment thread tests/by-util/test_pr.rs Outdated
new_ucmd!()
.args(&["-m", "/dev/null", "/dev/null"])
.succeeds()
.stdout_is("");

@cakebaker cakebaker Jun 25, 2026

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.

A detail: here you could use .no_output() or .no_stdout() instead of .stdout_is("").

@cakebaker cakebaker merged commit fa2c4b4 into uutils:main Jun 25, 2026
175 checks passed
@cakebaker

cakebaker commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@cakebaker cakebaker cakebaker approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

pr panics (Option::unwrap() on None) in merge mode -m on empty input

2 participants

@Devel08 @cakebaker