Skip to content

Conversation

mgsloan
Copy link
Contributor

@mgsloan mgsloan commented Sep 28, 2025

The bug is that when the first hunk start line is > context_lines and <= 2 * context_lines, all the lines before the hunk are included in the context.

This fixes the behavior to be the same as diff -u:

printf "a\nb\nc\nd\ne\nf\ng\nh\ni" > input1
printf "a\nb\nc\nd\nedit\nf\ng\nh\ni" > input2
diff -u input1 input2
@@ -2,7 +2,7 @@
 b
 c
 d
-e
+edit
 f
 g
 h

This came up in Zed when optimizing diffing in a case where the edited lines are known. These edited regions are then provided to imara-diff. Often this works perfectly, but when the first diff is not immediately after the context lines this bug occurs and causes extra prefix context lines to appear.

I realize that only diffing these edited regions will not always match a full input diff, but for this particular use-case it is preferable :)

Copy link
Collaborator

@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 a lot!

I could validate that the first commit fails with:

error: expect test failed
   --> src/tests.rs:235:9

You can update all `expect!` tests by running:

    env UPDATE_EXPECT=1 cargo test

To update a single test, place the cursor on `expect` token and use `run` feature of rust-analyzer.

Expect:
----
@@ -2,7 +2,7 @@
 b
 c
 d
-e
+edit
 f
 g
 h

----

Actual:
----
@@ -2,8 +2,8 @@
 a
 b
 c
 d
-e
+edit
 f
 g
 h

----

Diff:
----
@@ -2,78 +2,78 @@
 a
 b
 c
 d
-e
+edit
 f
 g
 h

----

and that it works with the second commit applied.

However, I will leave it to @pascalkuthe to fix this. The version of this in gitoxide was already fixed a while ago I believe.
The problem here remains: the whole library is quite under-tested.

mgsloan added a commit to zed-industries/imara-diff that referenced this pull request Sep 29, 2025
mgsloan added a commit to zed-industries/imara-diff that referenced this pull request Sep 29, 2025
@gcanat
Copy link

gcanat commented Sep 30, 2025

That's what I was trying to explain here #14 (comment) indeed.
👍

@pascalkuthe pascalkuthe merged commit 8de65af into pascalkuthe:master Oct 1, 2025
5 checks passed
@pascalkuthe
Copy link
Owner

thanks!

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.

4 participants