Reverting fails when too close to another hunk about diff-hl

[dgutov]

Hi!

I haven't been able to reproduce this exactly yet.

Does the committed state of the file contain the trailing newline?

When yes, the scenario works without issue.

When no (without trailing newline), I see the error right away: (Can’t find the beginning of the hunk), before the diff buffer is even shown.

Generally the absence of trailing newline is not a great idea for coding files, but we can support it here. I've pushed a patch that makes it work to master just now. Does it help in your scenario too?

[yuhan0]

Thanks for the quick response! I've pulled the latest changes but unfortunately they don't solve my issue.

Yes, both the commited and unstaged versions of the files have a trailing newline. I have require-final-newline set to t which enforces this on save, and this issue would occur in the middle of source files with 100s of LOC with the hunks nowhere close to the end of the file.

After some more digging I found that the error is caused by the default diff-hl-highlight-revert-hunk-function narrowing the buffer and cutting off the hunk header. As a workaround, setting the variable to ignore or manually performing M-x widen before answering the prompt solves the issue.

Maybe the bug is an off-by-one error in diff-hl-split-away-changes placing the point on the wrong line?

Another minimal repro with more context lines around the changes:

a
b
c
d
foo
bar
baz
w
x
y
z

After changing the foo and baz lines, I get the same error when attempting to revert the lower hunk, and see the same lack of header in the popup buffer.

Contents of the full widened *diff-hl* buffer:

diff --git a/src/repro.txt b/src/repro.txt
index 7121797..7d1d16b 100644
--- a/src/repro.txt
+++ b/src/repro.txt
@@ -2,4 +2,4 @@ a
 b
 c
 d
-foo
+foo*
@@ -6,5 +6,5 @@
 bar
-baz
+baz*
 w
 x
 y

I find it strange that we are parsing the diff output and manually splitting hunks, instead of calling diff with the appropriate switches in the first place, or using a single source of truth for both the fringe highlights and applying changes. Running git diff -U0 repro.txt in my terminal gives the expected output with 2 separate hunks.

[dgutov]

After some more digging I found that the error is caused by the default diff-hl-highlight-revert-hunk-function narrowing the buffer and cutting off the hunk header. As a workaround, setting the variable to ignore or manually performing M-x widen before answering the prompt solves the issue.

Thanks! I reproduced it now. It was my fault: I have the variable customized to the other available value (diff-hl-revert-highlight-first-column), which was the default before 2021.

Using widen is not a workaround, it's the proper fix (another option would be to use save-restriction), try this:

diff --git a/diff-hl.el b/diff-hl.el
index ce1f1ca..9960a51 100644
--- a/diff-hl.el
+++ b/diff-hl.el
@@ -609,6 +609,7 @@ in the source file, or the last line of the hunk above it."
                     (unless (yes-or-no-p (format "Revert current hunk in %s? "
                                                  file))
                       (user-error "Revert canceled")))
+                (widen)
                 (let ((diff-advance-after-apply-hunk nil))
                   (save-window-excursion
                     (diff-apply-hunk t)))

I find it strange that we are parsing the diff output and manually splitting hunks, instead of calling diff with the appropriate switches in the first place,
or using a single source of truth for both the fringe highlights and applying changes.

The splitting routine is fine (at least since the last commit), and we're using it to indeed have the fringe highlights and reverting changes use the same source of info. Calling the diff program one more time could have its own difficulties too, and note that it won't save us from having to find the bounds of the current hunk, to narrow to it, or to apply just that hunk.

There is another difference: using diff -U3 would coagulate hunks, but we do want to show some context around the minimal hunk when possible (when the other hunks are far away enough). There is no option for diff that would do that.

[dgutov]

I've pushed the change now. Let me know if something is still amiss.

[yuhan0]

Thanks! I can confirm the fix works, although there's still a minor inconsistency that the narrowed buffer does not show the hunk header, due to the off-by-one in the split-away-changes function.

The splitting routine is fine (at least since the last commit), and we're using it to indeed have the fringe highlights and reverting changes use the same source of info. Calling the diff program one more time could have its own difficulties too, and note that it won't save us from having to find the bounds of the current hunk, to narrow to it, or to apply just that hunk.

Unless I'm misunderstanding somethng, I stepped through the codebase and it looks like the fringes are updated using (diff-hl-changes), which calls either

• diff-hl-changes-buffer->diff-hl-diff-against-reference on *diff-hl* buffer, wrapped with diff-hl-with-diff-switches
• or the overriden diff-hl-flydiff-changes-buffer -> diff-hl-diff-buffer-with-reference on *diff-hl-diff* buffer

Whereas diff-hl-revert-hunk-1 generates a new temporary buffer and calls diff-hl-diff-against-reference on it (without the -diff-switches macro) and displays it as a popup, then using it to actually apply the changes.

I don't completely understand the distinction between against-reference and with-reference, but clearly there's two different calls to the diff program happening.

If my cursor is on a line with a fringe highlight derived from the *diff-hl* buffer, it shouldn't have to 're-derive' the current hunk, based on another call to diff with additional context lines and using parsing logic to 'split away' the hunk. That seems very unintuitive, even if the logic is sound 100% of the time. I shouldn't have to second-guess and check the popup buffer to verify if the hunk being reverted is indeed the one indicated by the fringe highlight. Hope that makes sense!

[yuhan0]

More generally, I don't know if displaying of context lines in the popup buffer is worth all this additional complexity? After all the user already has the full context in the original buffer that they are working from. By definition diff-hl-revert-hunk is being called with the cursor in the midst of this context.

If I care about the wider context relative to other hunks in the buffer, I would use something like magit-status-here which affords a much better UI to view and apply complex changes - I'm sure vc-diff has something similar.

[dgutov]

You are right, the naming is a bit confusing here. diff-hl-diff-against-reference and diff-hl-diff-buffer-with-reference do almost the same thing, except one of them can do it a little faster because it doesn't need to deal with buffer contents not synchronized to disk (which is the flydiff situation).

diff-hl-revert-hunk-1 mostly does things its own way to copy the UI of vc-revert, with its own alteration. It still calls diff-hl-diff-against-reference, though.

You are right that we could possibly store the hunks inside the overlays to avoid re-retching them more often. That wouldn't avoid having the two aforementioned functions, though (unless we just decide to use the slower-but-more-flexible one).

Regarding additional context, I do think it both looks nicer and a little more useful. But also keep in mind that diff-hl-highlight-revert-hunk-function is customizable and the other setting shows the full diff (with particular section highlighted in the first column). So we need a way to restore the full diff buffer anyway.

[dgutov]

although there's still a minor inconsistency that the narrowed buffer does not show the hunk header, due to the off-by-one in the split-away-changes function

This is hopefully fixed now.