-
-
Notifications
You must be signed in to change notification settings - Fork 423
Switch to imara-diff 0.2 in gix-blame
#2420
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
Switch to imara-diff 0.2 in gix-blame
#2420
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR completes the transition of gix-blame to always use the imara-diff v0.2-backed implementation (via gix-diff’s experimental blob diff feature), and removes the now-obsolete feature flags that previously selected between v0.1 and v0.2.
Changes:
- Remove
blame-experimentalfeature wiring from the workspace (Cargo.toml,gitoxide-core,gix) and stop runninggix-blametests with the oldblob-experimentalflag. - Make
gix-blamealways enablegix-diff’sblob-experimentalfeature and remove thegix-blame-levelblob-experimentalfeature. - Simplify/adjust the blame disparity test to run both Myers and Histogram under the unified setup.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
justfile |
Stops running gix-blame unit tests with the removed blob-experimental feature. |
gix/Cargo.toml |
Removes the blame-experimental feature flag from gix. |
gix-blame/tests/blame.rs |
Updates the disparity regression test to run both algorithms under the always-v0.2 setup. |
gix-blame/src/file/function.rs |
Removes the non-experimental blob diff code path, leaving the v0.2-based implementation. |
gix-blame/Cargo.toml |
Removes the crate feature and enables gix-diff’s blob-experimental unconditionally. |
gitoxide-core/Cargo.toml |
Removes the blame-experimental feature passthrough. |
Cargo.toml |
Removes the workspace-level blame-experimental feature passthrough. |
Comments suppressed due to low confidence (2)
gix-blame/tests/blame.rs:325
- This test uses multiple
.unwrap()calls (e.g.,Fixture::new().unwrap()), which makes failures harder to diagnose and goes against the project’s general “no unwrap” practice. Consider returninggix_testtools::Resultfrom the test and using?, or use.expect("…")with context.
let Fixture {
odb,
mut resource_cache,
suspect,
} = Fixture::new().unwrap();
gix-blame/tests/blame.rs:349
- More
.unwrap()s here (gix_blame::file(...).unwrap(),fixture_path().unwrap(),Baseline::collect(...).unwrap()). Prefer propagating errors via?(by making the test returngix_testtools::Result) or at least adding contextual.expect()messages so failures are actionable.
let lines_blamed = gix_blame::file(
&odb,
suspect,
None,
&mut resource_cache,
source_file_name.as_ref(),
gix_blame::Options {
diff_algorithm,
ranges: BlameRanges::default(),
since: None,
rewrites: Some(gix_diff::Rewrites::default()),
debug_track_path: false,
},
)
.unwrap()
.entries;
assert_eq!(lines_blamed.len(), 5);
let git_dir = fixture_path().unwrap().join(".git");
let baseline = Baseline::collect(git_dir.join(format!("{case}.baseline")), source_file_name).unwrap();
Byron
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fantastic, thanks!
Now we have experimental support by default for everyone who uses default gix features, but that's alright.
This PR changes
gix-blameto always useimara-diff0.2 throughgix-diff/blame-experimental. It removes theblob-experimentalfeature flag ingix-blameas well as theblame-experimentalfeature flag ingix.This PR does not yet change
gix-diffitself yet. I’m in the process of preparing a separate PR for that switch as well, but I thought the transition was easier to do and review in smaller PRs. If you think this should be done in a single PR, though, I’ll be happy to incorporate the other changes into this PR as well. Let me know what you think!