bug#59618: [PATCH] bug#59618: 29.0.50; global-whitespace-mode + org-capture: "Marker does not point anywhere"

[Richard Hansen <rhansen@rhansen.org>]

[-- Attachment #1.1.1: Type: text/plain, Size: 130 bytes --]

Attached is a new version of the patch.  The only change is that it was rebased onto latest emacs-29 so that it applies cleanly.

[-- Attachment #1.1.2: 0001-whitespace-Avoid-mutating-original-buffer-s-markers-.patch --]
[-- Type: text/x-patch, Size: 6806 bytes --]

From 41b0e596f32769f9601bcb3cc1f91cc2cb36641e Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Tue, 13 Dec 2022 01:33:43 -0500
Subject: [PATCH] whitespace: Avoid mutating original buffer's markers in
 clones

* lisp/whitespace.el (whitespace--clone): New hook function that is
run after cloning a buffer that copies `whitespace-bob-marker' and
`whitespace-eob-marker' and changes the copies to point to the new
buffer (Bug#59618).
(whitespace-color-on): Register the hook function.
(whitespace-color-off): Unregister the hook function.
---
 lisp/whitespace.el            | 15 +++++++
 test/lisp/whitespace-tests.el | 75 +++++++++++++++++++++++++++++++++++
 2 files changed, 90 insertions(+)

diff --git a/lisp/whitespace.el b/lisp/whitespace.el
index 9bc6ad9db4..558be1841a 100644
--- a/lisp/whitespace.el
+++ b/lisp/whitespace.el
@@ -2093,6 +2093,17 @@ whitespace-style-face-p
        t))
 
 
+(defun whitespace--clone ()
+  "Hook function run after `make-indirect-buffer' and `clone-buffer'."
+  (when (whitespace-style-face-p)
+    (setq-local whitespace-bob-marker
+                (copy-marker (marker-position whitespace-bob-marker)
+                             (marker-insertion-type whitespace-bob-marker)))
+    (setq-local whitespace-eob-marker
+                (copy-marker (marker-position whitespace-eob-marker)
+                             (marker-insertion-type whitespace-eob-marker)))))
+
+
 (defun whitespace-color-on ()
   "Turn on color visualization."
   (when (whitespace-style-face-p)
@@ -2111,6 +2122,8 @@ whitespace-color-on
               ;; The -1 ensures that it runs before any
               ;; `font-lock-mode' hook functions.
               -1 t)
+    (add-hook 'clone-buffer-hook #'whitespace--clone nil t)
+    (add-hook 'clone-indirect-buffer-hook #'whitespace--clone nil t)
     ;; Add whitespace-mode color into font lock.
     (setq
      whitespace-font-lock-keywords
@@ -2204,6 +2217,8 @@ whitespace-color-off
     (remove-hook 'before-change-functions #'whitespace-buffer-changed t)
     (remove-hook 'after-change-functions #'whitespace--update-bob-eob
                  t)
+    (remove-hook 'clone-buffer-hook #'whitespace--clone t)
+    (remove-hook 'clone-indirect-buffer-hook #'whitespace--clone t)
     (font-lock-remove-keywords nil whitespace-font-lock-keywords)
     (font-lock-flush)))
 
diff --git a/test/lisp/whitespace-tests.el b/test/lisp/whitespace-tests.el
index 3e94d7e921..12f6cb99a2 100644
--- a/test/lisp/whitespace-tests.el
+++ b/test/lisp/whitespace-tests.el
@@ -42,6 +42,13 @@ whitespace-tests--with-test-buffer
           '(whitespace-mode 1))
        ,@body)))
 
+(defmacro whitespace--with-buffer-selected (buffer-or-name &rest body)
+  (declare (debug (form body)) (indent 1))
+  `(save-window-excursion
+     (with-current-buffer (or ,buffer-or-name (current-buffer))
+       (with-selected-window (display-buffer (current-buffer))
+         ,@body))))
+
 (defun whitespace-tests--faceup (&rest lines)
   "Convenience wrapper around `faceup-test-font-lock-buffer'.
 Returns non-nil if the concatenated LINES match the current
@@ -337,6 +344,74 @@ whitespace-tests--empty-bob-eob-modified
       (whitespace-mode 1)
       (should (not (buffer-modified-p))))))
 
+(ert-deftest whitespace-tests--indirect-clone-breaks-base-markers ()
+  "Specific regression test for Bug#59618."
+  (whitespace-tests--with-test-buffer '(face empty)
+    (insert "\nx\n\n")
+    (let ((base (current-buffer))
+          ;; `unwind-protect' is not used to clean up `indirect'
+          ;; because the buffer should only be killed on success.
+          (indirect (clone-indirect-buffer (buffer-name) nil)))
+      (should (eq (marker-buffer whitespace-bob-marker) base))
+      (should (eq (marker-buffer whitespace-eob-marker) base))
+      (whitespace--with-buffer-selected indirect
+        ;; Mutate the indirect buffer to update its bob/eob markers.
+        (execute-kbd-macro (kbd "z RET M-< a")))
+      ;; With Bug#59618, the above mutation would cause the base
+      ;; buffer's markers to point inside the indirect buffer because
+      ;; the indirect buffer erroneously shared marker objects with
+      ;; the base buffer.  Killing the indirect buffer would then
+      ;; invalidate those markers (make them point nowhere).
+      (kill-buffer indirect)
+      (should (eq (marker-buffer whitespace-bob-marker) base))
+      (should (eq (marker-buffer whitespace-eob-marker) base)))))
+
+(defun whitespace-tests--check-markers (buf bpos epos)
+  (with-current-buffer buf
+    (should (eq (marker-buffer whitespace-bob-marker) buf))
+    (should (eq (marker-position whitespace-bob-marker) bpos))
+    (should (eq (marker-buffer whitespace-eob-marker) buf))
+    (should (eq (marker-position whitespace-eob-marker) epos))))
+
+(ert-deftest whitespace-tests--indirect-clone-markers ()
+  "Test `whitespace--clone' on indirect clones."
+  (whitespace-tests--with-test-buffer '(face empty)
+    (insert "\nx\n\n")
+    (let ((base (current-buffer))
+          ;; `unwind-protect' is not used to clean up `indirect'
+          ;; because the buffer should only be killed on success.
+          (indirect (clone-indirect-buffer nil nil)))
+      (whitespace-tests--check-markers base 2 4)
+      (whitespace--with-buffer-selected indirect
+        (whitespace-tests--check-markers indirect 2 4)
+        ;; Mutate the buffer to trigger `after-change-functions' and
+        ;; thus `whitespace--update-bob-eob'.
+        (execute-kbd-macro (kbd "z RET M-< a"))
+        (whitespace-tests--check-markers indirect 1 8))
+      (kill-buffer indirect)
+      ;; When the buffer was modified above, the new "a" character at
+      ;; the beginning moved the base buffer's markers by one.  Emacs
+      ;; did not run the base buffer's `after-change-functions' after
+      ;; the indirect buffer was edited (Bug#46982), so the end result
+      ;; is just the shift by one.
+      (whitespace-tests--check-markers base 3 5))))
+
+(ert-deftest whitespace-tests--regular-clone-markers ()
+  "Test `whitespace--clone' on regular clones."
+  (whitespace-tests--with-test-buffer '(face empty)
+    (insert "\nx\n\n")
+    (let ((orig (current-buffer))
+          ;; `unwind-protect' is not used to clean up `clone' because
+          ;; the buffer should only be killed on success.
+          (clone (clone-buffer)))
+      (whitespace-tests--check-markers orig 2 4)
+      (whitespace--with-buffer-selected clone
+        (whitespace-tests--check-markers clone 2 4)
+        (execute-kbd-macro (kbd "z RET M-< a"))
+        (whitespace-tests--check-markers clone 1 8))
+      (kill-buffer clone)
+      (whitespace-tests--check-markers orig 2 4))))
+
 (provide 'whitespace-tests)
 
 ;;; whitespace-tests.el ends here
-- 
2.39.0


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]