diff-hl-magit-post-refresh doesn't work anymore
wyuenho opened this issue · 28 comments
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?
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).
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 the 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?
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.
Why can't you take a snapshot 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.
Isn't that what I'm going?
But I have to use Magit's hooks to find out when the "refresh" is happening.
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.
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?
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.
How do you even turn off vc support for git anyway?
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))
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.
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.
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.
Right.
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-hookisn'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 whendiff-hl-magit-pre-refreshcallsmagit-unstaged-files, then that always returns the exact same list of files as the later call indiff-hl-magit-post-refresh. In fact the lists are evenequalbecause the second call uses a cached result.magit-pre-call-git-hookandmagit-pre-start-git-hookwould be the correct hooks for the "pre" function. -
magit-unstaged-filesdoesn't necessarily return all "modified" files. It was once namedmagit-modified-filesbut 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. #172 fixes that by usingvc-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.)
Thanks, Jonas!
That certainly works for me.
No, sorry I haven't gotten around to that yet.
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.
Good deal, let me know if there's anything I can help with.
@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.
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.
@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.
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.