From f8df7308cdcb9c7a46069f0778024f38dd2b6a3b Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 18 May 2026 13:15:45 -0700 Subject: [PATCH 1/2] fix: make compare_lines a valid total order (#105) The previous comparator could return Ordering::Less for both compare_lines(a, b) and compare_lines(b, a) when two lines shared a prefix ending just before "**". That violated antisymmetry, so Vec::sort_by produced different orderings on different platforms, breaking "CODEOWNERS out of date" validation. Compare path components left-to-right with "**" treated as a sentinel that sorts before any non-"**" component, falling back to full-line lexical compare as a deterministic tiebreaker. The shared comparator is reused by codeowners_file_parser, so both generation and parse agree. --- src/ownership/file_generator.rs | 86 +++++++++++++++++++++++++++++---- 1 file changed, 77 insertions(+), 9 deletions(-) diff --git a/src/ownership/file_generator.rs b/src/ownership/file_generator.rs index 8878d41..6f39cc8 100644 --- a/src/ownership/file_generator.rs +++ b/src/ownership/file_generator.rs @@ -49,17 +49,37 @@ impl FileGenerator { } pub fn compare_lines(a: &String, b: &String) -> Ordering { - if let Some((prefix, _)) = a.split_once("**") - && b.starts_with(prefix) - { - return Ordering::Less; + let path_a = extract_path(a); + let path_b = extract_path(b); + + let mut comps_a = path_a.split('/'); + let mut comps_b = path_b.split('/'); + + loop { + match (comps_a.next(), comps_b.next()) { + (None, None) => return a.cmp(b), + (None, Some(_)) => return Ordering::Less, + (Some(_), None) => return Ordering::Greater, + (Some(ca), Some(cb)) => match compare_component(ca, cb) { + Ordering::Equal => continue, + ord => return ord, + }, + } } - if let Some((prefix, _)) = b.split_once("**") - && a.starts_with(prefix) - { - return Ordering::Greater; +} + +fn extract_path(line: &str) -> &str { + let stripped = line.strip_prefix("# ").unwrap_or(line); + stripped.split_once(' ').map(|(p, _)| p).unwrap_or(stripped) +} + +fn compare_component(a: &str, b: &str) -> Ordering { + match (a == "**", b == "**") { + (true, true) => Ordering::Equal, + (true, false) => Ordering::Less, + (false, true) => Ordering::Greater, + (false, false) => a.cmp(b), } - a.cmp(b) } #[cfg(test)] @@ -181,6 +201,54 @@ mod tests { assert_eq!(sorted, vec!["/directory/owner1/** @foo", "/directory/owner2/** @bar"]); } + #[test] + fn test_compare_lines_is_antisymmetric_for_shared_double_star_prefix() { + let a = "/foo/**/*bar* @org/example-team".to_string(); + let b = "/foo/**/baz/**/* @org/example-team".to_string(); + + assert_eq!(compare_lines(&a, &b), Ordering::Less); + assert_eq!(compare_lines(&b, &a), Ordering::Greater); + + let mut forward = vec![a.clone(), b.clone()]; + let mut reverse = vec![b.clone(), a.clone()]; + forward.sort_by(compare_lines); + reverse.sort_by(compare_lines); + assert_eq!(forward, reverse); + assert_eq!(forward, vec![a, b]); + } + + #[test] + fn test_compare_lines_total_order_across_special_character_set() { + let lines = vec![ + "/directory/** @bop".to_string(), + "/directory/owner/** @bar".to_string(), + "/directory/owner/(my_folder)/**/** @foo".to_string(), + "/directory/owner/(my_folder)/without_glob @zoo".to_string(), + "/directory/owner/my_folder/** @baz".to_string(), + ]; + for x in &lines { + assert_eq!(compare_lines(x, x), Ordering::Equal); + for y in &lines { + if x == y { + continue; + } + let xy = compare_lines(x, y); + let yx = compare_lines(y, x); + assert_eq!(xy.reverse(), yx, "asymmetric for {x:?} vs {y:?}"); + } + } + } + + #[test] + fn test_compare_lines_ignores_disabled_comment_prefix() { + let enabled = "/foo/owner/** @bar".to_string(); + let disabled = "# /foo/owner/** @bar".to_string(); + let other = "/foo/owner/(extra)/file @baz".to_string(); + + assert_eq!(compare_lines(&enabled, &other), compare_lines(&disabled, &other)); + assert_eq!(compare_lines(&other, &enabled), compare_lines(&other, &disabled)); + } + #[test] fn test_sorting_with_special_characters() { let entries = vec![ From c679de63cf11ac1dfacbb110b8ab14ed32cb60b5 Mon Sep 17 00:00:00 2001 From: Douglas Eichelberger Date: Mon, 18 May 2026 13:23:15 -0700 Subject: [PATCH 2/2] test: prove transitivity and cover edge branches in compare_lines - Rename the antisymmetry sweep so the name matches what it asserts. - Add a transitivity sweep over the same special-character set; this is the property that was actually missing from coverage of the Ord contract. - Add a length-asymmetry case (shorter path vs longer extension) and an equal-path tiebreaker case, exercising the (None, Some) / (Some, None) and full-line fallback arms. --- src/ownership/file_generator.rs | 50 ++++++++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 4 deletions(-) diff --git a/src/ownership/file_generator.rs b/src/ownership/file_generator.rs index 6f39cc8..a73837e 100644 --- a/src/ownership/file_generator.rs +++ b/src/ownership/file_generator.rs @@ -217,15 +217,19 @@ mod tests { assert_eq!(forward, vec![a, b]); } - #[test] - fn test_compare_lines_total_order_across_special_character_set() { - let lines = vec![ + fn special_character_set() -> Vec { + vec![ "/directory/** @bop".to_string(), "/directory/owner/** @bar".to_string(), "/directory/owner/(my_folder)/**/** @foo".to_string(), "/directory/owner/(my_folder)/without_glob @zoo".to_string(), "/directory/owner/my_folder/** @baz".to_string(), - ]; + ] + } + + #[test] + fn test_compare_lines_is_antisymmetric_across_special_character_set() { + let lines = special_character_set(); for x in &lines { assert_eq!(compare_lines(x, x), Ordering::Equal); for y in &lines { @@ -239,6 +243,44 @@ mod tests { } } + #[test] + fn test_compare_lines_is_transitive_across_special_character_set() { + let lines = special_character_set(); + for x in &lines { + for y in &lines { + for z in &lines { + let xy = compare_lines(x, y); + let yz = compare_lines(y, z); + let xz = compare_lines(x, z); + if xy != Ordering::Greater && yz != Ordering::Greater { + assert_ne!(xz, Ordering::Greater, "transitivity broken: {x:?} <= {y:?} <= {z:?} but x > z"); + } + if xy != Ordering::Less && yz != Ordering::Less { + assert_ne!(xz, Ordering::Less, "transitivity broken: {x:?} >= {y:?} >= {z:?} but x < z"); + } + } + } + } + } + + #[test] + fn test_compare_lines_orders_shorter_path_before_longer_extension() { + let shorter = "/foo @a".to_string(); + let longer = "/foo/bar @b".to_string(); + + assert_eq!(compare_lines(&shorter, &longer), Ordering::Less); + assert_eq!(compare_lines(&longer, &shorter), Ordering::Greater); + } + + #[test] + fn test_compare_lines_falls_back_to_full_line_when_paths_equal() { + let a = "/foo/** @a".to_string(); + let b = "/foo/** @b".to_string(); + + assert_eq!(compare_lines(&a, &b), Ordering::Less); + assert_eq!(compare_lines(&b, &a), Ordering::Greater); + } + #[test] fn test_compare_lines_ignores_disabled_comment_prefix() { let enabled = "/foo/owner/** @bar".to_string();