bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Richard Hansen]

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

Attached patch:

     whitespace: Update bob, eob markers in base and indirect buffers

     When a buffer is changed, update `whitespace-bob-marker' and
     `whitespace-eob-marker' not just in the current buffer, but
     also in the base buffer and all of its indirect buffers (if
     any) (Bug#46982).
     * lisp/whitespace.el (whitespace--indirect-buffers,
     whitespace--refresh-indirect-buffers,
     whitespace-unload-function): Track the relationships between
     base buffers and their indirect buffers.
     (whitespace--update-bob-eob-all, whitespace-color-on,
     whitespace-color-off): Define a new function that calls
     `whitespace--update-bob-eob' on the base buffer and all of its
     indirect buffers, and use this new function instead of
     `whitespace--update-bob-eob' in `after-change-functions'.

See Bug#46982 for additional context.  This can also be thought of as a 
continuation of the fix for Bug#59618 made in commit 
d76d7a3bebf1ff0b06a38f7f96d316752844ed10.

I'm not sure if this patch belongs on master, on emacs-29, or nowhere:
   * The bug it fixes is very minor, so it might not be worth the added 
complexity.
   * It touches code that has been the source of a few recent bugs, so 
I'm wary of introducing new bugs onto emacs-29.
   * This is a workaround for Bug#46982; it would be better to fix that 
bug instead (although fixing that bug might be infeasible for reasons of 
backwards compatibility).

[-- Attachment #1.1.2: 0001-whitespace-Update-bob-eob-markers-in-base-and-indire.patch --]
[-- Type: text/x-patch, Size: 7944 bytes --]

From eb0caae32d2cb3d8d1d568d1054a33850c368477 Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Thu, 15 Dec 2022 01:48:06 -0500
Subject: [PATCH] whitespace: Update bob, eob markers in base and indirect
 buffers

When a buffer is changed, update `whitespace-bob-marker' and
`whitespace-eob-marker' not just in the current buffer, but
also in the base buffer and all of its indirect buffers (if
any) (Bug#46982).
* lisp/whitespace.el (whitespace--indirect-buffers,
whitespace--refresh-indirect-buffers,
whitespace-unload-function): Track the relationships between
base buffers and their indirect buffers.
(whitespace--update-bob-eob-all, whitespace-color-on,
whitespace-color-off): Define a new function that calls
`whitespace--update-bob-eob' on the base buffer and all of its
indirect buffers, and use this new function instead of
`whitespace--update-bob-eob' in `after-change-functions'.
---
 lisp/whitespace.el            | 58 +++++++++++++++++++++++++++++++++--
 test/lisp/whitespace-tests.el | 35 +++++++++++++++++----
 2 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/lisp/whitespace.el b/lisp/whitespace.el
index b747293eb4..bc9d4b7998 100644
--- a/lisp/whitespace.el
+++ b/lisp/whitespace.el
@@ -2104,6 +2104,26 @@ whitespace--clone
                              (marker-insertion-type whitespace-eob-marker)))))
 
 
+(defvar whitespace--indirect-buffers nil
+  "Plist mapping a base buffer to a list of its indirect buffers.
+
+Used to work around Bug#46982.")
+
+
+(defun whitespace--refresh-indirect-buffers ()
+  "Refresh `whitespace--indirect-buffers'.
+
+Used to work around Bug#46982."
+  (setq whitespace--indirect-buffers nil)
+  ;; Keep track of all buffers -- not just those with
+  ;; `whitespace-mode' enabled -- in case `whitespace-mode' is enabled
+  ;; later.
+  (dolist (buf (buffer-list))
+    (let ((base (buffer-base-buffer buf)))
+      (when base
+        (push buf (plist-get whitespace--indirect-buffers base))))))
+
+
 (defun whitespace-color-on ()
   "Turn on color visualization."
   (when (whitespace-style-face-p)
@@ -2118,7 +2138,7 @@ whitespace-color-on
     (setq-local whitespace-buffer-changed nil)
     (add-hook 'post-command-hook #'whitespace-post-command-hook nil t)
     (add-hook 'before-change-functions #'whitespace-buffer-changed nil t)
-    (add-hook 'after-change-functions #'whitespace--update-bob-eob
+    (add-hook 'after-change-functions #'whitespace--update-bob-eob-all
               ;; The -1 ensures that it runs before any
               ;; `font-lock-mode' hook functions.
               -1 t)
@@ -2215,8 +2235,8 @@ whitespace-color-off
   (when (whitespace-style-face-p)
     (remove-hook 'post-command-hook #'whitespace-post-command-hook t)
     (remove-hook 'before-change-functions #'whitespace-buffer-changed t)
-    (remove-hook 'after-change-functions #'whitespace--update-bob-eob
-                 t)
+    (remove-hook 'after-change-functions
+                 #'whitespace--update-bob-eob-all 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)
@@ -2401,6 +2421,32 @@ whitespace--variable-watcher
       (when whitespace-mode
         (font-lock-flush)))))
 
+(defun whitespace--update-bob-eob-all (&optional beg end &rest _)
+  "Call `whitespace--update-bob-eob' for the base and its indirect buffers.
+
+This function is intended to be used in `after-change-functions'.
+The `whitespace--update-bob-eob' function is only called for a
+buffer if the buffer has this function in its `after-change-functions'
+hook.  See `after-change-functions' for the meaning of BEG and
+END."
+  ;; Change hooks do not run for the base buffer when editing an
+  ;; indirect buffer, or for indirect buffers when editing the base
+  ;; buffer, even though the change affects all of them simultaneously
+  ;; (Bug#46982).  Work around that limitation by manually updating
+  ;; them all here.  `whitespace--update-bob-eob' is idempotent, so if
+  ;; Bug#46982 is fixed this should continue to work correctly (though
+  ;; it will be doing unnecessary work).
+  (let* ((base (or (buffer-base-buffer) (current-buffer)))
+         (indirect (plist-get whitespace--indirect-buffers base)))
+    (dolist (buf (cons base indirect))
+      (with-current-buffer buf
+        (when (memq #'whitespace--update-bob-eob-all
+                    after-change-functions)
+          ;; Positions in a base buffer always match positions in
+          ;; indirect buffers (even if narrowing differs) so there is
+          ;; no need to translate BEG or END.
+          (whitespace--update-bob-eob beg end))))))
+
 (defun whitespace--update-bob-eob (&optional beg end &rest _)
   "Update `whitespace-bob-marker' and `whitespace-eob-marker'.
 Also apply `font-lock-multiline' text property.  If BEG and END
@@ -2589,6 +2635,10 @@ whitespace-warn-read-only
 \f
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
+(whitespace--refresh-indirect-buffers)
+(add-hook 'buffer-list-update-hook
+          #'whitespace--refresh-indirect-buffers)
+
 (defvar whitespace--watched-vars
   '(fill-column indent-tabs-mode tab-width whitespace-line-column))
 
@@ -2605,6 +2655,8 @@ whitespace-unload-function
     (dolist (buf (buffer-list))
       (set-buffer buf)
       (whitespace-mode -1)))
+  (remove-hook 'buffer-list-update-hook
+               #'whitespace--refresh-indirect-buffers)
   nil)					; continue standard unloading
 
 
diff --git a/test/lisp/whitespace-tests.el b/test/lisp/whitespace-tests.el
index 9a9fb55e4f..513328f5c7 100644
--- a/test/lisp/whitespace-tests.el
+++ b/test/lisp/whitespace-tests.el
@@ -388,12 +388,9 @@ whitespace-tests--indirect-clone-markers
         (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))))
+      ;; The base buffer's markers have also been updated thanks to a
+      ;; workaround for Bug#46982.
+      (whitespace-tests--check-markers base 1 8))))
 
 (ert-deftest whitespace-tests--regular-clone-markers ()
   "Test `whitespace--clone' on regular clones."
@@ -411,6 +408,32 @@ whitespace-tests--regular-clone-markers
       (kill-buffer clone)
       (whitespace-tests--check-markers orig 2 4))))
 
+(ert-deftest whitespace-tests--indirect-mutual-marker-update ()
+  "Edits should update markers in base and all indirect buffers."
+  (whitespace-tests--with-test-buffer '(face empty)
+    (insert "\nx\n\n")
+    (let* ((indirects (list (clone-indirect-buffer nil nil)
+                            (clone-indirect-buffer nil nil)))
+           (bufs (cons (current-buffer) indirects)))
+      (dolist (editbuf bufs)
+        (dolist (buf bufs)
+          (whitespace-tests--check-markers buf 2 4))
+        (ert-with-buffer-selected editbuf
+          (buffer-enable-undo)
+          (undo-boundary)
+          (with-undo-amalgamate
+            (execute-kbd-macro (kbd "z RET M-< a"))))
+        (dolist (buf bufs)
+          (whitespace-tests--check-markers buf 1 8))
+        (ert-with-buffer-selected editbuf
+          (execute-kbd-macro (kbd "C-_")))
+        (dolist (buf bufs)
+          (whitespace-tests--check-markers buf 2 4)))
+      ;; `unwind-protect' is not used to clean up `indirects' because
+      ;; the buffers should only be killed on success.
+      (dolist (buf indirects)
+        (kill-buffer buf)))))
+
 (provide 'whitespace-tests)
 
 ;;; whitespace-tests.el ends here
-- 
2.39.0


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

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Ihor Radchenko]

Richard Hansen <rhansen@rhansen.org> writes:

> +(defvar whitespace--indirect-buffers nil
> +  "Plist mapping a base buffer to a list of its indirect buffers.
> +
> +Used to work around Bug#46982.")

Org uses a similar variable for similar purposes.
Would it make more sense to expose the list of indirect buffers for a
given buffer in more centralized way?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Eli Zaretskii]

> Cc: 60333@debbugs.gnu.org
> From: Ihor Radchenko <yantar92@posteo.net>
> Date: Tue, 27 Dec 2022 06:34:52 +0000
> 
> Richard Hansen <rhansen@rhansen.org> writes:
> 
> > +(defvar whitespace--indirect-buffers nil
> > +  "Plist mapping a base buffer to a list of its indirect buffers.
> > +
> > +Used to work around Bug#46982.")
> 
> Org uses a similar variable for similar purposes.
> Would it make more sense to expose the list of indirect buffers for a
> given buffer in more centralized way?

Could be a useful feature, I think.

Stefan, any comments?

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> +(defvar whitespace--indirect-buffers nil
>> +  "Plist mapping a base buffer to a list of its indirect buffers.
>> +Used to work around Bug#46982.")
> Org uses a similar variable for similar purposes.
> Would it make more sense to expose the list of indirect buffers for a
> given buffer in more centralized way?

"Expose" is the wrong term, because we don't have that info ready to
be exposed.  We'd either have to create&maintain that list, or compute
it on-demand when requested.

We could export some way for ELisp to check if BUFFER is the base buffer
of some other indirect buffers so as to skip the loop through
`buffer-list` in the common case.  So that sane code (i.e. code which
doesn't use indirect buffers) doesn't pay for the careless users of that
anti-feature :-)

If it weren't for backward compatibility, I'd suggest to make
`buffer-base-buffer` return t (rather than nil) when BUFFER is the base
buffer of other indirect buffers.


        Stefan

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:
>>> +(defvar whitespace--indirect-buffers nil
>>> +  "Plist mapping a base buffer to a list of its indirect buffers.
>>> +Used to work around Bug#46982.")
>> Org uses a similar variable for similar purposes.
>> Would it make more sense to expose the list of indirect buffers for a
>> given buffer in more centralized way?
>
> "Expose" is the wrong term, because we don't have that info ready to
> be exposed.  We'd either have to create&maintain that list, or compute
> it on-demand when requested.
>
> We could export some way for ELisp to check if BUFFER is the base buffer
> of some other indirect buffers so as to skip the loop through
> `buffer-list` in the common case.  So that sane code (i.e. code which
> doesn't use indirect buffers) doesn't pay for the careless users of that
> anti-feature :-)
>
> If it weren't for backward compatibility, I'd suggest to make
> `buffer-base-buffer` return t (rather than nil) when BUFFER is the base
> buffer of other indirect buffers.

What about:

1. Maintain internal indirect buffer list associated with buffers in C
2. Every time an indirect buffer is created, add the new buffer to that list
3. Add a new function `buffer-indirect-buffers' that will return the
   list of indirect buffers associated with the current buffer.
   The function will do something like:

   (unless (buffer-base-buffer buf)
     (let (result)
      (dolist (indirect BUFFER_INDIRECT(buf))
        (when (buffer-live-p indirect) (push indirect result)))
      BUFFER_INDIRECT(buf) = result
      result))

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> "Expose" is the wrong term, because we don't have that info ready to
>> be exposed.  We'd either have to create&maintain that list, or compute
>> it on-demand when requested.
[...]
> 1. Maintain internal indirect buffer list associated with buffers in C

That's the "create&maintain that list" option.
Personally I don't like it: I'd like to reduce the amount of support we
provide in C for indirect buffers rather than increase it.


        Stefan

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>     When a buffer is changed, update `whitespace-bob-marker' and
>     `whitespace-eob-marker' not just in the current buffer, but
>     also in the base buffer and all of its indirect buffers (if
>     any) (Bug#46982).

That doesn't sound right.  We should only update them in those indirect buffers
which also use whitespace eob/bob markers.  It should be their individual
responsibility to register hook functions to update their own markers.


        Stefan

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Richard Hansen]

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

On 12/28/22 09:51, Stefan Monnier wrote:
>>      When a buffer is changed, update `whitespace-bob-marker' and
>>      `whitespace-eob-marker' not just in the current buffer, but
>>      also in the base buffer and all of its indirect buffers (if
>>      any) (Bug#46982).
> 
> That doesn't sound right.  We should only update them in those indirect buffers
> which also use whitespace eob/bob markers.  It should be their individual
> responsibility to register hook functions to update their own markers.

The changelog was worded poorly; it does what you expect it to do. 
Attached is an updated patch with hopefully better wording; please let 
me know if it still needs improvement.

[-- Attachment #1.1.2: v2-0001-whitespace-Update-bob-eob-markers-in-base-and-ind.patch --]
[-- Type: text/x-patch, Size: 8006 bytes --]

From 1e5efbdac0201aa87873923ba62a605f977b4e5f Mon Sep 17 00:00:00 2001
From: Richard Hansen <rhansen@rhansen.org>
Date: Thu, 15 Dec 2022 01:48:06 -0500
Subject: [PATCH v2] whitespace: Update bob, eob markers in base and indirect
 buffers

When a buffer is changed, call `whitespace--update-bob-eob'
not just on the current buffer but also on the base buffer and
all of its indirect buffers where applicable (Bug#46982).
* lisp/whitespace.el (whitespace--indirect-buffers,
whitespace--refresh-indirect-buffers,
whitespace-unload-function): Track the relationships between
base buffers and their indirect buffers.
(whitespace--update-bob-eob-all, whitespace-color-on,
whitespace-color-off): Define a new function that calls
`whitespace--update-bob-eob' on the base buffer and all of its
indirect buffers that have `whitespace--update-bob-eob-all' in
their `after-change-functions', and use this new function
instead of `whitespace--update-bob-eob' in
`after-change-functions'.
---
 lisp/whitespace.el            | 58 +++++++++++++++++++++++++++++++++--
 test/lisp/whitespace-tests.el | 35 +++++++++++++++++----
 2 files changed, 84 insertions(+), 9 deletions(-)

diff --git a/lisp/whitespace.el b/lisp/whitespace.el
index b747293eb4..bc9d4b7998 100644
--- a/lisp/whitespace.el
+++ b/lisp/whitespace.el
@@ -2104,6 +2104,26 @@ whitespace--clone
                              (marker-insertion-type whitespace-eob-marker)))))
 
 
+(defvar whitespace--indirect-buffers nil
+  "Plist mapping a base buffer to a list of its indirect buffers.
+
+Used to work around Bug#46982.")
+
+
+(defun whitespace--refresh-indirect-buffers ()
+  "Refresh `whitespace--indirect-buffers'.
+
+Used to work around Bug#46982."
+  (setq whitespace--indirect-buffers nil)
+  ;; Keep track of all buffers -- not just those with
+  ;; `whitespace-mode' enabled -- in case `whitespace-mode' is enabled
+  ;; later.
+  (dolist (buf (buffer-list))
+    (let ((base (buffer-base-buffer buf)))
+      (when base
+        (push buf (plist-get whitespace--indirect-buffers base))))))
+
+
 (defun whitespace-color-on ()
   "Turn on color visualization."
   (when (whitespace-style-face-p)
@@ -2118,7 +2138,7 @@ whitespace-color-on
     (setq-local whitespace-buffer-changed nil)
     (add-hook 'post-command-hook #'whitespace-post-command-hook nil t)
     (add-hook 'before-change-functions #'whitespace-buffer-changed nil t)
-    (add-hook 'after-change-functions #'whitespace--update-bob-eob
+    (add-hook 'after-change-functions #'whitespace--update-bob-eob-all
               ;; The -1 ensures that it runs before any
               ;; `font-lock-mode' hook functions.
               -1 t)
@@ -2215,8 +2235,8 @@ whitespace-color-off
   (when (whitespace-style-face-p)
     (remove-hook 'post-command-hook #'whitespace-post-command-hook t)
     (remove-hook 'before-change-functions #'whitespace-buffer-changed t)
-    (remove-hook 'after-change-functions #'whitespace--update-bob-eob
-                 t)
+    (remove-hook 'after-change-functions
+                 #'whitespace--update-bob-eob-all 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)
@@ -2401,6 +2421,32 @@ whitespace--variable-watcher
       (when whitespace-mode
         (font-lock-flush)))))
 
+(defun whitespace--update-bob-eob-all (&optional beg end &rest _)
+  "Call `whitespace--update-bob-eob' for the base and its indirect buffers.
+
+This function is intended to be used in `after-change-functions'.
+The `whitespace--update-bob-eob' function is only called for a
+buffer if the buffer has this function in its `after-change-functions'
+hook.  See `after-change-functions' for the meaning of BEG and
+END."
+  ;; Change hooks do not run for the base buffer when editing an
+  ;; indirect buffer, or for indirect buffers when editing the base
+  ;; buffer, even though the change affects all of them simultaneously
+  ;; (Bug#46982).  Work around that limitation by manually updating
+  ;; them all here.  `whitespace--update-bob-eob' is idempotent, so if
+  ;; Bug#46982 is fixed this should continue to work correctly (though
+  ;; it will be doing unnecessary work).
+  (let* ((base (or (buffer-base-buffer) (current-buffer)))
+         (indirect (plist-get whitespace--indirect-buffers base)))
+    (dolist (buf (cons base indirect))
+      (with-current-buffer buf
+        (when (memq #'whitespace--update-bob-eob-all
+                    after-change-functions)
+          ;; Positions in a base buffer always match positions in
+          ;; indirect buffers (even if narrowing differs) so there is
+          ;; no need to translate BEG or END.
+          (whitespace--update-bob-eob beg end))))))
+
 (defun whitespace--update-bob-eob (&optional beg end &rest _)
   "Update `whitespace-bob-marker' and `whitespace-eob-marker'.
 Also apply `font-lock-multiline' text property.  If BEG and END
@@ -2589,6 +2635,10 @@ whitespace-warn-read-only
 \f
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 
+(whitespace--refresh-indirect-buffers)
+(add-hook 'buffer-list-update-hook
+          #'whitespace--refresh-indirect-buffers)
+
 (defvar whitespace--watched-vars
   '(fill-column indent-tabs-mode tab-width whitespace-line-column))
 
@@ -2605,6 +2655,8 @@ whitespace-unload-function
     (dolist (buf (buffer-list))
       (set-buffer buf)
       (whitespace-mode -1)))
+  (remove-hook 'buffer-list-update-hook
+               #'whitespace--refresh-indirect-buffers)
   nil)					; continue standard unloading
 
 
diff --git a/test/lisp/whitespace-tests.el b/test/lisp/whitespace-tests.el
index 9a9fb55e4f..513328f5c7 100644
--- a/test/lisp/whitespace-tests.el
+++ b/test/lisp/whitespace-tests.el
@@ -388,12 +388,9 @@ whitespace-tests--indirect-clone-markers
         (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))))
+      ;; The base buffer's markers have also been updated thanks to a
+      ;; workaround for Bug#46982.
+      (whitespace-tests--check-markers base 1 8))))
 
 (ert-deftest whitespace-tests--regular-clone-markers ()
   "Test `whitespace--clone' on regular clones."
@@ -411,6 +408,32 @@ whitespace-tests--regular-clone-markers
       (kill-buffer clone)
       (whitespace-tests--check-markers orig 2 4))))
 
+(ert-deftest whitespace-tests--indirect-mutual-marker-update ()
+  "Edits should update markers in base and all indirect buffers."
+  (whitespace-tests--with-test-buffer '(face empty)
+    (insert "\nx\n\n")
+    (let* ((indirects (list (clone-indirect-buffer nil nil)
+                            (clone-indirect-buffer nil nil)))
+           (bufs (cons (current-buffer) indirects)))
+      (dolist (editbuf bufs)
+        (dolist (buf bufs)
+          (whitespace-tests--check-markers buf 2 4))
+        (ert-with-buffer-selected editbuf
+          (buffer-enable-undo)
+          (undo-boundary)
+          (with-undo-amalgamate
+            (execute-kbd-macro (kbd "z RET M-< a"))))
+        (dolist (buf bufs)
+          (whitespace-tests--check-markers buf 1 8))
+        (ert-with-buffer-selected editbuf
+          (execute-kbd-macro (kbd "C-_")))
+        (dolist (buf bufs)
+          (whitespace-tests--check-markers buf 2 4)))
+      ;; `unwind-protect' is not used to clean up `indirects' because
+      ;; the buffers should only be killed on success.
+      (dolist (buf indirects)
+        (kill-buffer buf)))))
+
 (provide 'whitespace-tests)
 
 ;;; whitespace-tests.el ends here
-- 
2.39.0


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

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Richard Hansen]

[-- Attachment #1.1: Type: text/plain, Size: 2040 bytes --]

On 12/28/22 09:44, Stefan Monnier wrote:
>>> "Expose" is the wrong term, because we don't have that info ready to
>>> be exposed.  We'd either have to create&maintain that list, or compute
>>> it on-demand when requested.
> [...]
>> 1. Maintain internal indirect buffer list associated with buffers in C
> 
> That's the "create&maintain that list" option.
> Personally I don't like it: I'd like to reduce the amount of support we
> provide in C for indirect buffers rather than increase it.

What alternative would you prefer?

How about something like this in a shared location (e.g., simple.el next 
to `clone-indirect-buffer'):

     (defvar indirect-buffers--cached nil)

     (defun indirect-buffers--invalidate-cache ()
       (setq indirect-buffers--cached nil)
       (remove-hook 'buffer-list-update-hook
                    #'indirect-buffers--invalidate-cache))

     (defun indirect-buffers ()
       (unless indirect-buffers--cached
         (setq indirect-buffers--cached '(nil nil))
         (dolist (buf (buffer-list))
           (let ((base (buffer-base-buffer buf)))
              (when base
               (push buf (plist-get indirect-buffers--cached base)))))
         (add-hook 'buffer-list-update-hook
                   #'indirect-buffers--invalidate-cache))
       indirect-buffers--cached)

Other options I can see:

   #1: Apply this patch as-is, keep Org as-is, and live with the code 
duplication.
   #2: Reject this patch and keep Org as-is.
   #3: Advise `make-indirect-buffer'.  The advice would record new 
indirect buffers and add a `kill-buffer-hook' function to clean up the 
entry.  (Note, however, that `make-indirect-buffer' is a primitive 
function.)
   #4: Fix Bug#46982.  (One possible approach that maintains backwards 
compatibility:  Teach `after-change-functions' to look for a symbol 
property that means "run me not just for changes made in this buffer, 
but also if a change is made in this buffer's base/indirect buffer".)

Thanks,
Richard

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

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Ihor Radchenko]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

> ... I'd like to reduce the amount of support we
> provide in C for indirect buffers rather than increase it.

Indirect buffer list can be a buffer-local Elisp variable as well. Not
an internal buffer state.

Would it make sense to refactor `make-indirect-buffer' into Elisp part +
smaller C part?

-- 
Ihor Radchenko // yantar92,
Org mode contributor,
Learn more about Org mode at <https://orgmode.org/>.
Support Org development at <https://liberapay.com/org-mode>,
or support my work at <https://liberapay.com/yantar92>

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

> Would it make sense to refactor `make-indirect-buffer' into Elisp part +
> smaller C part?

I think that's where we're headed, yes.


        Stefan

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Would it make sense to refactor `make-indirect-buffer' into Elisp part +
>> smaller C part?
> I think that's where we're headed, yes.

And then the ELisp part of `make-indirect-buffer` could (among other
things) add buffer-local `after/before-change-functions` that make sure
to run the hooks in all the buffers sharing the same text.

This way whitespace.el wouldn't need to do anything special.


        Stefan

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Eli Zaretskii]

> Cc: 60333@debbugs.gnu.org
> Date: Thu, 29 Dec 2022 01:54:21 -0500
> From: Richard Hansen <rhansen@rhansen.org>
> 
> On 12/28/22 09:51, Stefan Monnier wrote:
> >>      When a buffer is changed, update `whitespace-bob-marker' and
> >>      `whitespace-eob-marker' not just in the current buffer, but
> >>      also in the base buffer and all of its indirect buffers (if
> >>      any) (Bug#46982).
> > 
> > That doesn't sound right.  We should only update them in those indirect buffers
> > which also use whitespace eob/bob markers.  It should be their individual
> > responsibility to register hook functions to update their own markers.
> 
> The changelog was worded poorly; it does what you expect it to do. 
> Attached is an updated patch with hopefully better wording; please let 
> me know if it still needs improvement.

Stefan, any further comments, or should this go in?

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Eli Zaretskii]

> Cc: 60333@debbugs.gnu.org
> Date: Thu, 29 Dec 2022 03:11:11 -0500
> From: Richard Hansen <rhansen@rhansen.org>
> 
> On 12/28/22 09:44, Stefan Monnier wrote:
> >>> "Expose" is the wrong term, because we don't have that info ready to
> >>> be exposed.  We'd either have to create&maintain that list, or compute
> >>> it on-demand when requested.
> > [...]
> >> 1. Maintain internal indirect buffer list associated with buffers in C
> > 
> > That's the "create&maintain that list" option.
> > Personally I don't like it: I'd like to reduce the amount of support we
> > provide in C for indirect buffers rather than increase it.
> 
> What alternative would you prefer?
> 
> How about something like this in a shared location (e.g., simple.el next 
> to `clone-indirect-buffer'):
> 
>      (defvar indirect-buffers--cached nil)
> 
>      (defun indirect-buffers--invalidate-cache ()
>        (setq indirect-buffers--cached nil)
>        (remove-hook 'buffer-list-update-hook
>                     #'indirect-buffers--invalidate-cache))
> 
>      (defun indirect-buffers ()
>        (unless indirect-buffers--cached
>          (setq indirect-buffers--cached '(nil nil))
>          (dolist (buf (buffer-list))
>            (let ((base (buffer-base-buffer buf)))
>               (when base
>                (push buf (plist-get indirect-buffers--cached base)))))
>          (add-hook 'buffer-list-update-hook
>                    #'indirect-buffers--invalidate-cache))
>        indirect-buffers--cached)
> 
> Other options I can see:
> 
>    #1: Apply this patch as-is, keep Org as-is, and live with the code 
> duplication.
>    #2: Reject this patch and keep Org as-is.
>    #3: Advise `make-indirect-buffer'.  The advice would record new 
> indirect buffers and add a `kill-buffer-hook' function to clean up the 
> entry.  (Note, however, that `make-indirect-buffer' is a primitive 
> function.)
>    #4: Fix Bug#46982.  (One possible approach that maintains backwards 
> compatibility:  Teach `after-change-functions' to look for a symbol 
> property that means "run me not just for changes made in this buffer, 
> but also if a change is made in this buffer's base/indirect buffer".)

And this.

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> On 12/28/22 09:51, Stefan Monnier wrote:
>> >>      When a buffer is changed, update `whitespace-bob-marker' and
>> >>      `whitespace-eob-marker' not just in the current buffer, but
>> >>      also in the base buffer and all of its indirect buffers (if
>> >>      any) (Bug#46982).
>> > 
>> > That doesn't sound right.  We should only update them in those indirect buffers
>> > which also use whitespace eob/bob markers.  It should be their individual
>> > responsibility to register hook functions to update their own markers.
>> 
>> The changelog was worded poorly; it does what you expect it to do. 
>> Attached is an updated patch with hopefully better wording; please let 
>> me know if it still needs improvement.
>
> Stefan, any further comments, or should this go in?

I think it shouldn't because it fixes a corner case of a much
wider problem.
E.g.:

    emacs -Q lisp/subr.el
    M-x make-indirect-buffer RET RET foo RET
    C-x 2 C-x b foo RET
    C-d C-d C-d

You should now see the first line of `subr.el` as:

    subr.el --- basic lisp subroutines for Emacs  -*- lexical-binding:t -*-

yet still highlighted as a comment because the buffer modification (and
hence the `after-change-functions`) was performed in the indirect buffer
which is in `fundamental-mode` and doesn't know it should mark the line
for re-fontification.


        Stefan

bug#60333: [PATCH] whitespace: Update bob, eob markers in base and indirect buffers

[Stefan Monnier via Bug reports for GNU Emacs, the Swiss army knife of text editors]

>> Other options I can see:
>> 
>>    #1: Apply this patch as-is, keep Org as-is, and live with the code 
>> duplication.
>>    #2: Reject this patch and keep Org as-is.
>>    #3: Advise `make-indirect-buffer'.  The advice would record new 
>> indirect buffers and add a `kill-buffer-hook' function to clean up the 
>> entry.  (Note, however, that `make-indirect-buffer' is a primitive 
>> function.)
>>    #4: Fix Bug#46982.  (One possible approach that maintains backwards 
>> compatibility:  Teach `after-change-functions' to look for a symbol 
>> property that means "run me not just for changes made in this buffer, 
>> but also if a change is made in this buffer's base/indirect buffer".)
>
> And this.

I think the saner way is to rename `make-indirect-buffer` to
`internal-make-indirect-buffer` and then define `make-indirect-buffer`
on top of it in ELisp where we make it register extra info about the
various indirect buffers, register a hook function on
`before/after-change-functions` which runs the hooks of the other
buffers sharing the same text, ...

Fixing the problem just for whitespace.el seems pointless because it's
just one particular spot in a wide area of brokenness: all features that
rely on `after/before-change-functions` are broken one way or another in
the presence of edits in indirect buffers.

That's clearly outside the scope of `emacs-29`, tho.


        Stefan