fix: make compare_lines a valid total order (#105) (#106)

* 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.

* 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.

Author: dduugg

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,96 @@ 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]);
+    }
+
+    fn special_character_set() -> Vec<String> {
+        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 {
+                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_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();
+        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![