Skip to content

Support for multiple blame ranges #1973

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Apr 27, 2025

Conversation

holodorum
Copy link
Contributor

This PR adds support for multiple ranges to blame instead of a single-range. This is compatible with git's behavior. Using git it's possible to blame multiple ranges as follows:

git blame example.rs -L 1,2 -L 2,3 

This results in a blame of lines 1-3 in example.rs. After this update we are able to run the same command using gix!

Initially I tried implementing this without the refactoring to a BlameRanges type. However, it became messy quickly because the ranges had the type Option<Vec<std::ops::Range<u32>>> type. With BlameRanges the complexity is quite comprehensible I feel.

A possible improvement could be to add an enum that signals whether the ranges are zero_based or one_based.

@holodorum holodorum force-pushed the feature/blame-range-support branch from 93bb148 to ead0dca Compare April 27, 2025 10:19
holodorum and others added 4 commits April 27, 2025 22:38
This update replaces single-range handling with the `BlameRanges` type, allowing multiple 1-based inclusive line ranges to be specified for blame operations.

It hides some of the implementation details of the range logic, prepares for compatibility with `git` behavior, and adds tests to validate multi-range scenarios.
…> -L <other-range> ...`

Update the blame subcommand to handle multiple line ranges. This allows specifying multiple `-L` options similar to the usage of git.
- cargo fmt
- apply some IDE suggestions
- partition tests more
- run doc-tests
- use RangeInclusive for a less confusing representation
@Byron Byron force-pushed the feature/blame-range-support branch from ead0dca to d4461e7 Compare April 27, 2025 20:58
Copy link
Member

@Byron Byron left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much, this looks great!

Please note that I did refactor the commits to properly indicate breakage with conventional commits, and there are some other refactors too which you might find interesting.

Also CC @cruessler just in case he'd like to see other changes in a follow-up, the reason I merge it already is that it's implemented very non-invasively without basically no changes to the algorithm itself.

@Byron Byron enabled auto-merge April 27, 2025 21:04
@Byron Byron merged commit de13b16 into GitoxideLabs:main Apr 27, 2025
22 checks passed
@holodorum
Copy link
Contributor Author

Wow that was quick! And thank you, there are some insightful refactors and updates :)

@cruessler
Copy link
Contributor

cruessler commented Apr 28, 2025

@holodorum @Byron Just a couple of minor observations, some possibly nitpicky, so feel free to ignore. :-)

  • It might have made sense to also rename options.range to options.ranges as now there is a slight inconsistency, in particular between options.range and BlameRanges.
  • Is there a reason the initial construction of ranges is done via Vec::with_capacity and a for loop instead of via collect?
  • Since BlameRanges is now its own type (which I really like), I think we could still write a couple of unit tests in addition to the unit tests added for gix_blame::file.
  • Should the AsRange parser be renamed to AsInclusiveRange for consistency?

Apart from that, I think this is already a great addition!

@cruessler
Copy link
Contributor

Also, I feel there’s a certain contradiction in calling the method is_empty while returning a non-empty Vec in to_zero_based_exclusive. Can we find a name for is_empty that communicates its intent in a different way?

@holodorum
Copy link
Contributor Author

That are some good suggestions, thanks @cruessler.
I feel the same about that is_empty check. What about an enum along these lines?

enum BlameRangesState {
    Empty,
    OneInclusive(BlameRanges),
    ZeroInclusive(BlameRanges),
}

@cruessler
Copy link
Contributor

is_empty doesn’t seem to be used anymore, so it could also be removed at this point. 😄

@Byron
Copy link
Member

Byron commented Apr 28, 2025

Apologies for being so merge-happy 😅, next time I will wait for your review.
Maybe there could be a follow-up or a refactor-commit with the next contribution of @holodorum .

@cruessler
Copy link
Contributor

@Byron No worries! I think we can address all comments in a follow-up PR as you suggest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants