From 595490893cda28a61255ab79c001a7db7fa9aef7 Mon Sep 17 00:00:00 2001 From: Craig Jennings Date: Sun, 14 Jun 2026 14:19:45 -0500 Subject: fix(modeline): drop per-render truename, guard vc fetch against signals The VC modeline cache rebuilt its key every render, and the key included file-truename, so a stat ran on every redisplay rather than once per refresh as the comment claimed. Now it keys on (file show-remote). A moved symlink target is caught at the next TTL refresh, when vc-backend resolves the link fresh. And cj/modeline-vc-fetch is wrapped in condition-case returning nil, so a git signal on a slow or unmounted filesystem degrades to no-VC-info instead of breaking all redisplay. --- modules/modeline-config.el | 43 ++++++++++++--------- tests/test-modeline-config-vc-cache-key.el | 60 ++++++++++-------------------- tests/test-modeline-config-vc-cache.el | 7 ++++ 3 files changed, 52 insertions(+), 58 deletions(-) diff --git a/modules/modeline-config.el b/modules/modeline-config.el index 0e6e5d0fb..21ecd7e47 100644 --- a/modules/modeline-config.el +++ b/modules/modeline-config.el @@ -137,12 +137,12 @@ Uses built-in cached values for performance.") cj/modeline-vc-cache-set-p nil)) (defun cj/modeline-vc-cache-key (file) - "Return the cache key for FILE. -Includes the resolved `file-truename' so that if FILE is a symlink whose -target moves to a different VC tree, the key changes and the cache is not -served a stale backend. The extra `file-truename' is one stat per refresh, -cheap next to the VC calls the cache avoids." - (list file (file-truename file) cj/modeline-vc-show-remote)) + "Return the cache key for FILE: the file path and `cj/modeline-vc-show-remote'. +`file-truename' is deliberately omitted -- the mode-line rebuilds this key on +every render to check cache validity, so a stat here would run per redisplay. +A symlink whose target moves to a different VC tree is picked up at the next +TTL refresh, when `vc-backend' resolves the link fresh." + (list file cj/modeline-vc-show-remote)) (defun cj/modeline-vc-cache-valid-p (key now) "Return non-nil when cached VC data is valid for KEY at NOW." @@ -157,18 +157,25 @@ Return a plist with `:branch' and `:state', or nil when FILE has no VC data. Uses `vc-git--symbolic-ref' for branch names when available (it returns the symbolic ref like \"main\" instead of a SHA when HEAD is on a branch), but falls back to `vc-working-revision' if the internal accessor is missing -- -the symbol is internal and can be renamed or removed between Emacs versions." - (unless (and (file-remote-p file) (not cj/modeline-vc-show-remote)) - (when-let* ((backend (vc-backend file)) - (branch (vc-working-revision file backend))) - (when (eq backend 'Git) - (unless (fboundp 'vc-git--symbolic-ref) - (require 'vc-git nil 'noerror)) - (when (fboundp 'vc-git--symbolic-ref) - (when-let* ((symbolic (ignore-errors (vc-git--symbolic-ref file)))) - (setq branch symbolic)))) - (list :branch branch - :state (vc-state file backend))))) +the symbol is internal and can be renamed or removed between Emacs versions. + +The whole VC probe is wrapped in `condition-case' returning nil. These are +synchronous git calls that, on TTL expiry, run while the mode-line is built; +on a slow or unmounted filesystem a signal here would land in redisplay and +break it. Caching nil degrades to \"no VC info\" instead." + (condition-case nil + (unless (and (file-remote-p file) (not cj/modeline-vc-show-remote)) + (when-let* ((backend (vc-backend file)) + (branch (vc-working-revision file backend))) + (when (eq backend 'Git) + (unless (fboundp 'vc-git--symbolic-ref) + (require 'vc-git nil 'noerror)) + (when (fboundp 'vc-git--symbolic-ref) + (when-let* ((symbolic (ignore-errors (vc-git--symbolic-ref file)))) + (setq branch symbolic)))) + (list :branch branch + :state (vc-state file backend)))) + (error nil))) (defun cj/modeline-vc-info () "Return cached modeline VC data for the current buffer." diff --git a/tests/test-modeline-config-vc-cache-key.el b/tests/test-modeline-config-vc-cache-key.el index ae869f4b8..6ba7985c2 100644 --- a/tests/test-modeline-config-vc-cache-key.el +++ b/tests/test-modeline-config-vc-cache-key.el @@ -1,56 +1,36 @@ ;;; test-modeline-config-vc-cache-key.el --- Tests for VC modeline cache key -*- lexical-binding: t; -*- ;;; Commentary: -;; The VC modeline cache keys on the file. A symlink whose target moves to a -;; different VC tree must invalidate the cache, so the key includes the -;; resolved `file-truename', not just the symlink path. +;; The VC modeline cache keys on the file path and the `cj/modeline-vc-show-remote' +;; flag only. `file-truename' is deliberately NOT in the key: it would run on +;; every redisplay (the mode-line rebuilds the key each render to check validity), +;; and a moved symlink target is picked up at the next TTL refresh anyway, since +;; `vc-backend' resolves the link fresh. The per-render stat isn't worth it. ;;; Code: (require 'ert) -(require 'cl-lib) (add-to-list 'load-path (expand-file-name "modules" user-emacs-directory)) (require 'modeline-config) -;;; Normal Cases - -(ert-deftest test-modeline-vc-cache-key-includes-truename () - "Normal: the cache key includes the resolved truename of the file." - (let ((f (make-temp-file "cj-mlkey-"))) - (unwind-protect - (should (member (file-truename f) (cj/modeline-vc-cache-key f))) - (delete-file f)))) - -;;; Boundary Cases - -(ert-deftest test-modeline-vc-cache-key-changes-when-symlink-target-moves () - "Boundary: re-pointing a symlink to a new target changes the cache key. -The symlink path is identical both times; only its truename differs, so a -key that ignored the truename would serve a stale VC backend." - (let* ((dir (make-temp-file "cj-mlkey-dir-" t)) - (target-a (expand-file-name "a" dir)) - (target-b (expand-file-name "b" dir)) - (link (expand-file-name "link" dir))) - (unwind-protect - (progn - (write-region "" nil target-a) - (write-region "" nil target-b) - (make-symbolic-link target-a link) - (let ((key-a (cj/modeline-vc-cache-key link))) - (delete-file link) - (make-symbolic-link target-b link) - (let ((key-b (cj/modeline-vc-cache-key link))) - (should-not (equal key-a key-b))))) - (delete-directory dir t)))) +(ert-deftest test-modeline-vc-cache-key-is-file-and-show-remote () + "Normal: the key is (FILE SHOW-REMOTE), with no per-render file-truename stat." + (let ((cj/modeline-vc-show-remote nil)) + (should (equal (cj/modeline-vc-cache-key "/x/y.el") '("/x/y.el" nil))))) + +(ert-deftest test-modeline-vc-cache-key-tracks-show-remote () + "Boundary: toggling show-remote yields a different key (separate cache entry)." + (should-not (equal (let ((cj/modeline-vc-show-remote nil)) + (cj/modeline-vc-cache-key "/x/y.el")) + (let ((cj/modeline-vc-show-remote t)) + (cj/modeline-vc-cache-key "/x/y.el"))))) (ert-deftest test-modeline-vc-cache-key-stable-for-same-file () - "Boundary: the key is stable across calls for an unchanged file." - (let ((f (make-temp-file "cj-mlkey-stable-"))) - (unwind-protect - (should (equal (cj/modeline-vc-cache-key f) - (cj/modeline-vc-cache-key f))) - (delete-file f)))) + "Boundary: the key is stable across calls for an unchanged file + show-remote." + (let ((cj/modeline-vc-show-remote nil)) + (should (equal (cj/modeline-vc-cache-key "/x/y.el") + (cj/modeline-vc-cache-key "/x/y.el"))))) (provide 'test-modeline-config-vc-cache-key) ;;; test-modeline-config-vc-cache-key.el ends here diff --git a/tests/test-modeline-config-vc-cache.el b/tests/test-modeline-config-vc-cache.el index b6aafbfbe..dab755442 100644 --- a/tests/test-modeline-config-vc-cache.el +++ b/tests/test-modeline-config-vc-cache.el @@ -98,5 +98,12 @@ (should (text-property-any 0 (length rendered) 'mouse-face 'mode-line-highlight rendered))))) +(ert-deftest test-modeline-config-vc-fetch-swallows-vc-errors () + "Error: a signal from the VC backend is swallowed (returns nil) rather than +propagating into the mode-line redisplay path, where it would break all redisplay." + (cl-letf (((symbol-function 'file-remote-p) (lambda (&rest _) nil)) + ((symbol-function 'vc-backend) (lambda (&rest _) (error "git boom")))) + (should (null (cj/modeline-vc-fetch "/tmp/project/file.el"))))) + (provide 'test-modeline-config-vc-cache) ;;; test-modeline-config-vc-cache.el ends here -- cgit v1.2.3