fix(graph): solve commit graph sort transitivity issue

[foxfirecodes]

DISCLAIMER: this fix was written by Claude Code. it looks reasonable to me but i am severely lacking in the context of how the commit graph works to know if this solution is right. just sharing this to maybe save you some time, take with a grain of salt

🧢 Changes

this PR solves a sort transitivity issue when constructing the commit graph. it also adds some unit tests to avoid this issue cropping up in the future

☕️ Reasoning

see #12343 for more context, but tl;dr after a force push my commit graph ended up in a state where Git Butler panicked trying to parse. this boils down to a violation of the rules of transitivity when it comes to sorting. i will share the explanation that Claude Code gave me as it does a better job explaining than me lol:

  Root Cause: GenThenTime::cmp() violates transitivity

  The Ord impl at crates/but-graph/src/init/walk.rs:555-563:

  fn cmp(&self, other: &Self) -> Ordering {
      let time_cmp = self.committer_time.cmp(&other.committer_time).reverse();
      match (self.generation, other.generation) {
          (Some(a), Some(b)) => a.cmp(&b).reverse().then(time_cmp),
          _ => time_cmp,  // BUG: ignores generation when only one side has it
      }
  }

  Concrete transitivity violation:
  - A: gen=Some(3), time=200
  - B: gen=None, time=150
  - C: gen=Some(5), time=100
  ┌────────────┬───────────────────────┬──────────────────────────────────┐
  │ Comparison │         Path          │              Result              │
  ├────────────┼───────────────────────┼──────────────────────────────────┤
  │ A vs B     │ mixed gen → time only │ A < B (200 > 150, younger first) │
  ├────────────┼───────────────────────┼──────────────────────────────────┤
  │ B vs C     │ mixed gen → time only │ B < C (150 > 100, younger first) │
  ├────────────┼───────────────────────┼──────────────────────────────────┤
  │ A vs C     │ both Some → gen first │ A > C (gen 5 > 3, younger first) │
  └────────────┴───────────────────────┴──────────────────────────────────┘
  A < B < C but A > C — transitivity broken. Newer Rust sort implementations
  detect this and panic.

  The issue is that when both have generations, generation is primary. But when
  one is None, it falls back to time-only ordering, and these two orderings can
  disagree.

  Fix

  The fix is to give None a consistent sentinel value so generation ordering is
  always total. Using u32::MAX (matching git's GENERATION_NUMBER_INFINITY
  convention) treats unknown-generation commits as "assume youngest, process
  first" — conservative and safe since the sort is an optimization, not a
  correctness requirement.

🎫 Affected issues

Fixes: #12343

[vercel]

@foxfirecodes is attempting to deploy a commit to the GitButler Team on Vercel.

A member of the Team first needs to authorize it.

[Copilot]

Pull request overview

This PR fixes a critical transitivity violation in the commit graph's sorting logic that caused panics during graph traversal after force-pushing. The bug occurred when the comparison function for GenThenTime mixed commits with and without generation numbers, leading to inconsistent ordering that violated Rust's sort contract.

Changes:

• Fixed the GenThenTime::cmp implementation to treat None generation as u32::MAX (matching Git's GENERATION_NUMBER_INFINITY convention), ensuring a total order
• Updated documentation to clearly explain the rationale for using u32::MAX as a sentinel value
• Added comprehensive test coverage for the transitivity fix and various generation/time combinations

[Caleb-T-Owens]

This looks like a reasonable change to me. I've started CI on it.

Out of an abundance of caution I would also like to defer to @Byron before merging since he has the most knowledge of this part of the code.

[Byron]

Thanks a lot for setting up the fix!

I will merge it shortly after a quick hands-on.

[Copilot]

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.

[Byron]

Thanks so much for the fix - I think it's good, too.

Nothing else breaks, the tests actually test for this property and the old version fails them.

Let's bring it in.