Fix #171 make Magit hooks great again

[wyuenho]

Re: #171 calling magit-unstaged-files when magit is refreshing will always result in nil because magit only refreshes when everything is done.

[dgutov]

@tarsius Could you review this patch?

This seems like a problematic change triggering a process call per buffer. Which means O(N) for a fairly expensive O.

IIUC the previously used (magit-unstaged-files t) didn't have this problem -- it either accessed a pre-existing list or fetched it with one process call. Right?

[wyuenho]

But the properties for vc-state are cached in an obarray?

[dgutov]

Cached, yes.

But if they have been reset and not updated for each of those buffers yet, the effect will be exactly as described.

Whether that is the case, again, depends on what Magit does behind the scenes.

[wyuenho]

I'm going to close this PR. While I think this PR is perfectly reasonable, it's still not the most thorough way to keep all git backed buffers up to date. The One True Way to keep all git backed buffers up to date to listen to changes from one single source of truth and then trigger the necessary updates on all relevant buffers. I'm dropping the following hack for posterity. The code size could perhaps be cut in half, or there may be bugs, but it's a good start.

;; Refresh VC related states reliably across packages.
(when file-notify--library
  (defvar watched-git-heads nil
    "A list of .git/logs/HEAD file to watcher mappings.")

  (defun refresh-git-backed-buffer ()
    "Refresh the current buffer base on the current git state."
    (if (and (boundp 'magit-mode) (derived-mode-p 'magit-mode))
        (magit-refresh-buffer)
      (unless revert-buffer-in-progress-p
        (cond
         ((or (bound-and-true-p diff-hl-flydiff-mode)
              (bound-and-true-p diff-hl-mode))
          (diff-hl-update))
         ((bound-and-true-p diff-hl-dired-mode)
          (diff-hl-dired-update))
         ((bound-and-true-p vc-dir-mode)
          (vc-dir-refresh)
          (when (bound-and-true-p diff-hl-dir-mode)
            (diff-hl-dir-update)))
         ((vc-backend (buffer-file-name))
          (vc-refresh-state))))))

  (defun handle-git-state-change (event)
    "Callback to `file-notify-add-watch' to handle git state change.

EVENT is a file system notification event.  If EVENT is a change
event, find all the visible buffers belonging to the same git
repo and refresh all of the version control minor mode states.
If EVENT is a deleted or renamed event, remove the file system
notification watcher.

See `file-notify-add-watch' for more details."
    (pcase-let ((`(,desc ,action ,file ,@_) event))
      (pcase action
        ((or 'deleted 'renamed)
         (file-notify-rm-watch desc))
        ('changed
         (let ((vc-root (apply 'file-name-concat "/" (seq-subseq (split-string file "/") 0 -3))))
           (dolist (frame (frame-list))
             (when (not (frame-parent frame))
               (dolist (buf (buffer-list frame))
                 (when (not (string-prefix-p " " (buffer-name buf)))
                   (with-current-buffer buf
                     (when (string-prefix-p vc-root (expand-file-name default-directory))
                       (refresh-git-backed-buffer))))))))))))

  (defun watch-git-head ()
    (let ((file (buffer-file-name)))
      (when (eq 'Git (vc-backend file))
        (let* ((root (expand-file-name (vc-git-root file)))
               (head (file-name-concat root ".git" "logs" "HEAD")))
          (unless (assoc-default head watched-git-heads)
            (ignore-errors
              (setf (alist-get head watched-git-heads nil nil 'equal)
                    (file-notify-add-watch head '(change) 'handle-git-state-change))))))))

  (defun reap-git-head-watchers ()
    (when-let* ((buffer-default-dirs-sans-current
                 (seq-uniq
                  (mapcar
                   (lambda (buf) (with-current-buffer buf (expand-file-name default-directory)))
                   (seq-filter
                    (lambda (buf)
                      (not (or (string-prefix-p " " (buffer-name buf))
                               (equal buf (current-buffer)))))
                    (apply 'append
                           (mapcar
                            'buffer-list
                            (seq-filter
                             (lambda (frame) (not (frame-parent frame)))
                             (frame-list))))))))
                (git-heads-to-delete
                 (seq-filter
                  (lambda (git-head)
                    (let ((vc-root (apply 'file-name-concat "/" (seq-subseq (split-string git-head "/") 0 -3))))
                      (not (seq-some (apply-partially 'string-prefix-p vc-root) buffer-default-dirs-sans-current))))
                  (mapcar 'car watched-git-heads))))

      (dolist (git-head git-heads-to-delete)
        (let ((watcher (assoc-default git-head watched-git-heads)))
          (file-notify-rm-watch watcher)
          (setf (alist-get git-head watched-git-heads nil t 'equal) nil)))))

  (add-hook 'find-file-hook 'watch-git-head)
  (add-hook 'kill-buffer-hook 'reap-git-head-watchers))

[dgutov]

I think the most productive approach here would be to ping Jonas from time to time, until he either provides his own solution, or blesses one of yours. I don't have the mental bandwidth right now to get into which of the approaches is better for Magit (which I don't use). Thanks either way.

…

On Wed, Aug 31, 2022, at 1:59 AM, Jimmy Yuen Ho Wong wrote: Closed #172 <#172>. — Reply to this email directly, view it on GitHub <#172 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACCMBKNHYLDJWGSXMC7JOTV32G5PANCNFSM5FT62LPA>. You are receiving this because you commented.Message ID: ***@***.***>

[tarsius]

Yeah, this has kept falling through the cracks. But it is a "just one more thing" issue, and I am currently trying to take a break from those (see https://www.reddit.com/r/emacs/comments/11f7urf/thanks_for_your_support_and_patience/). I still plan to look into this again at some point, but won't get around to that for at least another few weeks.