Skip to content

Conversation

@cruessler
Copy link
Contributor

This is a quick conversion of gix-blame to use imara-diff 0.2 through gix-diff’s new blob-experimental feature. I haven’t run any benchmarks yet, just wanted to open the PR, so you can have a look yourself! The update made one test that previously was marked should_panic pass, so that’s already promising. :-)

@Byron
Copy link
Member

Byron commented Dec 7, 2025

This looks very promising indeed! Some complexity now moved to imara-diff.

Let's see those performance comparisons as well :).
Until we are more sure how this works, you could put this change behind a feature flag as well, supporting both implementations side-by-side. Alternatively, this can stay open until we know what to do with V2. And… once it's clear that it is not slower, I guess it can just be adopted, provided all existing unified diff tests can be ported which will be quite an undertaking I suppose.

@cruessler
Copy link
Contributor Author

I added a feature flag, blob-experimental, to gix-blame. This should hopefully also make the full diff easier to read as it now mostly contains additions and very few changes.

I decided to re-use the comment to blob-experimental that I copied from gix-diff. Feel free to re-phrase, though.

I’ll mark this PR as ready even though CI hasn’t completed yet (so I can turn off my computer :-)). If anything comes up, I’ll address it, but apart from that, I hope it is finished.

The only thing that’s missing is proably a line such as the following one in justfile:

cargo nextest run -p gix-blame --features blob-experimental --no-fail-fast

@cruessler cruessler marked this pull request as ready for review December 13, 2025 14:02
Copy link

Copilot AI left a 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 updates gix-blame to use imara-diff 0.2 through the new blob-experimental feature in gix-diff. The update is feature-gated to allow testing both versions, and notably, a test that previously required should_panic for the Myers algorithm now passes with the new version.

  • Feature-gated implementation allows testing both imara-diff 0.1 and 0.2
  • New blob_changes function implementation for v2 API with simplified hunk processing
  • Test organization updated to separate v1 and v2 test cases with appropriate feature gates

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File Description
gix-blame/Cargo.toml Adds blob-experimental feature flag to enable imara-diff 0.2 via gix-diff
justfile Adds test command for gix-blame with the new blob-experimental feature
gix-blame/tests/blame.rs Splits diff_disparity test into v1 and v2 variants, updates comments to reflect that v2 tests now pass
gix-blame/src/file/function.rs Implements feature-gated blob_changes function for imara-diff 0.2 API with updated hunk processing logic

Comment on lines 889 to 918
let changes = diff
.hunks()
.fold((Vec::new(), 0), |(mut hunks, mut last_seen_after_end), hunk| {
let Hunk { before, after } = hunk;

// This checks for unchanged hunks.
if after.start > last_seen_after_end {
hunks.push(Change::Unchanged(last_seen_after_end..after.start));
}

match (!before.is_empty(), !after.is_empty()) {
(_, true) => {
hunks.push(Change::AddedOrReplaced(
after.start..after.end,
before.end - before.start,
));
}
(true, false) => {
hunks.push(Change::Deleted(after.start, before.end - before.start));
}
(false, false) => unreachable!("BUG: imara-diff provided a non-change"),
}

last_seen_after_end = after.end;

(hunks, last_seen_after_end)
});

stats.blobs_diffed += 1;
Ok(changes.0)
Copy link

Copilot AI Dec 17, 2025

Choose a reason for hiding this comment

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

The new implementation is missing logic to handle the final unchanged hunk at the end of the file. In the original implementation (lines 812-815), there's a check in the finish method that adds a final Change::Unchanged if there are lines remaining after the last change. This ensures the entire file is represented by Change entries.

The v2 implementation should add similar logic after the fold operation to check if last_seen_after_end is less than the total number of lines in the destination file, and if so, push a final Change::Unchanged hunk.

Copilot uses AI. Check for mistakes.
Copy link
Member

Choose a reason for hiding this comment

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

Great catch!
We also don't have that could catch this.

- fix probably critical, and untested error related to the last hunk
@Byron Byron force-pushed the update-to-imara-diff-0-2-in-gix-blame branch from a0fee1a to 0b7c1dd Compare December 17, 2025 06:02
@Byron
Copy link
Member

Byron commented Dec 17, 2025

Thanks a lot, this looks very promising already!
Now, technically, gix-blame is basically as good as the Git one in terms of correctness, and probably comes quite close in terms of performance. This makes me curious for performance comparisons as well, but my guess is that imara v1 and v2 have the same performance in practice.

Lastly, let me point out a critical issue that Copilot found which I 'fixed', but also noticed that there is no test that captures this particular handling. Now I wonder if Blame doesn't care due to its nature, so the special case could be dropped, or if we are really missing that one test that runs into this.

@Byron Byron enabled auto-merge December 17, 2025 06:05
@Byron Byron merged commit 691a205 into GitoxideLabs:main Dec 17, 2025
28 checks passed
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.

2 participants