Improve support for multiple blame ranges

[holodorum]

This PR implements various improvements suggested in PR #1973.

The main change is converting the BlameRanges struct, into an enum to support both OneBasedInclusive and ZeroBasedExclusive range formats. The FullFile variant, denotes that the entire file is to be blamed, serves as a more explicit substitute for a previously used "empty" range.

[Byron]

Thanks a lot for the follow-up!

@cruessler would probably be the one to do the first review, and I will get it merged right after. Thanks everyone

[cruessler]

Thanks a lot for the follow-up!

Before I start reviewing this PR in detail, I have a few questions regarding the high-level API. In particular, it seems as if the decision to change to_zero_based_exclusive(&self, max_lines: u32) -> Result<Vec<Range<u32>>, Error> to to_zero_based_exclusive(&self, max_lines: u32) -> Result<BlameRanges, Error> brings a couple of downsides with it. My main concern is that it leaks implementation details: the caller now has to know about BlameRanges’ internals while that was previously not the case. Also, two of BlameRanges’ methods are now fallible, forcing the caller to deal with errors that are coupled to the struct’s internals.

I think the proposed API can be simplified and stay more insulated from calling code by reducing the number of enum variants to WholeFile and PartialFile (or something more appropriately named) and staying with to_zero_based_exclusive(&self, max_lines: u32) -> Result<Vec<Range<u32>>, Error>. We could even go so far as to stay with the existing design that treats an empty ranges as covering the whole file.

What do you think?

[holodorum]

As discussed with @cruessler during a call, we decided to make the API more robust and less error-prone.

We've removed the distinction between BlameRanges:OneBasedInclusive and BlameRanges:ZeroBasedExclusive. Instead, we now only support two variants: PartialFile and WholeFile. Internally, BlameRanges always uses zero-based, exclusive ranges.

This means users no longer need to worry about converting between one-based and zero-based ranges. If a non-inclusive range is used during construction, an error will be thrown, helping to prevent subtle bugs.

[holodorum]

Bit surprised about the CI-error. Any hint for that?

[Byron]

I restarted the job, and I'd expect it to go through. There is some known flakiness with tests that deal with concurrent IO, and even though it's quite rare, it happens, unfortunately.

[EliahKagan]

The original failure in the test-fast job on macos-latest was due to #1816. That issue is the only current source of regular flakiness that I am aware of. I have not been keeping track of most occurrences of it, but lately I have observed such a failure every couple of days. (The other source of flakiness I am aware of is #2006, but that is not a regular source of flakiness--it seems to happen once or twice a year when not deliberately induced, and in any case it is not what happened here. If there are more known sources of CI flakiness, then I would be interested to learn of them, with the hope of helping out with them as well.)

Because the test-fast jobs are defined by a matrix with an implicit fail-fast: true strategy (i.e., fail-fast: false is not specified), any test-fast job failure will tell whatever other test-fast jobs are still running to cancel. It looks like you reran only the failed macos-latest job. This automatically also reruns the dependent tests-pass job, but not the sibling jobs that had been canceled. So although the macos-latest job passed when rerun, but the three canceled jobs did not rerun. tests-pass saw those jobs still had canceled status, and reported failure again.

I've rerun the workflow as a whole, and all jobs passed. From context, it looks like the failing tests might have been all that were still blocking this PR form being merged. However, since auto-merge wasn't enabled here and I haven't been following this PR closely, I have refrained from merging it, in case my understanding is not correct.

[Byron]

The original failure in the test-fast job on macos-latest was due to #1816.

Thanks for pointing this out! I remember now that the filesystem probe can have collisions across processes and produce incorrect values due to a race.

Thanks also for rerunning CI properly, I will keep that in mind.

[..] since auto-merge wasn't enabled here and I haven't been following this PR closely, I have refrained from merging it, in case my understanding is not correct.

Thank you, that's the right call. The plan here is that @cruessler will do the first review, and I take a look once he approves for merging.

[EliahKagan]

Thanks also for rerunning CI properly, I will keep that in mind.

The way I reran it was arguably overkill--when rerunning a whole workflow, one can select "Re-run failed jobs" instead of "Re-run all jobs", which in spite of its name, I think does also rerun canceled jobs from the same matrix as a failed job that were canceled because of it.

("Re-run all jobs" and "Re-run failed jobs" are available at the workflow level, and should not be confused with re-running a single job, which doesn't automatically re-run failed or canceled sibling jobs.)

Thank you, that's the right call.

Thanks--I wasn't sure if the requested review was covered by https://github.com/GitoxideLabs/gitoxide/pull/1976#issuecomment-2847048347 or not. I'm glad I held off from merging.

[Byron]

The way I reran it was arguably overkill--when rerunning a whole workflow, one can select "Re-run failed jobs" instead of "Re-run all jobs", which in spite of its name, I think does also rerun canceled jobs from the same matrix as a failed job that were canceled because of it.

That's a great pointer - I definitely thought "Re-run failed jobs" does the trick even for cancelled ones, which means I must have hit a rerun button on the level of the individual failed job.

[cruessler]

@holodorum Now that RustWeek is over, I’ll get to the review in the next couple of days!

[holodorum]

Thanks for the review @cruessler. I've implemented almost all your changes. I feel that the difference between one_based_inclusive_ranges and zero_based_exclusive leaves some room for error and confusion. What do you think about introducing a custom type OneBasedInclusiveRange and ZeroBasedExclusiveRange. We could keep the function names simple, and force the correct input.

[cruessler]

@holodorum I know it’s been some time (and it’s totally okay if you’ve moved on), but do you want to keep working on this PR? Otherwise I’m thinking of taking over and finishing myself. Let me know what your preferences are and if there’s anything I can do to help!

[holodorum]

@cruessler got a bit distracted indeed, my bad. Will update the PR with the changes this week. Main thing outstanding is not erroring, but turning WholeFile into a PartialFile as discussed in this comment right?

[cruessler]

@holodorum Sounds good! If you want, you can also have a look at https://github.com/GitoxideLabs/gitoxide/compare/main...cruessler:gitoxide:improve-blame-ranges for inspiration where I already incorporated most of my feedback into your changes. I also split the commits so that each commit only contains changes related to a single crate (something that gitoxide does). There’s a few more edge cases that BlameRanges should handle, some of which are already part of my branch. What’s missing is code for handling the following situation:

#[test]
fn to_zero_based_exclusive_ranges_doesnt_exceed_max_lines() {
    let mut ranges = BlameRanges::from_one_based_inclusive_range(1..=5).unwrap();
    ranges.add_one_based_inclusive_range(6..=10).unwrap();

    assert_eq!(ranges.to_zero_based_exclusive_ranges(7), vec![0..5, 6..7]);
}

[holodorum]

Ahh okay, @cruessler if you already did most of the work I'd think it makes most sense if you finish it.

[cruessler]

@holodorum This is the follow-up PR: https://github.com/GitoxideLabs/gitoxide/pull/2204.

[Byron]

Thanks everyone!

Closing this PR as it was superseded.