-
-
Notifications
You must be signed in to change notification settings - Fork 43
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
diff-hl-update-async can break magit-commit #213
Comments
Hi! Would it work to just avoid using 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.
|
But I think this approach works :-) |
Okay. So it doesn't use any unique major modes, does it? You could still add a function to 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: |
Right, Using
This issue cannot be determined by file name in general. But for the git commit case, it is |
That looks good. We could also extend the 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. |
`diff-hl-update-async` was enabled in f2696d7, causing a regression where with-editor buffers wouldn't get cleaned up properly. This would particularly affect Magit's COMMIT_EDITMSG buffers. To quote jds on Discord: It seems like after committing (with cc in the magit buffer), it leaves COMMIT_EDITMSG around. The next time I try & commit, a single c keypress immediately jumps to the old COMMIT_EDITMSG buffer, but it's inactive - C-c C-c closes that buffer but makes no change to git. I opt for advice instead of find-file or with-editor-mode hooks to restore normal behavior should with-editor-mode be later disabled without the death of its buffer (not relevant to magit, specifically, but it might be to other consumers of with-editor, now or in the future). Ref: dgutov/diff-hl#213 Amend: f2696d7
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. |
Hi Jonas, thanks for the bump. Yeah, this is unfortunate - |
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))) |
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. |
Personally, I prefer the precision of the But if you must settle for Otherwise, their only option is advice, and it's easier to let-bind |
`diff-hl-update-async` was enabled in f2696d7, causing a regression where with-editor buffers wouldn't get cleaned up properly. This would particularly affect Magit's COMMIT_EDITMSG buffers. To quote jds on Discord: It seems like after committing (with cc in the magit buffer), it leaves COMMIT_EDITMSG around. The next time I try & commit, a single c keypress immediately jumps to the old COMMIT_EDITMSG buffer, but it's inactive - C-c C-c closes that buffer but makes no change to git. I opt for advice instead of find-file or with-editor-mode hooks to restore normal behavior should with-editor-mode be later disabled without the death of its buffer (not relevant to magit, specifically, but it might be to other consumers of with-editor, now or in the future). Ref: dgutov/diff-hl#213 Amend: f2696d7
@hlissner Okay, I've pushed that solution, though with slightly different variable name. Let me know if something's not working well. |
Thanks a lot! |
It's working great! Thanks for the quick turnaround (and for |
Welcome! |
dgutov/diff-hl@11f3113e7905 -> dgutov/diff-hl@f66345ed1f17 - Our diff-hl-update-async hack is no longer needed. The issue was addressed upstream. Amend: 778fc9a Ref: dgutov/diff-hl#213 Ref: dgutov/diff-hl@e264d13a3d55
Expected behavior: (
COMMIT_EDITMSG
buffer is killed after committing)magit-commit
useswith-editor-mode
to edit the commit message (COMMIT_EDITMSG
).C-c C-c
(with-editor-finish
) to save the commit message and close the bufferCOMMIT_EDITMSG
.Actual behavior: (
COMMIT_EDITMSG
buffer is not killed after committing)diff-hl-update-async
is enabled and the bufferCOMMIT_EDITMSG
is unsaved,C-c C-c
to commit the message,diff-hl-update
to run,diff-hl-update-async
thread will prevent the bufferCOMMIT_EDITMSG
to be killedmagit-commit
will try to reuseCOMMIT_EDITMSG
instead of creating a new buffer.The text was updated successfully, but these errors were encountered: