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

Previous Next

To reply to this bug, email your comments to 60333 AT debbugs.gnu.org.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Richard Hansen <rhansen <at> rhansen.org>
To: bug-gnu-emacs <at> gnu.org
Subject: [PATCH] whitespace: Update bob, eob markers in base and indirect
 buffers
Date: Mon, 26 Dec 2022 01:21:18 -0500

[Message part 1 (text/plain, inline)]

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).

[0001-whitespace-Update-bob-eob-markers-in-base-and-indire.patch (text/x-patch, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #8 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Ihor Radchenko <yantar92 <at> posteo.net>
To: Richard Hansen <rhansen <at> rhansen.org>
Cc: 60333 <at> debbugs.gnu.org
Subject: Re: bug#60333: [PATCH] whitespace: Update bob, eob markers in base
 and indirect buffers
Date: Tue, 27 Dec 2022 06:34:52 +0000

Richard Hansen <rhansen <at> 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>

Message #11 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Ihor Radchenko <yantar92 <at> posteo.net>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 60333 <at> debbugs.gnu.org, rhansen <at> rhansen.org
Subject: Re: bug#60333: [PATCH] whitespace: Update bob,
 eob markers in base and indirect buffers
Date: Tue, 27 Dec 2022 14:24:24 +0200

> Cc: 60333 <at> debbugs.gnu.org
> From: Ihor Radchenko <yantar92 <at> posteo.net>
> Date: Tue, 27 Dec 2022 06:34:52 +0000
> 
> Richard Hansen <rhansen <at> 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?

Message #14 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Ihor Radchenko <yantar92 <at> posteo.net>
Cc: 60333 <at> debbugs.gnu.org, Richard Hansen <rhansen <at> rhansen.org>
Subject: Re: bug#60333: [PATCH] whitespace: Update bob, eob markers in base
 and indirect buffers
Date: Tue, 27 Dec 2022 09:50:31 -0500

>> +(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

Message #17 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Ihor Radchenko <yantar92 <at> posteo.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 60333 <at> debbugs.gnu.org, Richard Hansen <rhansen <at> rhansen.org>
Subject: Re: bug#60333: [PATCH] whitespace: Update bob, eob markers in base
 and indirect buffers
Date: Wed, 28 Dec 2022 13:48:46 +0000

Stefan Monnier <monnier <at> 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>

Message #20 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Ihor Radchenko <yantar92 <at> posteo.net>
Cc: 60333 <at> debbugs.gnu.org, Richard Hansen <rhansen <at> rhansen.org>
Subject: Re: bug#60333: [PATCH] whitespace: Update bob, eob markers in base
 and indirect buffers
Date: Wed, 28 Dec 2022 09:44:22 -0500

>> "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

Message #23 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Richard Hansen <rhansen <at> rhansen.org>
Cc: 60333 <at> debbugs.gnu.org
Subject: Re: bug#60333: [PATCH] whitespace: Update bob, eob markers in base
 and indirect buffers
Date: Wed, 28 Dec 2022 09:51:55 -0500

>     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

Message #26 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Richard Hansen <rhansen <at> rhansen.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 60333 <at> debbugs.gnu.org
Subject: Re: bug#60333: [PATCH] whitespace: Update bob, eob markers in base
 and indirect buffers
Date: Thu, 29 Dec 2022 01:54:21 -0500

[Message part 1 (text/plain, inline)]

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.

[v2-0001-whitespace-Update-bob-eob-markers-in-base-and-ind.patch (text/x-patch, attachment)]

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #29 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Richard Hansen <rhansen <at> rhansen.org>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>,
 Ihor Radchenko <yantar92 <at> posteo.net>
Cc: 60333 <at> debbugs.gnu.org
Subject: Re: bug#60333: [PATCH] whitespace: Update bob, eob markers in base
 and indirect buffers
Date: Thu, 29 Dec 2022 03:11:11 -0500

[Message part 1 (text/plain, inline)]

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

[OpenPGP_signature (application/pgp-signature, attachment)]

Message #32 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Ihor Radchenko <yantar92 <at> posteo.net>
To: Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 60333 <at> debbugs.gnu.org, Richard Hansen <rhansen <at> rhansen.org>
Subject: Re: bug#60333: [PATCH] whitespace: Update bob, eob markers in base
 and indirect buffers
Date: Thu, 29 Dec 2022 11:20:00 +0000

Stefan Monnier <monnier <at> 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>

Message #35 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Ihor Radchenko <yantar92 <at> posteo.net>
Cc: 60333 <at> debbugs.gnu.org, Richard Hansen <rhansen <at> rhansen.org>
Subject: Re: bug#60333: [PATCH] whitespace: Update bob, eob markers in base
 and indirect buffers
Date: Thu, 29 Dec 2022 09:30:03 -0500

> 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

Message #38 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Ihor Radchenko <yantar92 <at> posteo.net>
Cc: 60333 <at> debbugs.gnu.org, Richard Hansen <rhansen <at> rhansen.org>
Subject: Re: bug#60333: [PATCH] whitespace: Update bob, eob markers in base
 and indirect buffers
Date: Thu, 29 Dec 2022 15:45:24 -0500

>> 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

Message #41 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Richard Hansen <rhansen <at> rhansen.org>
Cc: 60333 <at> debbugs.gnu.org, monnier <at> iro.umontreal.ca
Subject: Re: bug#60333: [PATCH] whitespace: Update bob,
 eob markers in base and indirect buffers
Date: Thu, 12 Jan 2023 11:32:21 +0200

> Cc: 60333 <at> debbugs.gnu.org
> Date: Thu, 29 Dec 2022 01:54:21 -0500
> From: Richard Hansen <rhansen <at> 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?

Message #44 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: Richard Hansen <rhansen <at> rhansen.org>,
 Stefan Monnier <monnier <at> iro.umontreal.ca>
Cc: 60333 <at> debbugs.gnu.org, yantar92 <at> posteo.net
Subject: Re: bug#60333: [PATCH] whitespace: Update bob,
 eob markers in base and indirect buffers
Date: Thu, 12 Jan 2023 11:33:15 +0200

> Cc: 60333 <at> debbugs.gnu.org
> Date: Thu, 29 Dec 2022 03:11:11 -0500
> From: Richard Hansen <rhansen <at> 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.

Message #47 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 60333 <at> debbugs.gnu.org, Richard Hansen <rhansen <at> rhansen.org>
Subject: Re: bug#60333: [PATCH] whitespace: Update bob, eob markers in base
 and indirect buffers
Date: Thu, 12 Jan 2023 11:15:00 -0500

>> 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

Message #50 received at 60333 <at> debbugs.gnu.org (full text, mbox):

From: Stefan Monnier <monnier <at> iro.umontreal.ca>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 60333 <at> debbugs.gnu.org, yantar92 <at> posteo.net,
 Richard Hansen <rhansen <at> rhansen.org>
Subject: Re: bug#60333: [PATCH] whitespace: Update bob, eob markers in base
 and indirect buffers
Date: Thu, 12 Jan 2023 11:21:00 -0500

>> 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

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.