Add legacy +POS/-POS handling in sort to pass GNU sort-field-limit test #9501

karanabe · 2025-11-27T23:07:59Z

This change translates GNU’s legacy +POS1 [-POS2] sort key syntax into -k form before clap parsing, gating it by _POSIX2_VERSION to match GNU behavior, and adds by-util tests to ensure acceptance/rejection behavior matches GNU and that the sort-field-limit compatibility test passes.

#9127

Support GNU’s obsolescent +POS1 [-POS2] syntax by translating it to -k before clap parses args, gated by _POSIX2_VERSION. Adds tests for accept and reject cases to ensure sort-field-limit GNU test passes.

github-actions · 2025-11-27T23:23:14Z

GNU testsuite comparison:

Congrats! The gnu test tests/sort/sort-field-limit is no longer failing!

tests/by-util/test_sort.rs

github-actions · 2025-12-01T17:10:24Z

GNU testsuite comparison:

Skip an intermittent issue tests/tail/overlay-headers (fails in this run but passes in the 'main' branch)
Congrats! The gnu test tests/sort/sort-field-limit is no longer failing!

cakebaker · 2025-12-09T09:39:18Z

src/uu/sort/src/sort.rs

+    let mut idx = 0;
+    let bytes = spec.as_bytes();
+    while idx < bytes.len() && bytes[idx].is_ascii_digit() {
+        idx += 1;
+    }


I think a functional approach is cleaner. Something like:

Suggested change

let mut idx = 0;

let bytes = spec.as_bytes();

while idx < bytes.len() && bytes[idx].is_ascii_digit() {

idx += 1;

}

let idx = spec.chars().take_while(|c| c.is_ascii_digit()).count();

This was fixed in this commit. 3b52b5d

cakebaker · 2025-12-09T09:43:52Z

src/uu/sort/src/sort.rs

+        let mut char_idx = 0;
+        let stripped_bytes = stripped.as_bytes();
+        while char_idx < stripped_bytes.len() && stripped_bytes[char_idx].is_ascii_digit() {
+            char_idx += 1;
+        }


Same here.

Suggested change

let mut char_idx = 0;

let stripped_bytes = stripped.as_bytes();

while char_idx < stripped_bytes.len() && stripped_bytes[char_idx].is_ascii_digit() {

char_idx += 1;

}

let char_idx = stripped.chars().take_while(|c| c.is_ascii_digit()).count();

This was fixed in this commit. 3b52b5d

cakebaker · 2025-12-09T10:06:33Z

src/uu/sort/src/sort.rs

+#[derive(Debug, Clone)]
+struct LegacyKeyPart {
+    field: usize,
+    char: usize,


The name char is a bit unlucky.

This was fixed in this commit. 3b52b5d

cakebaker · 2025-12-09T14:26:12Z

src/uu/sort/src/sort.rs

+            to.field.saturating_add(1)
+        };
+
+        let mut end_part = end_field.to_string();


What's the reason for introducing end_part? Couldn't you reorder things a bit and push directly to keydef?

This was fixed in this commit. bf7a8ab

cakebaker · 2025-12-09T15:35:47Z

src/uu/sort/src/sort.rs

+    }
+}
+
+fn parse_legacy_part(spec: &str, default_char: usize) -> Option<LegacyKeyPart> {


Is the default_char param necessary? It's 0 in both places this function is called.

This was fixed in this commit. 3b52b5d

cakebaker · 2025-12-09T15:47:29Z

src/uu/sort/src/sort.rs

+                        if stripped
+                            .as_bytes()
+                            .first()
+                            .is_some_and(|c| c.is_ascii_digit())
+                        {


I think you can simplify it with starts_with:

Suggested change

if stripped

.as_bytes()

.first()

.is_some_and(|c| c.is_ascii_digit())

{

if stripped.starts_with(|c: char| c.is_ascii_digit()) {

This was fixed in this commit. e477de9

codspeed-hq · 2025-12-14T07:41:33Z

CodSpeed Performance Report

Merging #9501 will degrade performances by 4.51%

_{Comparing karanabe:test/fix-sort-field-limit (a10c06e) with main (64203e3)}

Summary

⚡ 1 improvement
❌ 2 regressions
✅ 124 untouched
⏩ 6 skipped¹

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

Benchmarks breakdown

	Benchmark	`BASE`	`HEAD`	Change
❌	`sort_ascii_c_locale`	21.6 ms	22.6 ms	-4.51%
⚡	`sort_numeric`	24 ms	23.5 ms	+2.04%
❌	`tsort_input_parsing_heavy[5000]`	82.1 ms	83.8 ms	-2.04%

6 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 · 2025-12-14T07:43:15Z

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/sort/sort-field-limit is no longer failing!

github-actions · 2025-12-14T08:36:22Z

GNU testsuite comparison:

Skipping an intermittent issue tests/tail/overlay-headers (passes in this run but fails in the 'main' branch)
Congrats! The gnu test tests/sort/sort-field-limit is no longer failing!

karanabe · 2025-12-14T11:48:52Z

@cakebaker Thanks for the review! I've addressed all the comments.
Regarding the test failure, test_tail::test_retry6 stdout does not seem to be related to this change and appears to be failing elsewhere as well.
Could you please take another look?

cakebaker · 2025-12-15T09:00:11Z

Congrats! The gnu test tests/sort/sort-field-limit is no longer failing!

Kudos, thanks!

sort: add legacy +POS/-POS parsing for GNU compat

287815f

Support GNU’s obsolescent +POS1 [-POS2] syntax by translating it to -k before clap parses args, gated by _POSIX2_VERSION. Adds tests for accept and reject cases to ensure sort-field-limit GNU test passes.

cakebaker reviewed Nov 28, 2025

View reviewed changes

tests/by-util/test_sort.rs Outdated Show resolved Hide resolved

sort: align legacy key tests with GNU field limit

6656199

cakebaker reviewed Dec 1, 2025

View reviewed changes

tests/by-util/test_sort.rs Outdated Show resolved Hide resolved

sort: rename legacy max-field test for clarity

335b6e4

karanabe requested a review from cakebaker December 1, 2025 17:00

cakebaker reviewed Dec 9, 2025

View reviewed changes

karanabe added 3 commits December 14, 2025 16:24

Simplify legacy key parsing inputs

3b52b5d

Inline legacy key end serialization

bf7a8ab

Use starts_with for legacy arg digit check

e477de9

Merge branch 'main' into test/fix-sort-field-limit

a10c06e

karanabe requested a review from cakebaker December 14, 2025 11:48

cakebaker merged commit 06d843f into uutils:main Dec 15, 2025
125 of 127 checks passed

cakebaker mentioned this pull request Dec 15, 2025

GNU coreutils 9.9: Test report #9127

Open

Uh oh!

Add legacy +POS/-POS handling in sort to pass GNU sort-field-limit test #9501

Add legacy +POS/-POS handling in sort to pass GNU sort-field-limit test #9501

Uh oh!

Conversation

karanabe commented Nov 27, 2025

Uh oh!

github-actions bot commented Nov 27, 2025

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Dec 1, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codspeed-hq bot commented Dec 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Merging #9501 will degrade performances by 4.51%

Summary

Benchmarks breakdown

Footnotes

Uh oh!

github-actions bot commented Dec 14, 2025

Uh oh!

github-actions bot commented Dec 14, 2025

Uh oh!

karanabe commented Dec 14, 2025

Uh oh!

Uh oh!

cakebaker commented Dec 15, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codspeed-hq bot commented Dec 14, 2025 •

edited

Loading