diff-hl-dired: Handle nested file paths

[rynffoll]

I've made that changes to integrate diff-hl-dired with dired-subtree

Some details how it works before and how it'll work after

context:

We have some changes by paths:

go.mod
go.sum
cmd/service/main.go

before: highlight only first level

after: highlight first and nested paths

Notes

For better integration with dired-subree we can add this hook:

(add-hook 'dired-subtree-after-insert-hook #'diff-hl-dired-update)

[rynffoll]

I've tried to add hook:

(add-hook 'dired-subtree-after-insert-hook #'diff-hl-dired-update)

But I got an error:

error in process sentinel: vc-exec-after: Unexpected process state

When dired-subtree expands a subdirectory, diff-hl-dired-update is called while a previous VC process is still running. The existing code calls kill-process, which triggers the VC sentinel (vc-exec-after) with an unexpected process state.

I'm not an elisp expert, but replacing kill-process with delete-process seems to fix this - would that be the correct approach here?

[rynffoll]

I've tried to add hook:

(add-hook 'dired-subtree-after-insert-hook #'diff-hl-dired-update)

But I got an error:

error in process sentinel: vc-exec-after: Unexpected process state

When dired-subtree expands a subdirectory, diff-hl-dired-update is called while a previous VC process is still running. The existing code calls kill-process, which triggers the VC sentinel (vc-exec-after) with an unexpected process state.

I'm not an elisp expert, but replacing kill-process with delete-process seems to fix this - would that be the correct approach here?

Resolved by introducing a diff-hl-dired--status state machine (nil -> running -> pending) instead of killing the process.
When diff-hl-dired-update is called while a VC process is already running, it transitions to pending and returns early. When the running process completes, it checks the state and schedules a retry via run-at-time. This way N concurrent calls collapse into a single retry after the current process finishes.

Also moved diff-hl-dired-clear from diff-hl-dired-update to diff-hl-dired-highlight-items - clearing overlays right before redrawing avoids the flicker window between clear and async callback.

[dgutov]

Hi! Thank you for the submission.

Regarding your initial solution of using delete-process - did it fail to work in some cases? There is some protection against processes dying unexpectedly in VC's helpers, but at least since Emacs 31 vc-git-dir-status-goto-stage uses the newly added vc-run-delayed-success - which should do nothing after the process is deleted

Economically, deleting the process right away to launch it with correct parameters should finish sooner. But perhaps you were still seeing errors. Try it with Emacs 31 if you can.

[rynffoll]

Hi! Thank you for the submission.

Regarding your initial solution of using delete-process - did it fail to work in some cases? There is some protection against processes dying unexpectedly in VC's helpers, but at least since Emacs 31 vc-git-dir-status-goto-stage uses the newly added vc-run-delayed-success - which should do nothing after the process is deleted

Economically, deleting the process right away to launch it with correct parameters should finish sooner. But perhaps you were still seeing errors. Try it with Emacs 31 if you can.

Last week I've tried Emacs 31 and delete-process - it works a bit better than the previous one, I didn't get a question about killing the previous process.
But I got errors like .git/index.lock': File exists., the same errors I had when I tried replacing delete-process with kill-process.

[dgutov]

I see. Does adding GIT_OPTIONAL_LOCKS=0 to process-environment make any difference?

[rynffoll]

I see. Does adding GIT_OPTIONAL_LOCKS=0 to process-environment make any difference?

Mb I can do something wrong, but I cannot see any difference, I've tried to set GIT_OPTIONAL_LOCKS=0 globally and also for process buffer only.

[rynffoll]

I see. Does adding GIT_OPTIONAL_LOCKS=0 to process-environment make any difference?

Mb I can do something wrong, but I cannot see any difference, I've tried to set GIT_OPTIONAL_LOCKS=0 globally and also for process buffer only.

I've made some investigations around vc-git package.
We have a call chain like this:
diff-hl-dired-update -> vc-call-backend 'dir-status-files -> vc-git-dir-status-goto-stage which runs a chain of async git processes: update-index (GIT_OPTIONAL_LOCKS=0 applies only for read operations) -> diff-index -> ls-files-*

[dgutov]

We have a call chain like this:
diff-hl-dired-update -> vc-call-backend 'dir-status-files -> vc-git-dir-status-goto-stage which runs a chain of async git processes: update-index (GIT_OPTIONAL_LOCKS=0 applies only for read operations) -> diff-index -> ls-files-*

I see. And maybe the first of the commands (which is update-index) is usually hit in your case because the code runs in a hook very closely after.

Hmm, but looking at where the hook are called, maybe there is a simpler way.

IIUC the conflict comes from diff-hl-dired-update being called twice: once from dired-after-readin-hook directly, and once from dired-subtree-after-insert-hook, which happens to be called from dired-subtree--after-readin, which is on dired-after-readin-hook as well - which is set up for the case when the whole Dired buffer is refreshed. I don't think we need for both invocations to be made, although possibly to ensure that the first one is done after all the output is read. Which might amount to

diff --git a/diff-hl-dired.el b/diff-hl-dired.el
index 4a643af..aa566c7 100644
--- a/diff-hl-dired.el
+++ b/diff-hl-dired.el
@@ -86,7 +98,7 @@ status indicators."
       (progn
         (diff-hl-maybe-define-bitmaps)
         (set (make-local-variable 'diff-hl-dired-process-buffer) nil)
-        (add-hook 'dired-after-readin-hook 'diff-hl-dired-update nil t))
+        (add-hook 'dired-after-readin-hook 'diff-hl-dired-update 10 t))
     (remove-hook 'dired-after-readin-hook 'diff-hl-dired-update t)
     (diff-hl-dired-clear)))

Although it doesn't seem necessary in my testing, but probably because of the way the hooks got settled because of the loading order.

But you'll want the update happen when dired-subtree-insert is called directly. So I can suggest checking this-command:

(defun diff-hl-dired-update-if-subtree-insert ()
  (when (eq this-command 'dired-subtree-insert)
    (diff-hl-dired-update)))

(add-hook 'dired-subtree-after-insert-hook #'diff-hl-dired-update-if-subtree-insert)

This config seems to work in my testing, without tracking the state of the update, or re-launching it. That should help simplify the patch.

[dgutov]

That said, and speaking of the contribution as a whole, for accumulated patches of roughly >15 lines the FSF has a process to collect copyright assignments. And diff-hl is a part of Emacs in this regard.

So please LMK if you will be willing to sign such a paper (it's been done by a lot of people, and for citizens of some countries such as US and Germany, the process can be done electronically and quickly). If no - we'll need to reduce the size of the change to under 15 lines, preferably under 10 (just additions, not deletions). But with my suggestion above it might still be doable.

[rynffoll]

We have a call chain like this:
diff-hl-dired-update -> vc-call-backend 'dir-status-files -> vc-git-dir-status-goto-stage which runs a chain of async git processes: update-index (GIT_OPTIONAL_LOCKS=0 applies only for read operations) -> diff-index -> ls-files-*

I see. And maybe the first of the commands (which is update-index) is usually hit in your case because the code runs in a hook very closely after.

Hmm, but looking at where the hook are called, maybe there is a simpler way.

IIUC the conflict comes from diff-hl-dired-update being called twice: once from dired-after-readin-hook directly, and once from dired-subtree-after-insert-hook, which happens to be called from dired-subtree--after-readin, which is on dired-after-readin-hook as well - which is set up for the case when the whole Dired buffer is refreshed. I don't think we need for both invocations to be made, although possibly to ensure that the first one is done after all the output is read. Which might amount to

diff --git a/diff-hl-dired.el b/diff-hl-dired.el
index 4a643af..aa566c7 100644
--- a/diff-hl-dired.el
+++ b/diff-hl-dired.el
@@ -86,7 +98,7 @@ status indicators."
       (progn
         (diff-hl-maybe-define-bitmaps)
         (set (make-local-variable 'diff-hl-dired-process-buffer) nil)
-        (add-hook 'dired-after-readin-hook 'diff-hl-dired-update nil t))
+        (add-hook 'dired-after-readin-hook 'diff-hl-dired-update 10 t))
     (remove-hook 'dired-after-readin-hook 'diff-hl-dired-update t)
     (diff-hl-dired-clear)))

Although it doesn't seem necessary in my testing, but probably because of the way the hooks got settled because of the loading order.

But you'll want the update happen when dired-subtree-insert is called directly. So I can suggest checking this-command:

(defun diff-hl-dired-update-if-subtree-insert ()
  (when (eq this-command 'dired-subtree-insert)
    (diff-hl-dired-update)))

(add-hook 'dired-subtree-after-insert-hook #'diff-hl-dired-update-if-subtree-insert)

This config seems to work in my testing, without tracking the state of the update, or re-launching it. That should help simplify the patch.

It looks nice, thanks.
I've tested your approach for the last few days and it works.

That said, and speaking of the contribution as a whole, for accumulated patches of roughly >15 lines the FSF has a process to collect copyright assignments. And diff-hl is a part of Emacs in this regard.

So please LMK if you will be willing to sign such a paper (it's been done by a lot of people, and for citizens of some countries such as US and Germany, the process can be done electronically and quickly). If no - we'll need to reduce the size of the change to under 15 lines, preferably under 10 (just additions, not deletions). But with my suggestion above it might still be doable.

I've tried to reduce my patch as much as possible, but it's still more than 15 lines because of the changes to handle nested paths.

Anyway, I'm willing to sign the needed paper. Could you explain how the process works in my case? I'm a citizen of Russia.

[dgutov]

Great! Thanks for testing.

I've tried to reduce my patch as much as possible, but it's still more than 15 lines because of the changes to handle nested paths.

I suppose that depends on how to count the changes (some lines are only different in whitespace). Still, if we go by strict definition, it's over the limit, and signing the papers will invite you to contribute to more and different parts of Emacs! ✨ ✨ ✨

Anyway, I'm willing to sign the needed paper. Could you explain how the process works in my case? I'm a citizen of Russia.

Sure. You take the form linked below and send the result to the instructed email. Then you wait for the response and the subsequent instructions in the coming email (it might take a few weeks). Some countries' citizens can sign electronically, but that was not the case for Russians last I checked. So it will probably require physical mail, to send the final signed document.

In any case, we won't have to wait for all of it to complete before merging the change - announcing the intention and starting the process is considered enough for the initial contributions.

Savannah's CGit seems down today, so the link will be this: https://raw.githubusercontent.com/emacs-mirror/emacs/refs/heads/master/etc/copyright-assign.txt

Please keep the answer to the first question unchanged (the program is "Emacs" as a whole).

Thanks.

[rynffoll]

Great! Thanks for testing.

I've tried to reduce my patch as much as possible, but it's still more than 15 lines because of the changes to handle nested paths.

I suppose that depends on how to count the changes (some lines are only different in whitespace). Still, if we go by strict definition, it's over the limit, and signing the papers will invite you to contribute to more and different parts of Emacs! ✨ ✨ ✨

Anyway, I'm willing to sign the needed paper. Could you explain how the process works in my case? I'm a citizen of Russia.

Sure. You take the form linked below and send the result to the instructed email. Then you wait for the response and the subsequent instructions in the coming email (it might take a few weeks). Some countries' citizens can sign electronically, but that was not the case for Russians last I checked. So it will probably require physical mail, to send the final signed document.

In any case, we won't have to wait for all of it to complete before merging the change - announcing the intention and starting the process is considered enough for the initial contributions.

Savannah's CGit seems down today, so the link will be this: https://raw.githubusercontent.com/emacs-mirror/emacs/refs/heads/master/etc/copyright-assign.txt

Please keep the answer to the first question unchanged (the program is "Emacs" as a whole).

Thanks.

I've sent the request to assign@gnu.org, thanks for your help.