change(gix-blame)!: make invalid BlameRanges unrepresentable

[Ivan Žužak (izuzak)]

Hello! 👋 This is my first PR to this project, so I'm going to do a brief introduction: my name is Ivan, and ~2 years ago I built a small TUI app for exploring Git blames (nothing popular, just a personal project). The app is written in Rust and uses command-line Git for all Git information extraction/querying. I'd like to try replacing that direct Git usage with gitoxide, specifically gix-blame. And as a part of that, I'm hoping to contribute a bit to the gitoxide project.

---

Okay, now about this PR. Note: I did write most of the code and PR description myself (my issues/PRs are often detailed+long -- sorry! 🙈), but also used an AI agent for a final review and edit -- it caught a few small problems both in my changes and in pre-existing code, and also added a few notes to the commit messages and code comments.

TL;DR version since the full explanation is a bit long

Currently, it's possible to construct BlameRanges::PartialFile values with empty internal ranges (e.g. a reversed range 2..1). The gix_blame::file() function takes a BlameRanges value as input and eventually attempts to create a BlameEntry. Attempting to create a BlameEntry from an empty range violates a pre-existing internal invariant and causes a panic. Directly constructed overlapping or duplicate ranges can also silently produce duplicate blame entries.

With the changes in this PR, the constructors and the add_one_based_inclusive_range() method reject reversed one-based ranges (which were previously converted into empty internal ranges) and return the existing InvalidOneBasedLineRange error. BlameRanges also becomes an opaque struct, so callers can no longer bypass those methods to create or mutate invalid selections directly. It either selects the whole file or holds a non-empty collection of non-empty, sorted and disjoint ranges.

This is a breaking change: the public BlameRanges enum variants were removed, and to_zero_based_exclusive_ranges() is now crate-private to reduce public API surface. The existing constructors remain, and two new read-only accessors (is_whole_file() and selected_ranges()) allow callers to inspect the internal BlameRanges selections.

And here's the longer version below. 📚

The problem I observed

I noticed that the gix blame command line wrapper (correctly) rejects reversed ranges (e.g. -L 2,1 instead of -L 1,2):

> gix blame -L 2,1 gix-blame/tests/blame.rs

error: invalid value '2,1' for '-L <RANGES>': error: invalid value for one of the arguments

> gix blame -L 1,2 gix-blame/tests/blame.rs

d2e98f3c 1 gix-blame/tests/blame.rs 1 use std::{collections::BTreeMap, path::PathBuf};
26bfd2d7 2 gix-blame/tests/blame.rs 2

However, there's no such validation for reversed ranges in the API and trying to use such ranges leads to panics. Specifically:

• The private BlameRanges::inclusive_to_zero_based_exclusive method only checks that the start of the range is greater than 0. This method is called from the public BlameRanges::from_one_based_inclusive_range, BlameRanges::from_one_based_inclusive_ranges and BlameRanges::add_one_based_inclusive_range methods used for creating and modifying BlameRanges::PartialFile values. So, a BlameRanges::PartialFile value with an empty internal range can be created here, e.g. BlameRanges::from_one_based_inclusive_range(2..=1) results in a 1..1 range.
• On top of that, a BlameRanges::PartialFile value with a reversed/empty range can also be created directly since the enum and variants are public, e.g. PartialFile(vec![2..1]).

In both cases, a BlameRanges value is passed to gix_blame::file() as input, and gix_blame::file() doesn't validate the included ranges. Things eventually lead to a BlameEntry being created and as a part of that creation -- force_non_zero() is called with the range's length which then calls NonZeroU32::new(n).expect(...). For an empty range, the range's length n is 0, causing NonZeroU32::new(n) to return None and thus the expect() call panics with BUG: hunks are never empty.

While looking at direct creation of BlameRanges::PartialFile values, I also found a few other problems:

• PartialFile(vec![0..3, 1..4]) succeeds, but blames lines 2 and 3 twice.
• PartialFile(vec![0..2, 0..2]) succeeds, but duplicates every entry.
• PartialFile(vec![]) succeeds and blames nothing, while from_one_based_inclusive_ranges(vec![]) blames everything.

The BlameRanges constructors already merge overlapping and adjacent ranges and sort the result, but direct construction bypasses all of that.

I tried searching prior issues and PRs for reports of this reversed-range panic, but couldn't find any. I did find #1766 which added gix blame -L start,end and the start <= end validation for the CLI call. I also found #1976 where the range.start() == 0 check was suggested (and finally implemented in the follow-up #2204), but it doesn't seem there were discussions about the start > end or RangeInclusive::is_empty() cases.

The solution in this PR

Initially, I thought "I'll just add a bit of validation in a few places", and even implemented that approach. But as I was writing up the PR for that change -- it occurred to me that preventing invalid BlameRanges from being constructed in the first place (the old "parse, don't validate" idea) might be a more robust and durable approach, even though it would be a breaking change.

So, that "make invalid BlameRanges unrepresentable" approach is what's in this PR and I split the changes into three commits:

1. 9390c8a - Reject reversed one-based ranges in BlameRanges::inclusive_to_zero_based_exclusive(), returning the existing Error::InvalidOneBasedLineRange. This covers both constructors and add_one_based_inclusive_range().
2. fd1aad3 - Replace the public BlameRanges enum with an opaque struct over a private Selection enum. All construction and modification of selected ranges go through validation and the existing merge_zero_based_exclusive_range() logic. The result is either the whole file or a non-empty collection of non-empty, sorted, disjoint and non-adjacent ranges. Also, add read-only accessors (is_whole_file() and selected_ranges()) and make to_zero_based_exclusive_ranges() crate-private (a few notes about this change are in a separate section below).
3. 3dac5d6 - Change the now-private to_zero_based_exclusive_ranges() method's max_lines parameter from u32 to NonZeroU32. Previously, resolving a whole-file selection with 0 produced vec![0..0], another empty range. gix_blame::file() already returned early for content without lines, so it didn't hit this case. But the new parameter type makes handling 0 a requirement for calling the resolver (which should be more robust).

Regression tests cover reversed-range rejection (including unchanged state on error), normalized range inspection, and non-empty whole-file resolution. An integration test also checks that blaming empty untracked content returns no entries.

The expect() in force_non_zero() stays since a BlameEntry without lines is still a bug. This PR prevents such ranges from being created and passed to gix_blame::file().

If the breaking changes feel too disruptive or you don't like that approach in general, I'm happy to go back to the validation approach or something else based on your feedback. (That said, in my first attempt -- the validation approach also introduced a small breaking change due to a new variant in an exhaustive Error enum.)

Also, one trade-off is that callers who previously constructed BlameRanges::PartialFile manually from already-normalized ranges must now go through the merge_zero_based_exclusive_range() normalization too. This will obviously have worse performance, so if this is a concern -- optimizing bulk construction could be something to discuss.

Breaking changes and migration paths

One breaking change is that the BlameRanges::WholeFile and BlameRanges::PartialFile enum variants are no longer public.

• To select the whole file, use BlameRanges::default().
• To select ranges, use the existing from_one_based_inclusive_range(), from_one_based_inclusive_ranges() and add_one_based_inclusive_range() methods.
• To inspect a selection, use is_whole_file() or selected_ranges(). The latter returns Option<&[Range<u32>]>: None for the whole file, or a borrowed slice of normalized 0-based exclusive ranges.
• To select ranges based on non-empty 0-based exclusive ranges, convert each start..end to the 1-based inclusive (start + 1)..=end before passing it to a constructor. If this feels like a common use-case, we might offer new constructors for that as well?

The other breaking change is that to_zero_based_exclusive_ranges() is no longer pub but rather pub(crate).

• To inspect a selection, use selected_ranges() and is_whole_file() instead.
• To blame it, pass the selection through Options::ranges to gix_blame::file(), which resolves it internally.
• There is no public replacement for obtaining file-clamped ranges without running blame -- callers would need to do it themselves. See more notes below.

Why make the to_zero_based_exclusive_ranges() range resolution internal?

TLDR: changing the to_zero_based_exclusive_ranges() method from pub to pub(crate) reduces the public API surface since it felt like it didn't need to be public. If this doesn't feel helpful, I can remove that change.

to_zero_based_exclusive_ranges() does more than expose the stored ranges: it resolves a selection against the line count of a particular file, clamping ranges that reach past its end and dropping those that start past it. That behavior is unchanged. (Also note: for ordered positive ranges, Git also clamps ends past EOF, but errors on starts past EOF rather than dropping those ranges.)

gix_blame::file() already obtains the actual content and counts its lines. Callers could compute that count themselves, but would need the same content and line-counting rules -- it isn't something known from the selection alone. The new accessors let callers inspect a selection without a line count.

Also, the PR which introduced this method removed the previous version of it called to_zero_based_exclusive() which was also pub. But that method was documented as "used internally by the blame algorithm" implying that this method is primarily there for internal use and perhaps could have been pub(crate) from the start. And as far as I could tell, there's no external usage of this method in public repositories. Of course, there could be some usage in private repositories.

What does Git do for reversed ranges?

While I was working on this, I checked what command-line git blame does and -- it actually behaves differently! Instead of rejecting reversed ranges, it swaps them for you:

> git blame -L 1,2 gix-blame/tests/blame.rs
d2e98f3cf4 (Christoph Rüßler 2025-05-22 16:00:49 +0200 1) use std::{collections::BTreeMap, path::PathBuf};
26bfd2d733 (Byron            2024-12-23 17:29:28 +0100 2)

> git blame -L 2,1 gix-blame/tests/blame.rs
d2e98f3cf4 (Christoph Rüßler 2025-05-22 16:00:49 +0200 1) use std::{collections::BTreeMap, path::PathBuf};
26bfd2d733 (Byron            2024-12-23 17:29:28 +0100 2)

The behavior comes from here and there's even a regression test for it, but that regression test explicitly classifies this as undocumented behavior. Also, the git blame docs only say that the two numbers should be 1-based and don't mention the swapping:

-L <start>,<end>
  ...
  If <start> or <end> is a number, it specifies an absolute line number (lines count from 1).

So, technically, gix_blame::file() is not matching Git's behavior with respect to reversed ranges, but on the other hand -- Git's swapping behavior is explicitly marked as undocumented and the gix blame CLI command already rejects reversed ranges. So, I think rejecting them in the API also makes more sense than silently swapping them.

Thanks for reading my novel! 🙇

[Sebastian Thiel (Byron)]

Hi Ivan Žužak (@izuzak), it's great having you, and of course, to talk to a human for a change :D!

I am looking forward to seeing you contribute more as you will make the implementation more suitable for the TUI of yours, as that's exactly what I thought gix-blame should support very well one day, including incremental blaming, caching, and progress + interruptibility.

With that said, I read the tl;dr and think it's a great first improvement to see :).
Let's give Christoph Rüßler (@cruessler) some time to take a look as well, and I will do the final review once he gave it a look.

[Christoph Rüßler (cruessler)]

Thanks a lot, in particular for the detailed description! I’ll have a look and hope to be able to provide a first review in a couple of days!