diff-hl-magit-post-refresh doesn't work anymore

[wyuenho]

I'm on the emacs-28 branch HEAD and using latest Magit and libgit from Melpa, I've followed the README to do

(with-eval-after-load 'magit
    (add-hook 'magit-pre-refresh-hook 'diff-hl-magit-pre-refresh)
    (add-hook 'magit-post-refresh-hook 'diff-hl-magit-post-refresh))

However, the post-fresh hook doesn't seem to be able to call diff-hl-update update anymore after magit-commit. The reason seems to be after magit-commit, the file is considered as up-to-date by git, so neither of the branches will hit diff-hl-update.

What I don't understand is, why is diff-hl-magit-post-refresh so complicated? Why not just call diff-hl-update as long as the file still exists on disk and in vc?

[dgutov]

AFAIK the code worked in the past, and I'm not a Magit user, to be able to ensure that it continues working after every update.

The reason seems to be after magit-commit, the file is considered as up-to-date by git, so neither of the branches will hit diff-hl-update.

It's not a problem if the file wasn't up-to-date before the refresh. If so, it should have been saved to diff-hl--magit-unstaged-files. And (member file modified-files) should return non-nil.

The problem with calling diff-hl-update in every buffer is, well, the operation slows down linearly with the number of buffers. vc-state value, on the other hand, might be cached since some previous operation (Magit probably updates it).

[wyuenho]

The problem is, (magit-unstaged-files t) isn't going to return the file as unstaged, so the first branch won't be hit either. The prerequisite of being able to commit a file is to stage it. In fact, Magit only refreshes after staging or unstaging is done, for any operation. Furthermore, there's no magit-pre-stage-hook either, there's only magit-post-stage-hook. So, if the intention is to catch Magit before any work is done, and then only update the buffers that Magit has done work on, this implementation isn't going to work. I too remember this used to work for commit about a year ago, but not anymore.

I'm not sure how best to fix this without calling (diff-hl-update) on every buffer. I mean, how many buffers can one session be open anyway? If you just have one large buffer with lots of formatting changes, you don't even need to have many buffers to slow down the update, just that one might be enough. So, is this optimization necessary and sufficient?

[dgutov]

I too remember this used to work for commit about a year ago, but not anymore.

Perhaps @tarsius could advise.

I mean, how many buffers can one session be open anyway?

Recalling certain bug reports on the Emacs bug tracker, you would be surprised.

[wyuenho]

Why can't you take a snapshot of the vc-state of all the files with diff-hl-mode turned on pre-refresh, and then just diff them again post-fresh? This way you don't have to go thru magit at all.

[dgutov]

Isn't that what I'm going?

But I have to use Magit's hooks to find out when the "refresh" is happening.

[tarsius]

Briefly, I don't remember making any changes over the last year that might cause a change in behavior but while I use diff-hl and find it useful, I apparently don't rely on it enough to have noticed and change.

I can look at how magit and diff-hl interact again at a later time, but right now I have to much on my plate to get going right away.

[tarsius]

Ah, I see there's a patch already. Of course I am happy if I don't have to dig into this myself.

Please note though that some magit users have turned of vc support for git -- would this patch work for users who have done that?

[wyuenho]

some magit users have turned of vc support for git -- would this patch work for users who have done that

Probably not, but what we have now wouldn't work for them anyway.

[wyuenho]

How do you even turn off vc support for git anyway?

[tarsius]

Probably not, but what we have now wouldn't work for them anyway.

Since you wrote the patch and have now been made aware of this potential issue, you should investigate it. We should fix it for everyone.

How do you even turn off vc support for git anyway?

(setq vc-handled-backends (delete 'Git vc-handled-backends))

[wyuenho]

My patch is straightly an improvement. I'm satisfied with my patch. I don't have time to cater to those who have turned off the git backend and uses diff-hl, since that has never worked for them. Catering to them would require Magit changes to keep the files unstaged when pre-refresh is run. That sounds far far more complicated than what I need.

[dgutov]

@tarsius

Since you wrote the patch and have now been made aware of this potential issue, you should investigate it. We should fix it for everyone.

How would you fix it, aside from implementing Magit-specific code paths for that case? Like an override for diff-hl-changes-buffer, at least. And diff-hl would need to be made aware, somehow, of having to use that code path.

Anyway, it does indeed seem orthogonal to the present issue.

[tarsius]

@dgutov I'll try to look into this soon but I am a bit swamped right now.

[tarsius]

some magit users have turned of vc support for git -- would this patch work for users who have done that

Probably not, but what we have now wouldn't work for them anyway.

Turns out that is not a problem because diff-hl relies on VC being enabled anyway.

[dgutov]

Right.

[tarsius]

I have taken an in depth look and now think that this never worked properly. Not since fdbf34a and while I stopped my investigation there probably also not before. I believe the perception that this "worked a while ago" and now doesn't work anymore is due to certain states/state chances having always worked while others never did, and people remember the good cases and have run into the bad cases more often before reporting that there is "a regression".

Two major issues with the current implementation are:

• magit-pre-refresh-hook isn't the correct hook. I know I suggested that but I was wrong. This hook runs before magit buffers are refreshed but after git was run, so when diff-hl-magit-pre-refresh calls magit-unstaged-files, then that always returns the exact same list of files as the later call in diff-hl-magit-post-refresh. In fact the lists are even equal because the second call uses a cached result. magit-pre-call-git-hook and magit-pre-start-git-hook would be the correct hooks for the "pre" function.

• magit-unstaged-files doesn't necessarily return all "modified" files. It was once named magit-modified-files but that was a bug that has been fixed with the rename. If a file has staged changes, but no additional unstaged changes, then it is not included in the list of returned by this function. Fix #171 make Magit hooks great again #172 fixes that by using vc-state.

I now think this should be implemented in magit and have a little proof-of-concept implementation, though it hasn't been optimized at all yet. I'll improve that over the next week or two.

This implementation hooks into the autorevert functionality, which also updates the vc-state. So it is "accidentally optimized" by not needlessly refreshing the state twice for certain buffers. It also is limited to buffers from the current repository, but I haven't done anything to avoid setting the state (once) for files that don't actually need that. That could be done using the above hooks though, and I will of course experiment with that.

So all in all I recommend you just leave things as they are now; this was broken for five years and we can wait a few more weeks. (This is high on my priority list, but not all the way at the top.)

[dgutov]

Thanks, Jonas!

That certainly works for me.

[wyuenho]

Hi @tarsius , has this been implemented in magit yet?

[tarsius]

No, sorry I haven't gotten around to that yet.

[tarsius]

I cannot promise anything but I am almost caught up with leftovers, so it's possible I will look into this soonish. Then again I am trying to spend more time in the sun instead of on the computer in the summer months, so maybe not.

[wyuenho]

Good deal, let me know if there's anything I can help with.

[dgutov]

Here's a recent related report: #201

[wyuenho]

@tarsius How did your past 3 years go? Any chance you can put this at the very top of your todo list this year? If not, can you outline a workaround here please? Thanks.

[tarsius]

Priority one is getting releases out. Then I want to complete a very important secret project. Then a "work on things I actually want to work on" period must follow to avoid burnout. Then I have to work on earning more than 1/3 of the median income where I live (i.e., get above below poverty line). After that I plan to work on longstanding issues such as this.

[dgutov]

@tarsius I really hear you, on all of those points.

Perhaps when time permits, you could outline your idea of the solution, though? I'd suggest going with the simplest approach among the ones being considered.

EDIT: I understand that you might really want to review (and/or guide and/or fix) halfway solutions by somebody else, but perhaps some middle ground could be reached.

[dgutov]

Alternatively, somebody particularly motivated by this issue could start with their view of the solution, hopefully one that addresses the problems outlined in #171 (comment).

To reiterate what's already been said, a part of the change will likely be in Magit, so being familiar with its structure is a requirement.

[dpathakj]

@dgutov writes above:

The problem with calling diff-hl-update in every buffer is, well, the operation slows down linearly with the number of buffers.

What goes wrong in diff-hl if you apply updates only for visible buffers? There are likely to be much fewer of those. Does diff-hl not refresh its state if a buffer becomes visible where it wasn't before it doesn't seem to from my testing, but would that be problematic to add? (git-gutter seems to do that but I haven't tracked down exactly how.) A pessimistic update of all visible buffers where diff-hl is active might not be that bad. It should require no changes from magit.

This is what git-gutter offers (adding this to the right hooks is left to the user):

(defun git-gutter:update-all-windows ()
  "Update git-gutter information for all visible buffers."
  (interactive)
  (dolist (buf (buffer-list))
    (when (get-buffer-window buf 'visible)
      (with-current-buffer buf
        (when git-gutter-mode
          (git-gutter))))))

Calling it from a magit hook seems to work just fine, but I haven't tested it with a large number of buffers open. It is still technically linear in the number of buffers, because it has to check each buffer for visibility, but should be much cheaper than running (diff-hl-update) for every buffer. (It is actually O(num_buffers * num_windows) if I understand the implementation of get-buffer-window correctly.) I'd still expect it to be fairly fast with a large number of buffers, but I have not tested this hypothesis.

There may also be an emacs idiom for "do the following only for visible buffers" that is better than git-gutter's approach; I don't hack on this stuff often.

Actually, one possibly better mechanism might be something like:

(dolist (frame (visible-frame-list))
  (dolist (window (window-list frame))
    (with-current-buffer (window-buffer window)
      (when diff-hl-mode
        (diff-hl-update))))))

which avoids slowdowns in the case (buffer-list) is large. I don't actually know if there's a good reason git-gutter doesn't do it that way.

(It might be possible to do slightly better by only triggering an update for a buffer visiting a file inside the repository that triggered the update, but that optimization may not be worthwhile since this approach already tries to update very few buffers. I'd guess under 10 in the vast majority of cases, the median might be more like 1 or 2.)

[wyuenho]

I have a strong suspicion that something like this will work:

(let ((diff-hl--magit-staged-files))
    (add-hook 'git-commit-setup-hook
              (lambda ()
                (unless (and diff-hl-disable-on-remote
                             (file-remote-p default-directory))
                  (setq diff-hl--magit-staged-files
                        (magit-staged-files)))))

    (add-hook 'magit-post-refresh-hook
              (lambda ()
                (unless (and diff-hl-disable-on-remote
                             (file-remote-p default-directory))
                  (let* ((topdir (magit-toplevel))
                         (modified-files
                          (mapcar (lambda (file) (expand-file-name file topdir))
                                  diff-hl--magit-staged-files))
                         (unmodified-states '(up-to-date ignored unregistered)))
                    (setq diff-hl--magit-staged-files nil)
                    (dolist (buf (buffer-list))
                      (when (and (buffer-local-value 'diff-hl-mode buf)
                                 (not (buffer-modified-p buf))
                                 ;; Solve the "cloned indirect buffer" problem
                                 ;; (diff-hl-mode could be non-nil there, even if
                                 ;; buffer-file-name is nil):
                                 (buffer-file-name buf)
                                 (file-in-directory-p (buffer-file-name buf) topdir)
                                 (file-exists-p (buffer-file-name buf)))
                        (with-current-buffer buf
                          (let* ((file buffer-file-name)
                                 (backend (vc-backend file)))
                            (when backend
                              (cond
                               ((member file modified-files)
                                (when (memq (vc-state file) unmodified-states)
                                  (vc-state-refresh file backend))
                                (diff-hl-update))
                               ((not (memq (vc-state file backend) unmodified-states))
                                (vc-state-refresh file backend)
                                (diff-hl-update)))))))))))
              100))

[dgutov]

@dpathakj Updating all visible windows seems like a good idea for git-gutter, which I think updates the views after every command. We have it a bit more complicated here, with the default configuration only updating buffers after an edit was made and saved. With diff-hl-flydiff-mode it should work better, though.

Perhaps we should offer this approach as an option.

(Just commented on wyuenho's patch in the PR.)