From 0ff00ec44c8b8b124457e201f3ae32d379c00f23 Mon Sep 17 00:00:00 2001 From: John Shin Date: Mon, 26 Jun 2023 16:27:09 -0700 Subject: [PATCH 1/5] uucore: leading zeros are ignored in version compare --- src/uucore/src/lib/mods/version_cmp.rs | 23 ++++++++--------------- 1 file changed, 8 insertions(+), 15 deletions(-) diff --git a/src/uucore/src/lib/mods/version_cmp.rs b/src/uucore/src/lib/mods/version_cmp.rs index 99b8c8b402c..2bbaffd2559 100644 --- a/src/uucore/src/lib/mods/version_cmp.rs +++ b/src/uucore/src/lib/mods/version_cmp.rs @@ -141,8 +141,7 @@ pub fn version_cmp(mut a: &str, mut b: &str) -> Ordering { b = &b[b_numerical_end..]; if a.is_empty() && b.is_empty() { - // Default to the lexical comparison. - return str_cmp; + return std::cmp::Ordering::Equal; } } } @@ -229,14 +228,14 @@ mod tests { // Leading zeroes assert_eq!( version_cmp("012", "12"), - Ordering::Less, - "A single leading zero can make a difference" + Ordering::Equal, + "A single leading zero does not make a difference" ); assert_eq!( version_cmp("000800", "0000800"), - Ordering::Greater, - "Leading number of zeroes is used even if both non-zero number of zeros" + Ordering::Equal, + "Multiple leading zeros do not make a difference" ); // Numbers and other characters combined @@ -280,14 +279,8 @@ mod tests { assert_eq!( version_cmp("aa10aa0022", "aa010aa022"), - Ordering::Greater, - "The leading zeroes of the first number has priority." - ); - - assert_eq!( - version_cmp("aa10aa0022", "aa10aa022"), - Ordering::Less, - "The leading zeroes of other numbers than the first are used." + Ordering::Equal, + "Test multiple numeric values with leading zeros" ); assert_eq!( @@ -307,7 +300,7 @@ mod tests { assert_eq!( version_cmp("aa2000000000000000000000bb", "aa002000000000000000000000bb"), - Ordering::Greater, + Ordering::Equal, "Leading zeroes for numbers larger than u64::MAX are \ handled correctly without crashing" ); From ca315499c3372f51aafb16949f5e7b22e45f5309 Mon Sep 17 00:00:00 2001 From: John Shin Date: Mon, 26 Jun 2023 16:45:57 -0700 Subject: [PATCH 2/5] ls: update test result for version compare --- tests/by-util/test_ls.rs | 49 ++++------------------------------------ 1 file changed, 4 insertions(+), 45 deletions(-) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 1266a7cab92..6814a640ecc 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -2312,56 +2312,15 @@ fn test_ls_version_sort() { let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; for filename in [ - "a2", - "b1", - "b20", - "a1.4", - "a1.40", - "b3", - "b11", - "b20b", - "b20a", - "a100", - "a1.13", - "aa", - "a1", - "aaa", - "a1.00000040", - "abab", - "ab", - "a01.40", - "a001.001", - "a01.0000001", - "a01.001", - "a001.01", + "a2", "b1", "b20", "a1.4", "b3", "b11", "b20b", "b20a", "a100", "a1.13", "aa", "a1", "aaa", + "abab", "ab", "a01.40", "a001.001", ] { at.touch(filename); } let mut expected = vec![ - "a1", - "a001.001", - "a001.01", - "a01.0000001", - "a01.001", - "a1.4", - "a1.13", - "a01.40", - "a1.00000040", - "a1.40", - "a2", - "a100", - "aa", - "aaa", - "ab", - "abab", - "b1", - "b3", - "b11", - "b20", - "b20a", - "b20b", - "", // because of '\n' at the end of the output + "a1", "a001.001", "a1.4", "a1.13", "a01.40", "a2", "a100", "aa", "aaa", "ab", "abab", "b1", + "b3", "b11", "b20", "b20a", "b20b", "", // because of '\n' at the end of the output ]; let result = scene.ucmd().arg("-1v").succeeds(); From d2e7ba2da1abe2bb2c1af710a36b50b2b8b50842 Mon Sep 17 00:00:00 2001 From: John Shin Date: Tue, 27 Jun 2023 12:15:58 -0700 Subject: [PATCH 3/5] sort: add tests for stable and unstable sort --- src/uucore/src/lib/mods/version_cmp.rs | 8 +++----- tests/by-util/test_sort.rs | 19 +++++++++++++++++++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/uucore/src/lib/mods/version_cmp.rs b/src/uucore/src/lib/mods/version_cmp.rs index 2bbaffd2559..828b7f2a668 100644 --- a/src/uucore/src/lib/mods/version_cmp.rs +++ b/src/uucore/src/lib/mods/version_cmp.rs @@ -106,7 +106,7 @@ pub fn version_cmp(mut a: &str, mut b: &str) -> Ordering { // 1. Compare leading non-numerical part // 2. Compare leading numerical part // 3. Repeat - loop { + while !a.is_empty() || !b.is_empty() { let a_numerical_start = a.find(|c: char| c.is_ascii_digit()).unwrap_or(a.len()); let b_numerical_start = b.find(|c: char| c.is_ascii_digit()).unwrap_or(b.len()); @@ -139,11 +139,9 @@ pub fn version_cmp(mut a: &str, mut b: &str) -> Ordering { a = &a[a_numerical_end..]; b = &b[b_numerical_end..]; - - if a.is_empty() && b.is_empty() { - return std::cmp::Ordering::Equal; - } } + + Ordering::Equal } #[cfg(test)] diff --git a/tests/by-util/test_sort.rs b/tests/by-util/test_sort.rs index e66a405abbb..0c8af8969f2 100644 --- a/tests/by-util/test_sort.rs +++ b/tests/by-util/test_sort.rs @@ -134,6 +134,25 @@ fn test_version_empty_lines() { test_helper("version-empty-lines", &["-V", "--version-sort"]); } +#[test] +fn test_version_sort_unstable() { + new_ucmd!() + .arg("--sort=version") + .pipe_in("0.1\n0.02\n0.2\n0.002\n0.3\n") + .succeeds() + .stdout_is("0.1\n0.002\n0.02\n0.2\n0.3\n"); +} + +#[test] +fn test_version_sort_stable() { + new_ucmd!() + .arg("--stable") + .arg("--sort=version") + .pipe_in("0.1\n0.02\n0.2\n0.002\n0.3\n") + .succeeds() + .stdout_is("0.1\n0.02\n0.2\n0.002\n0.3\n"); +} + #[test] fn test_human_numeric_whitespace() { test_helper( From 4acd02c7a1cac5762ab301b4724fa498a9c0ff53 Mon Sep 17 00:00:00 2001 From: John Shin Date: Tue, 4 Jul 2023 16:08:54 -0700 Subject: [PATCH 4/5] ls: string compare if version_cmp returns equal --- src/uu/ls/src/ls.rs | 6 +++-- tests/by-util/test_ls.rs | 49 ++++++++++++++++++++++++++++++++++++---- 2 files changed, 49 insertions(+), 6 deletions(-) diff --git a/src/uu/ls/src/ls.rs b/src/uu/ls/src/ls.rs index 7db591cf3b7..a5438f518eb 100644 --- a/src/uu/ls/src/ls.rs +++ b/src/uu/ls/src/ls.rs @@ -1931,8 +1931,10 @@ fn sort_entries(entries: &mut [PathData], config: &Config, out: &mut BufWriter entries.sort_by_key(|k| Reverse(k.md(out).map(|md| md.len()).unwrap_or(0))), // The default sort in GNU ls is case insensitive Sort::Name => entries.sort_by(|a, b| a.display_name.cmp(&b.display_name)), - Sort::Version => entries - .sort_by(|a, b| version_cmp(&a.p_buf.to_string_lossy(), &b.p_buf.to_string_lossy())), + Sort::Version => entries.sort_by(|a, b| { + version_cmp(&a.p_buf.to_string_lossy(), &b.p_buf.to_string_lossy()) + .then(a.p_buf.to_string_lossy().cmp(&b.p_buf.to_string_lossy())) + }), Sort::Extension => entries.sort_by(|a, b| { a.p_buf .extension() diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index 6814a640ecc..1266a7cab92 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -2312,15 +2312,56 @@ fn test_ls_version_sort() { let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; for filename in [ - "a2", "b1", "b20", "a1.4", "b3", "b11", "b20b", "b20a", "a100", "a1.13", "aa", "a1", "aaa", - "abab", "ab", "a01.40", "a001.001", + "a2", + "b1", + "b20", + "a1.4", + "a1.40", + "b3", + "b11", + "b20b", + "b20a", + "a100", + "a1.13", + "aa", + "a1", + "aaa", + "a1.00000040", + "abab", + "ab", + "a01.40", + "a001.001", + "a01.0000001", + "a01.001", + "a001.01", ] { at.touch(filename); } let mut expected = vec![ - "a1", "a001.001", "a1.4", "a1.13", "a01.40", "a2", "a100", "aa", "aaa", "ab", "abab", "b1", - "b3", "b11", "b20", "b20a", "b20b", "", // because of '\n' at the end of the output + "a1", + "a001.001", + "a001.01", + "a01.0000001", + "a01.001", + "a1.4", + "a1.13", + "a01.40", + "a1.00000040", + "a1.40", + "a2", + "a100", + "aa", + "aaa", + "ab", + "abab", + "b1", + "b3", + "b11", + "b20", + "b20a", + "b20b", + "", // because of '\n' at the end of the output ]; let result = scene.ucmd().arg("-1v").succeeds(); From 4db5a60667005ddc48235463f84ef342776a6014 Mon Sep 17 00:00:00 2001 From: John Shin Date: Sun, 9 Jul 2023 15:29:42 -0700 Subject: [PATCH 5/5] ls: add back version cmp test --- tests/by-util/test_ls.rs | 49 ++++++++++++++++++++++++++++++++++++---- 1 file changed, 45 insertions(+), 4 deletions(-) diff --git a/tests/by-util/test_ls.rs b/tests/by-util/test_ls.rs index a6835f279d3..2eef6912162 100644 --- a/tests/by-util/test_ls.rs +++ b/tests/by-util/test_ls.rs @@ -2311,15 +2311,56 @@ fn test_ls_version_sort() { let scene = TestScenario::new(util_name!()); let at = &scene.fixtures; for filename in [ - "a2", "b1", "b20", "a1.4", "b3", "b11", "b20b", "b20a", "a100", "a1.13", "aa", "a1", "aaa", - "abab", "ab", "a01.40", "a001.001", + "a2", + "b1", + "b20", + "a1.4", + "a1.40", + "b3", + "b11", + "b20b", + "b20a", + "a100", + "a1.13", + "aa", + "a1", + "aaa", + "a1.00000040", + "abab", + "ab", + "a01.40", + "a001.001", + "a01.0000001", + "a01.001", + "a001.01", ] { at.touch(filename); } let mut expected = vec![ - "a1", "a001.001", "a1.4", "a1.13", "a01.40", "a2", "a100", "aa", "aaa", "ab", "abab", "b1", - "b3", "b11", "b20", "b20a", "b20b", "", // because of '\n' at the end of the output + "a1", + "a001.001", + "a001.01", + "a01.0000001", + "a01.001", + "a1.4", + "a1.13", + "a01.40", + "a1.00000040", + "a1.40", + "a2", + "a100", + "aa", + "aaa", + "ab", + "abab", + "b1", + "b3", + "b11", + "b20", + "b20a", + "b20b", + "", // because of '\n' at the end of the output ]; let result = scene.ucmd().arg("-1v").succeeds();