When diff-hl-mode is enabled, revert-buffer issues 3 calls to vc-diff

[tomfitzhenry]

Steps to reproduce

1. Modify a file tracked by vc, and save that file.
2. revert-buffer

Expected

revert-buffer triggers 1 call to vc-git-diff.

Actual

revert-buffer triggers 3 calls to vc-git-diff (observed via debug-on-entry)

This is a problem if vc-git-diff is slow (vc-diff can take ~200ms for me, which I know is atypical). It's especially a problem if many files are reverted (e.g. in a vc-retrieve-tag). I can find a vc-oriented

Stack traces

* vc-git-diff(("/home/tom/git-repo/foo") nil nil " *diff-hl* ")
  apply(vc-git-diff (("/home/tom/git-repo/foo") nil nil " *diff-hl* "))
  vc-call-backend(Git diff ("/home/tom/git-repo/foo") nil nil " *diff-hl* ")
  diff-hl-changes-buffer("/home/tom/git-repo/foo" Git)
  diff-hl-changes()
  diff-hl-update()
  run-hooks(diff-hl-mode-hook diff-hl-mode-on-hook)
  diff-hl-mode(1)
  turn-on-diff-hl-mode()
  diff-hl--global-turn-on()
  global-diff-hl-mode-enable-in-buffers()
  run-hooks(change-major-mode-after-body-hook after-change-major-mode-hook)
  normal-mode(t)
  after-find-file(nil nil t nil nil)
  revert-buffer--default(t nil)
  revert-buffer(t)
  funcall-interactively(revert-buffer t)
  call-interactively(revert-buffer record nil)
  command-execute(revert-buffer record)

  apply(vc-git-diff (("/home/tom/git-repo/foo") nil nil " *diff-hl* "))
  vc-call-backend(Git diff ("/home/tom/git-repo/foo") nil nil " *diff-hl* ")
  diff-hl-changes-buffer("/home/tom/git-repo/foo" Git)
  diff-hl-changes()
  diff-hl-update()
  run-hooks(diff-hl-mode-hook diff-hl-mode-on-hook)
  diff-hl-mode(1)
  turn-on-diff-hl-mode()
  diff-hl--global-turn-on()
  global-diff-hl-mode-enable-in-buffers()
  run-hooks(after-change-major-mode-hook)
  run-mode-hooks()
  fundamental-mode()
  set-buffer-major-mode(#<buffer foo>)
  set-auto-mode()
  normal-mode(t)
  after-find-file(nil nil t nil nil)
  revert-buffer--default(t nil)
  revert-buffer(t)
  funcall-interactively(revert-buffer t)
  call-interactively(revert-buffer record nil)
  command-execute(revert-buffer record)

Debugger entered--entering a function:
* vc-git-diff(("/home/tom/git-repo/foo") nil nil " *diff-hl* ")
  apply(vc-git-diff (("/home/tom/git-repo/foo") nil nil " *diff-hl* "))
  vc-call-backend(Git diff ("/home/tom/git-repo/foo") nil nil " *diff-hl* ")
  diff-hl-changes-buffer("/home/tom/git-repo/foo" Git)
  diff-hl-changes()
  diff-hl-update()
  run-hooks(revert-buffer-internal-hook)
  revert-buffer--default(t nil)
  revert-buffer(t)
  funcall-interactively(revert-buffer t)
  call-interactively(revert-buffer record nil)
  command-execute(revert-buffer record)

[dgutov]

Thanks for the report. I just pushed a change that should eliminate the last one.

Another extra call it a bit trickier because the minor mode does get enabled twice (by normal-mode), so a fix will need some kind of debounce.

[dgutov]

Unless we come up with something more elegant, here's a patch to try:

diff --git a/diff-hl.el b/diff-hl.el
index 0f59263..a28d463 100644
--- a/diff-hl.el
+++ b/diff-hl.el
@@ -358,6 +358,15 @@ performance when viewing such files in certain conditions."
                 (overlay-put h 'insert-in-front-hooks hook)
                 (overlay-put h 'insert-behind-hooks hook)))))))))
 
+(defvar-local diff-hl--modified-tick nil)
+
+(put 'diff-hl--modified-tick 'permanent-local t)
+
+(defun diff-hl-update-once ()
+  (unless (equal diff-hl--modified-tick (buffer-modified-tick))
+    (diff-hl-update)
+    (setq diff-hl--modified-tick (buffer-modified-tick))))
+
 (defun diff-hl-add-highlighting (type shape)
   (let ((o (make-overlay (point) (point))))
     (overlay-put o 'diff-hl t)
@@ -571,7 +580,7 @@ The value of this variable is a mode line template as in
                     ;; let's wait until the state information is
                     ;; saved, in order not to fetch it twice.
                     'find-file-hook)
-                  'diff-hl-update t t)
+                  'diff-hl-update-once t t)
         (add-hook 'vc-checkin-hook 'diff-hl-update nil t)
         ;; https://github.com/magit/magit/issues/603
         (add-hook 'magit-revert-buffer-hook 'diff-hl-update nil t)

[tomfitzhenry]

This works great, thanks!

[dgutov]

OK, regarding the previous fix, note to self: after-revert-hook is still needed for auto-revert-mode, or other modes that call revert-buffer with non-nil preserve-modes.

diff-hl should look up whether revert-buffer-preserve-modes is non-nil in its hook function.

[dgutov]

This should do it.

Hopefully it won't slow things down too much for you now that auto-revert-mode reverts trigger diff-hl-update again.