diff-hl-update-async can break magit-commit

[whhone]

Expected behavior: (COMMIT_EDITMSG buffer is killed after committing)

1. magit-commit uses with-editor-mode to edit the commit message (COMMIT_EDITMSG).
2. The user uses C-c C-c (with-editor-finish) to save the commit message and close the buffer COMMIT_EDITMSG.

Actual behavior: (COMMIT_EDITMSG buffer is not killed after committing)

1. When diff-hl-update-async is enabled and the buffer COMMIT_EDITMSG is unsaved,
2. The user uses C-c C-c to commit the message,
3. with-editor will save the buffer, causing diff-hl-update to run,
4. the diff-hl-update-async thread will prevent the buffer COMMIT_EDITMSG to be killed
5. The next magit-commit will try to reuse COMMIT_EDITMSG instead of creating a new buffer.

[dgutov]

Hi!

Would it work to just avoid using diff-hl in such buffers?

Like this:

diff --git a/diff-hl.el b/diff-hl.el
index 29dfe31..0d979ed 100644
--- a/diff-hl.el
+++ b/diff-hl.el
@@ -158,7 +158,7 @@ the end position as its only argument."
                  (const :tag "Narrow to the hunk"
                         diff-hl-revert-narrow-to-hunk)))

-(defcustom diff-hl-global-modes '(not image-mode)
+(defcustom diff-hl-global-modes '(not image-mode with-editor-mode)
   "Modes for which `diff-hl-mode' is automagically turned on.
 This affects the behavior of `global-diff-hl-mode'.
 If nil, no modes have `diff-hl-mode' automatically turned on.

[whhone]

with-editor-mode is a minor mode and diff-hl-global-modes assumes major-mode.

But I think this approach works :-)

[dgutov]

Okay. So it doesn't use any unique major modes, does it?

You could still add a function to find-file-hook, I guess, which would turn off diff-hl-mode in certain unsuitable buffers. Can they be determined by file name? We could add regexps to the diff-hl-global-modes syntax as well.

Finally, it might be worth looking into the reason for step 4. Are all buffers assigned to live background threads unkillable? Maybe there is a flag somewhere. There is one for processes: (process-query-on-exit-flag process).

[whhone]

Right, with-editor-mode is a minor mode.

---

Using find-file-hook to disable diff-hl-update-async can fix the issue.

(add-hook 'find-file-hook
          #'(lambda ()
              (when (bound-and-true-p with-editor-mode)
                (setq-local diff-hl-update-async nil))))

---

This issue cannot be determined by file name in general. But for the git commit case, it is ~/.git/COMMIT_EDITMSG.

[dgutov]

That looks good.

We could also extend the diff-hl-update-async variable so it could be set to a predicate function -- that function would be called every time an update is done, but for the above comparison that would be cheap enough.

An ideal solution would perhaps try to eliminate the incompatibility between async updates and w-e-m somehow, but it's not urgent, nor incompatible with the described above.

[tarsius]

Finally, it might be worth looking into the reason for step 4. Are all buffers assigned to live background threads unkillable?

kill-buffer calls thread_check_current_buffer directly (not via the hook) and if that returns true, then the buffer is not killed. It doesn't look like there is a way around that.

There also does not appear to be a way to get the buffer associated with a thread, so with-editor cannot check if there are any threads associated with the buffer in question (in which case it could wait or inform the user or something).

@dgutov, please make advices unnecessary, such as the one @hlissner added to doom.

[dgutov]

Hi Jonas, thanks for the bump.

Yeah, this is unfortunate - kill-buffer-query-functions are queried after that check, so there's no hook to use to try to detach the thread.

[dgutov]

Does this work?

diff --git a/diff-hl.el b/diff-hl.el
index 9ef52af..945ef4c 100644
--- a/diff-hl.el
+++ b/diff-hl.el
@@ -395,7 +395,8 @@ didn't work reliably in such during testing."
   "Updates the diff-hl overlay."
   (if (and diff-hl-update-async
            ;; Disable threading on the remote file as it is unreliable.
-           (not (file-remote-p default-directory)))
+           (not (file-remote-p default-directory))
+           (not (bound-and-true-p with-editor-mode)))
       ;; TODO: debounce if a thread is already running.
       (make-thread 'diff-hl--update-safe "diff-hl--update-safe")
     (diff-hl--update)))

[dgutov]

We could try a slightly more guessworky approach, though.

diff --git a/diff-hl.el b/diff-hl.el
index 9ef52af..04304c4 100644
--- a/diff-hl.el
+++ b/diff-hl.el
@@ -395,7 +395,8 @@ didn't work reliably in such during testing."
   "Updates the diff-hl overlay."
   (if (and diff-hl-update-async
            ;; Disable threading on the remote file as it is unreliable.
-           (not (file-remote-p default-directory)))
+           (not (file-remote-p default-directory))
+           (not (local-variable-p 'kill-buffer-query-functions)))
       ;; TODO: debounce if a thread is already running.
       (make-thread 'diff-hl--update-safe "diff-hl--update-safe")
     (diff-hl--update)))

To maybe detect most similar situations.

[hlissner]

Personally, I prefer the precision of the with-editor-mode check. Plus, with-editor seems unique enough in the Emacs space to target it directly like that, imo.

But if you must settle for (not (local-variable-p 'kill-buffer-query-functions)), could it be done in a separate predicate function? And that either diff-hl-update-async be changed to take a function, or introduce something like a diff-hl-inhibit-async-functions variable (with said predicate function in it by default)? That way, users/packages can handle the edge cases, if they crop up.

Otherwise, their only option is advice, and it's easier to let-bind with-editor-mode in advice than it is to temporarily un-localize a variable in a way that'll fool local-variable-p.

[dgutov]

@hlissner Okay, I've pushed that solution, though with slightly different variable name.

Let me know if something's not working well.

[tarsius]

Thanks a lot!

[hlissner]

It's working great! Thanks for the quick turnaround (and for diff-hl itself)!

[dgutov]

Welcome!