bug#62417: 30.0.50; Regression: 59ecf25fc860 is the first bad commit

bug#62417: 30.0.50; Regression: 59ecf25fc860 is the first bad commit

[João Távora]

This was originally reported in
https://github.com/joaotavora/sly/issues/534.  I had forgotten about it
high time I reported it, since it seems to be breaking the SLY Common
Lisp package in more places than I expected.

Before this commit:

   5ecf25fc86081c9df05b84194c36414c225c265 is the first bad commit
   commit 59ecf25fc86081c9df05b84194c36414c225c265
   Author: Philip Kaludercic <philipk@posteo.net>
   Date:   Thu Mar 10 10:59:52 2022 +0100
    
       * window.el (display-buffer-assq-regexp): Use buffer-match
    
    lisp/window.el | 15 ++++-----------
    1 file changed, 4 insertions(+), 11 deletions(-)

SLY, as can be downloaded from https://github.com/joaotavora/sly
operates normally in the following minimal invocation

   src/emacs -Q -nw -L ~/Source/Emacs/sly -l sly --eval '(setq tab-always-indent (quote complete))' -f sly
   ;; type (def, following by TAB

(This is the command I used for bisecting the error.)

After this commit, an error comes up about

   (wrong-type-argument stringp #<buffer *sly-completions*>)

I haven't yet investigated the reason.  There are other use cases inside
SLY that are also failing with similar errors, but they are not as easy
to trigger.

SLY in it its current state has worked for every version of Emacs 25
onwards, probably also 24. I just tested this case with Emacs 26 and it
works fine.

João

bug#62417: 30.0.50; Regression: 59ecf25fc860 is the first bad commit

[João Távora]

tag 62417 patch

João Távora <joaotavora@gmail.com> writes:

> I haven't yet investigated the reason.  There are other use cases inside
> SLY that are also failing with similar errors, but they are not as easy
> to trigger.

The simples way to fix this is to make display-buffer-assq-regexp keep
the old protocol before trying `buffer-match-p'.

diff --git a/lisp/window.el b/lisp/window.el
index 2da2f8bb2c8..0932a05aabf 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -7502,8 +7502,13 @@ display-buffer-assq-regexp
 the form of the action argument passed to `display-buffer'."
   (catch 'match
     (dolist (entry alist)
-      (when (buffer-match-p (car entry) buffer-name action)
-        (throw 'match (cdr entry))))))
+      (let ((key (car entry)))
+        (when (or (and (stringp key)
+                       (string-match-p key buffer-name))
+                  (and (functionp key)
+                       (funcall key buffer-name action))
+                  (buffer-match-p (car entry) buffer-name action))
+          (throw 'match (cdr entry)))))))
 
 (defvar display-buffer--same-window-action
   '(display-buffer-same-window

Another way would be to fix this in buffer-match-p.

João

bug#62417: 30.0.50; Regression: 59ecf25fc860 is the first bad commit

[Philip Kaludercic]

João Távora <joaotavora@gmail.com> writes:

> tag 62417 patch
>
> João Távora <joaotavora@gmail.com> writes:
>
>> I haven't yet investigated the reason.  There are other use cases inside
>> SLY that are also failing with similar errors, but they are not as easy
>> to trigger.
>
> The simples way to fix this is to make display-buffer-assq-regexp keep
> the old protocol before trying `buffer-match-p'.
>
> diff --git a/lisp/window.el b/lisp/window.el
> index 2da2f8bb2c8..0932a05aabf 100644
> --- a/lisp/window.el
> +++ b/lisp/window.el
> @@ -7502,8 +7502,13 @@ display-buffer-assq-regexp
>  the form of the action argument passed to `display-buffer'."
>    (catch 'match
>      (dolist (entry alist)
> -      (when (buffer-match-p (car entry) buffer-name action)
> -        (throw 'match (cdr entry))))))
> +      (let ((key (car entry)))
> +        (when (or (and (stringp key)
> +                       (string-match-p key buffer-name))
> +                  (and (functionp key)
> +                       (funcall key buffer-name action))
> +                  (buffer-match-p (car entry) buffer-name action))
> +          (throw 'match (cdr entry)))))))
>  
>  (defvar display-buffer--same-window-action
>    '(display-buffer-same-window
>
> Another way would be to fix this in buffer-match-p.

I cannot make out what is broken in `buffer-match-p'?  The patch would
appear to me to be redundant, because both strings and functions are
handled the same way in that function.  If you could explain the
background, I think it would be better to fix `buffer-match-p',
considering that this should be how it behaves.

> João

-- 
Philip Kaludercic

bug#62417: 30.0.50; Regression: 59ecf25fc860 is the first bad commit

[João Távora]

On Fri, Mar 24, 2023 at 4:05 PM Philip Kaludercic <philipk@posteo.net> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> > tag 62417 patch
> >
> > João Távora <joaotavora@gmail.com> writes:
> >
> >> I haven't yet investigated the reason.  There are other use cases inside
> >> SLY that are also failing with similar errors, but they are not as easy
> >> to trigger.
> >
> > The simples way to fix this is to make display-buffer-assq-regexp keep
> > the old protocol before trying `buffer-match-p'.
> >
> > diff --git a/lisp/window.el b/lisp/window.el
> > index 2da2f8bb2c8..0932a05aabf 100644
> > --- a/lisp/window.el
> > +++ b/lisp/window.el
> > @@ -7502,8 +7502,13 @@ display-buffer-assq-regexp
> >  the form of the action argument passed to `display-buffer'."
> >    (catch 'match
> >      (dolist (entry alist)
> > -      (when (buffer-match-p (car entry) buffer-name action)
> > -        (throw 'match (cdr entry))))))
> > +      (let ((key (car entry)))
> > +        (when (or (and (stringp key)
> > +                       (string-match-p key buffer-name))
> > +                  (and (functionp key)
> > +                       (funcall key buffer-name action))
> > +                  (buffer-match-p (car entry) buffer-name action))
> > +          (throw 'match (cdr entry)))))))
> >
> >  (defvar display-buffer--same-window-action
> >    '(display-buffer-same-window
> >
> > Another way would be to fix this in buffer-match-p.
>
> I cannot make out what is broken in `buffer-match-p'?  The patch would
> appear to me to be redundant, because both strings and functions are
> handled the same way in that function.  If you could explain the
> background, I think it would be better to fix `buffer-match-p',
> considering that this should be how it behaves.

If you pass a string to buffer-match-p, it will become
a buffer by the time it is passed to the function.  So
functions that expect strings cannot be in
buffer-display-alist after your change, whereas before
they could.

João

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[João Távora]

João Távora <joaotavora@gmail.com> writes:

>> > Another way would be to fix this in buffer-match-p.
>> I cannot make out what is broken in `buffer-match-p'?  The patch would
>> appear to me to be redundant, because both strings and functions are
>> handled the same way in that function.  If you could explain the
>> background, I think it would be better to fix `buffer-match-p',
>> considering that this should be how it behaves.
> If you pass a string to buffer-match-p, it will become
> a buffer by the time it is passed to the function.  So
> functions that expect strings cannot be in
> buffer-display-alist after your change, whereas before
> they could.

If the previous explanation is somehow hard to understand, here's a
hopefully simpler one with a repro which doesn't require SLY.  In Emacs
28 the docstring for `display-buffer-alist` states (emphasis mine):

   If non-nil, this is an alist of elements (CONDITION . ACTION),
   where:
    
    CONDITION is either a regexp matching buffer names, or a
     function that takes two arguments - a buffer name and the
     ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
     ACTION argument of `display-buffer' - and returns a boolean.

In Emacs 29, the docstring was changed to state:

    If non-nil, this is an alist of elements (CONDITION . ACTION),
    where:
     
     CONDITION is passed to `buffer-match-p', along with the buffer
      that is to be displayed and the ACTION argument of
      `display-buffer', to check if ACTION should be used.

Any code that was written for the Emacs 28 contract in mind like, for
example:

   (defun foop (buffer-name _alist) (string-match "foop" buffer-name))

   (add-to-list 'display-buffer-alist '(foop . display-buffer-other-frame))

Will now fail with an obscure error message.  I've checked "Incompatible
Lisp Changes in Emacs 29.1" in etc/NEWS and could not find a mention to
this, so I assume it was not intentional.

So it is most clearly a regression.

We should plug it in Emacs 29 as quickly as possible.  

I showed one way to go about it, but maybe other ways are possible, like
extending the `buffer-match-p' mini-language to allow for this case --
not sure that is feasible since it could require asking its CONDITION
function if it accepts buffers or buffer names.  Or requiring that all
CONDITION functions added in the wild now also support buffer names.

Whichever way we go, the Emacs 28 contract of `display-buffer-alist`
must be upheld before Emacs 29 is released.

João

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[Eli Zaretskii]

> Cc: 62417@debbugs.gnu.org
> From: João Távora <joaotavora@gmail.com>
> Date: Fri, 24 Mar 2023 19:48:35 +0000
> 
> If the previous explanation is somehow hard to understand, here's a
> hopefully simpler one with a repro which doesn't require SLY.  In Emacs
> 28 the docstring for `display-buffer-alist` states (emphasis mine):
> 
>    If non-nil, this is an alist of elements (CONDITION . ACTION),
>    where:
>     
>     CONDITION is either a regexp matching buffer names, or a
>      function that takes two arguments - a buffer name and the
>      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>      ACTION argument of `display-buffer' - and returns a boolean.
> 
> In Emacs 29, the docstring was changed to state:
> 
>     If non-nil, this is an alist of elements (CONDITION . ACTION),
>     where:
>      
>      CONDITION is passed to `buffer-match-p', along with the buffer
>       that is to be displayed and the ACTION argument of
>       `display-buffer', to check if ACTION should be used.
> 
> Any code that was written for the Emacs 28 contract in mind like, for
> example:
> 
>    (defun foop (buffer-name _alist) (string-match "foop" buffer-name))
> 
>    (add-to-list 'display-buffer-alist '(foop . display-buffer-other-frame))
> 
> Will now fail with an obscure error message.  I've checked "Incompatible
> Lisp Changes in Emacs 29.1" in etc/NEWS and could not find a mention to
> this, so I assume it was not intentional.
> 
> So it is most clearly a regression.

There's something missing in the above description, since
buffer-match-p accepts a function as its CONDITION argument, and calls
that function with the buffer and ACTION.  So it sounds like code
written for Emacs 28 should still work.  What is missing here that
explains the breakage?

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

On Sat, Mar 25, 2023, 12:55 Eli Zaretskii <eliz@gnu.org> wrote:.

> >
> > So it is most clearly a regression.
>
> There's something missing in the above description, since
> buffer-match-p accepts a function as its CONDITION argument, and calls
> that function with the buffer and ACTION.  So it sounds like code
> written for Emacs 28 should still work.  What is missing here that
> explains the breakage?
>

As I highlighted, Emacs used to call such functions with a buffer _name_
and an action. Now it calls them with a buffer _object_ and an action. This
is a backward-incompatible change.

João

[-- Attachment #2: Type: text/html, Size: 1032 bytes --]

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Sat, 25 Mar 2023 13:04:24 +0000
> Cc: "Philip K." <philipk@posteo.net>, 62417@debbugs.gnu.org
> 
> On Sat, Mar 25, 2023, 12:55 Eli Zaretskii <eliz@gnu.org> wrote:.
> 
>  > 
>  > So it is most clearly a regression.
> 
>  There's something missing in the above description, since
>  buffer-match-p accepts a function as its CONDITION argument, and calls
>  that function with the buffer and ACTION.  So it sounds like code
>  written for Emacs 28 should still work.  What is missing here that
>  explains the breakage?
> 
> As I highlighted, Emacs used to call such functions with a buffer _name_ and an action. Now it calls them
> with a buffer _object_ and an action.

No, buffer-match-p accepts a buffer object _or_ a buffer name as its
first argument.

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 1201 bytes --]

On Sat, Mar 25, 2023, 13:20 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: João Távora <joaotavora@gmail.com>
> > Date: Sat, 25 Mar 2023 13:04:24 +0000
> > Cc: "Philip K." <philipk@posteo.net>, 62417@debbugs.gnu.org
> >
> > On Sat, Mar 25, 2023, 12:55 Eli Zaretskii <eliz@gnu.org> wrote:.
> >
> >  >
> >  > So it is most clearly a regression.
> >
> >  There's something missing in the above description, since
> >  buffer-match-p accepts a function as its CONDITION argument, and calls
> >  that function with the buffer and ACTION.  So it sounds like code
> >  written for Emacs 28 should still work.  What is missing here that
> >  explains the breakage?
> >
> > As I highlighted, Emacs used to call such functions with a buffer _name_
> and an action. Now it calls them
> > with a buffer _object_ and an action.
>
> No, buffer-match-p accepts a buffer object _or_ a buffer name as its
> first argument.
>

Second argument.

>
But you're confused: this is not about buffer-match-p's arguments. It's
about the arguments to the function that you may also pass to
buffer-match-p in it's first CONDITION argument. Use my simple recipe in
both Emacs 28 and 29.

João

[-- Attachment #2: Type: text/html, Size: 2421 bytes --]

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Sat, 25 Mar 2023 13:56:40 +0000
> Cc: "Philip K." <philipk@posteo.net>, 62417@debbugs.gnu.org
> 
> But you're confused: this is not about buffer-match-p's arguments. It's about the arguments to the function
> that you may also pass to buffer-match-p in it's first CONDITION argument. Use my simple recipe in both
> Emacs 28 and 29.

The function should be called with the same BUFFER-OR-NAME argument
with which buffer-match-p was called.

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[João Távora]

[-- Attachment #1: Type: text/plain, Size: 728 bytes --]

On Sat, Mar 25, 2023, 14:13 Eli Zaretskii <eliz@gnu.org> wrote:

> > From: João Távora <joaotavora@gmail.com>
> > Date: Sat, 25 Mar 2023 13:56:40 +0000
> > Cc: "Philip K." <philipk@posteo.net>, 62417@debbugs.gnu.org
> >
> > But you're confused: this is not about buffer-match-p's arguments. It's
> about the arguments to the function
> > that you may also pass to buffer-match-p in it's first CONDITION
> argument. Use my simple recipe in both
> > Emacs 28 and 29.
>
> The function should be called with the same BUFFER-OR-NAME argument
> with which buffer-match-p was called.
>

Perhaps, that's a possibility among others. But that's not what happens
today. It always converts it to a buffer.

João

>

[-- Attachment #2: Type: text/html, Size: 1545 bytes --]

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Sat, 25 Mar 2023 13:56:40 +0000
>> Cc: "Philip K." <philipk@posteo.net>, 62417@debbugs.gnu.org
>> 
>> But you're confused: this is not about buffer-match-p's arguments. It's about the arguments to the function
>> that you may also pass to buffer-match-p in it's first CONDITION argument. Use my simple recipe in both
>> Emacs 28 and 29.
>
> The function should be called with the same BUFFER-OR-NAME argument
> with which buffer-match-p was called.

Here's your idea in a patch.  It fixes the issue.  Propose this be
pushed to emacs-29.

João

diff --git a/lisp/subr.el b/lisp/subr.el
index 99ddd813867..39866dd7acb 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -7114,7 +7114,7 @@ buffer-match-p
 - the symbol t, to always match,
 - the symbol nil, which never matches,
 - a regular expression, to match a buffer name,
-- a predicate function that takes a buffer object and ARG as
+- a predicate function that takes BUFFER-OR-NAME and ARG as
   arguments, and returns non-nil if the buffer matches,
 - a cons-cell, where the car describes how to interpret the cdr.
   The car can be one of the following:
@@ -7140,8 +7140,8 @@ buffer-match-p
                        (string-match-p condition (buffer-name buffer)))
                       ((pred functionp)
                        (if (eq 1 (cdr (func-arity condition)))
-                           (funcall condition buffer)
-                         (funcall condition buffer arg)))
+                           (funcall condition buffer-or-name)
+                         (funcall condition buffer-or-name arg)))
                       (`(major-mode . ,mode)
                        (eq
                         (buffer-local-value 'major-mode buffer)
diff --git a/lisp/window.el b/lisp/window.el
index 08ce8498655..e8daa0383ec 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -7510,8 +7510,8 @@ display-buffer-alist
 If non-nil, this is an alist of elements (CONDITION . ACTION),
 where:
 
- CONDITION is passed to `buffer-match-p', along with the buffer
-  that is to be displayed and the ACTION argument of
+ CONDITION is passed to `buffer-match-p', along with the name of
+  the buffer that is to be displayed and the ACTION argument of
   `display-buffer', to check if ACTION should be used.
 
  ACTION is a cons cell (FUNCTIONS . ALIST), where FUNCTIONS is an

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[Philip Kaludercic]

João Távora <joaotavora@gmail.com> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>>> From: João Távora <joaotavora@gmail.com>
>>> Date: Sat, 25 Mar 2023 13:56:40 +0000
>>> Cc: "Philip K." <philipk@posteo.net>, 62417@debbugs.gnu.org
>>> 
>>> But you're confused: this is not about buffer-match-p's arguments. It's about the arguments to the function
>>> that you may also pass to buffer-match-p in it's first CONDITION argument. Use my simple recipe in both
>>> Emacs 28 and 29.
>>
>> The function should be called with the same BUFFER-OR-NAME argument
>> with which buffer-match-p was called.
>
> Here's your idea in a patch.  It fixes the issue.  Propose this be
> pushed to emacs-29.
>
> João
>
> diff --git a/lisp/subr.el b/lisp/subr.el
> index 99ddd813867..39866dd7acb 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -7114,7 +7114,7 @@ buffer-match-p
>  - the symbol t, to always match,
>  - the symbol nil, which never matches,
>  - a regular expression, to match a buffer name,
> -- a predicate function that takes a buffer object and ARG as
>
> +- a predicate function that takes BUFFER-OR-NAME and ARG as
>    arguments, and returns non-nil if the buffer matches,
>  - a cons-cell, where the car describes how to interpret the cdr.
>    The car can be one of the following:
> @@ -7140,8 +7140,8 @@ buffer-match-p
>                         (string-match-p condition (buffer-name buffer)))
>                        ((pred functionp)
>                         (if (eq 1 (cdr (func-arity condition)))
> -                           (funcall condition buffer)
> -                         (funcall condition buffer arg)))
> +                           (funcall condition buffer-or-name)
> +                         (funcall condition buffer-or-name arg)))
>                        (`(major-mode . ,mode)
>                         (eq
>                          (buffer-local-value 'major-mode buffer)
> diff --git a/lisp/window.el b/lisp/window.el
> index 08ce8498655..e8daa0383ec 100644
> --- a/lisp/window.el
> +++ b/lisp/window.el
> @@ -7510,8 +7510,8 @@ display-buffer-alist
>  If non-nil, this is an alist of elements (CONDITION . ACTION),
>  where:
>  
> - CONDITION is passed to `buffer-match-p', along with the buffer
> -  that is to be displayed and the ACTION argument of
> + CONDITION is passed to `buffer-match-p', along with the name of
> +  the buffer that is to be displayed and the ACTION argument of
>    `display-buffer', to check if ACTION should be used.
>  
>   ACTION is a cons cell (FUNCTIONS . ALIST), where FUNCTIONS is an

LGTM

-- 
Philip Kaludercic

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: philipk@posteo.net,  62417@debbugs.gnu.org
> Date: Sun, 26 Mar 2023 21:22:14 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > The function should be called with the same BUFFER-OR-NAME argument
> > with which buffer-match-p was called.
> 
> Here's your idea in a patch.  It fixes the issue.  Propose this be
> pushed to emacs-29.

Please install on emacs-29, and thanks.

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Cc: philipk@posteo.net,  62417@debbugs.gnu.org
>> Date: Sun, 26 Mar 2023 21:22:14 +0100
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > The function should be called with the same BUFFER-OR-NAME argument
>> > with which buffer-match-p was called.
>> 
>> Here's your idea in a patch.  It fixes the issue.  Propose this be
>> pushed to emacs-29.
>
> Please install on emacs-29, and thanks.

Done. I took the liberty of pushing this additional change to the patch
I showed, which is essential to make this work.  I had forgotten to show
it.  Unless there are objections, we can close this bug.

 (defun display-buffer-assq-regexp (buffer-or-name alist action)
   "Retrieve ALIST entry corresponding to buffer specified by BUFFER-OR-NAME.
 This returns the cdr of the alist entry ALIST if the entry's
-key (its car) and BUFFER-OR-NAME satisfy `buffer-match-p', using
-the key as CONDITION argument of `buffer-match-p'.  ACTION should
-have the form of the action argument passed to `display-buffer'."
+key (its car) and the name of the buffer designated by
+BUFFER-OR-NAME satisfy `buffer-match-p', using the key as
+CONDITION argument of `buffer-match-p'.  ACTION should have the
+form of the action argument passed to `display-buffer'."
   (catch 'match
     (dolist (entry alist)
-      (when (buffer-match-p (car entry) buffer-or-name action)
+      (when (buffer-match-p (car entry) (if (stringp buffer-or-name)
+                                            buffer-or-name
+                                          (buffer-name buffer-or-name))
+                                          action)
         (throw 'match (cdr entry))))))

João

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Cc: philipk@posteo.net,  62417@debbugs.gnu.org
> Date: Mon, 27 Mar 2023 13:06:06 +0100
> 
> > Please install on emacs-29, and thanks.
> 
> Done. I took the liberty of pushing this additional change to the patch
> I showed, which is essential to make this work.  I had forgotten to show
> it.  Unless there are objections, we can close this bug.

I don't understand why is it essential, since buffer-match-p accepts
both buffers and their names.  Please explain.  (And I wish you
explained this before pushing, since there's no special rush anyway.)

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[João Távora]

On Mon, Mar 27, 2023 at 2:49 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > From: João Távora <joaotavora@gmail.com>
> > Cc: philipk@posteo.net,  62417@debbugs.gnu.org
> > Date: Mon, 27 Mar 2023 13:06:06 +0100
> >
> > > Please install on emacs-29, and thanks.
> >
> > Done. I took the liberty of pushing this additional change to the patch
> > I showed, which is essential to make this work.  I had forgotten to show
> > it.  Unless there are objections, we can close this bug.
>
> I don't understand why is it essential,

It's very easy to reproduce this problem.  Just see the code snippet
I included as part of the commit.

Author: João Távora <joaotavora@gmail.com>
Date:   Mon Mar 27 12:25:16 2023 +0100

    Fix accidental backward-incompatible change (bug#62417)

    This code used to work, but with the change of 59ecf25fc860 it stopped
    working:

       (defun foop (buffer-name _alist) (string-match "foop" buffer-name))
       (add-to-list 'display-buffer-alist '(foop . display-buffer-other-frame))

    This change makes it work again, restoring compatibility.

    * lisp/subr.el (buffer-match-p): Fix and adjust docstring.
    * lisp/window.el (display-buffer-alist): Adjust docstring.
    (display-buffer-assq-regexp): Make good on promise of display-buffer-alist.


If you remove the extra part and try that snippet in both emacs-28
and emacs-29, you'll reach the same conclusion as I

> since buffer-match-p accepts
> both buffers and their names.  Please explain.

In the patch I showed, which you and Philip approved, the docstring of
the variable display-buffer-alist was clarified to state that it is a buffer
name string, and _not_ a buffer object, that is passed to buffer-match-p.
This is absolutely necessary, and we've already been through this.

But naturally it's not enough to simply state that fact in a docstring.
You have to actually make good on the promise by actually passing a
buffer name to buffer-match-p, and not a buffer.  Otherwise, the user
functions that the user places in display-buffer-alist WILL be called with a
buffer _object_ always.  And for people programming against Emacs < 29,
those functions are always passed a buffer name _string_.

Do you understand?

I think there is still confusion.  It's understandable, as this new
buffer-match-p protocol makes what was previously a relatively simple
protocol is much harder to understand, because there's an added level
of indirection.  Presumably added in the name of flexibility, but that
flexibility actually already existed in Emacs 28, the buffer-match-p
mini-language just adds so-called syntactic sugar.

As I said: there are other perfectly plausible ways to address this
problem, including removing buffer-match-p from display-buffer-alist
logic and losing this particular sugar.

> (And I wish you
> explained this before pushing, since there's no special rush anyway.)

There are people with broken SLYs in the Emacs 29 builds and master
for a long time.  See the original link. I wish I didn't let it get
this far, that was my bad, but this is hurting users today.

João

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Mon, 27 Mar 2023 14:08:17 +0000
> Cc: philipk@posteo.net, 62417@debbugs.gnu.org
> 
> > since buffer-match-p accepts
> > both buffers and their names.  Please explain.
> 
> In the patch I showed, which you and Philip approved, the docstring of
> the variable display-buffer-alist was clarified to state that it is a buffer
> name string, and _not_ a buffer object, that is passed to buffer-match-p.
> This is absolutely necessary, and we've already been through this.

I don't understand why this is necessary, and I didn't intend to limit
buffer-match-p to accepting only buffer names.  Please explain why is
it necessary.

What I did say was that _if_ buffer-match-p will be able to accept
_both_ buffer names and objects _and_ will pass to the function
exactly the argument it was passed, i.e. either a buffer object or a
name of a buffer, _then_ the backward-incompatibility will be solved.

The responsibility of making sure buffer-match-p accepts a name when
the function expects only names is _on_the_caller_.  And the caller is
NOT display-buffer, it's the Lisp code which calls display-buffer or
which prepares the alist that will be passed to display-buffer.

> But naturally it's not enough to simply state that fact in a docstring.
> You have to actually make good on the promise by actually passing a
> buffer name to buffer-match-p, and not a buffer.  Otherwise, the user
> functions that the user places in display-buffer-alist WILL be called with a
> buffer _object_ always.  And for people programming against Emacs < 29,
> those functions are always passed a buffer name _string_.

Yes, but why in display-buffer-assq-regexp?  That function is not
supposed to have any knowledge about functions in
display-buffer-alist.  With the change you made you basically preclude
display-buffer-alist from having functions that want to accept buffer
objects.  That is not right.

So why cannot the code which prepares the alist make sure the function
accepts only buffer names, not buffer objects?

> I think there is still confusion.  It's understandable, as this new
> buffer-match-p protocol makes what was previously a relatively simple
> protocol is much harder to understand, because there's an added level
> of indirection.  Presumably added in the name of flexibility, but that
> flexibility actually already existed in Emacs 28, the buffer-match-p
> mini-language just adds so-called syntactic sugar.
> 
> As I said: there are other perfectly plausible ways to address this
> problem, including removing buffer-match-p from display-buffer-alist
> logic and losing this particular sugar.

Please don't fight "rearguard fights".  We are not going back on this
change which introduced buffer-match-p.  So suggesting that is a
non-starter.

> > (And I wish you
> > explained this before pushing, since there's no special rush anyway.)
> 
> There are people with broken SLYs in the Emacs 29 builds and master
> for a long time.  See the original link. I wish I didn't let it get
> this far, that was my bad, but this is hurting users today.

The users can easily make local changes.  That is not a reason to rush
changes which were not agreed upon, and leave some of us with bad
taste.

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[Eli Zaretskii]

> Cc: philipk@posteo.net, 62417@debbugs.gnu.org
> Date: Mon, 27 Mar 2023 18:20:48 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> > From: João Távora <joaotavora@gmail.com>
> > Date: Mon, 27 Mar 2023 14:08:17 +0000
> > Cc: philipk@posteo.net, 62417@debbugs.gnu.org
> > 
> > > since buffer-match-p accepts
> > > both buffers and their names.  Please explain.
> > 
> > In the patch I showed, which you and Philip approved, the docstring of
> > the variable display-buffer-alist was clarified to state that it is a buffer
> > name string, and _not_ a buffer object, that is passed to buffer-match-p.
> > This is absolutely necessary, and we've already been through this.
> 
> I don't understand why this is necessary, and I didn't intend to limit
> buffer-match-p to accepting only buffer names.  Please explain why is
> it necessary.
> 
> What I did say was that _if_ buffer-match-p will be able to accept
> _both_ buffer names and objects _and_ will pass to the function
> exactly the argument it was passed, i.e. either a buffer object or a
> name of a buffer, _then_ the backward-incompatibility will be solved.
> 
> The responsibility of making sure buffer-match-p accepts a name when
> the function expects only names is _on_the_caller_.  And the caller is
> NOT display-buffer, it's the Lisp code which calls display-buffer or
> which prepares the alist that will be passed to display-buffer.

To make a long story short: here's how the call to
display-buffer-assq-regexp looked like in Emacs 28:

      (let* ((user-action
	      (display-buffer-assq-regexp
	       (buffer-name buffer) display-buffer-alist action))

And here's how it looks like in Emacs 29:

    (let* ((user-action
            (display-buffer-assq-regexp
             buffer display-buffer-alist action))

Your change modified display-buffer-assq-regexp to pass to
buffer-match-p the name of the buffer if display-buffer-assq-regexp
was called with a buffer object.  This is not TRT, since, among other
issues, it changes the API of display-buffer-assq-regexp, which is a
public function.  Instead, we should do the following:

 . restore the calling convention of display-buffer-assq-regexp as it
   was in Emacs 28, where it accepted only buffer names, and fix its
   doc string accordingly
 . remove from display-buffer-assq-regexp the code which ensures
   buffer-match-p is called with the name of the buffer
 . make sure display-buffer call display-buffer-assq-regexp with a
   buffer's name, as it did in Emacs 28

OK?

(I can do this myself if you agree, and there are no other
objections.)

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[João Távora]

On Mon, Mar 27, 2023 at 5:33 PM Eli Zaretskii <eliz@gnu.org> wrote:
>
> > Cc: philipk@posteo.net, 62417@debbugs.gnu.org
> > Date: Mon, 27 Mar 2023 18:20:48 +0300
> > From: Eli Zaretskii <eliz@gnu.org>
> >
> > > From: João Távora <joaotavora@gmail.com>
> > > Date: Mon, 27 Mar 2023 14:08:17 +0000
> > > Cc: philipk@posteo.net, 62417@debbugs.gnu.org
> > >
> > > > since buffer-match-p accepts
> > > > both buffers and their names.  Please explain.
> > >
> > > In the patch I showed, which you and Philip approved, the docstring of
> > > the variable display-buffer-alist was clarified to state that it is a buffer
> > > name string, and _not_ a buffer object, that is passed to buffer-match-p.
> > > This is absolutely necessary, and we've already been through this.
> >
> > I don't understand why this is necessary, and I didn't intend to limit
> > buffer-match-p to accepting only buffer names.  Please explain why is
> > it necessary.
> >
> > What I did say was that _if_ buffer-match-p will be able to accept
> > _both_ buffer names and objects _and_ will pass to the function
> > exactly the argument it was passed, i.e. either a buffer object or a
> > name of a buffer, _then_ the backward-incompatibility will be solved.
> >
> > The responsibility of making sure buffer-match-p accepts a name when
> > the function expects only names is _on_the_caller_.  And the caller is
> > NOT display-buffer, it's the Lisp code which calls display-buffer or
> > which prepares the alist that will be passed to display-buffer.
>
> To make a long story short: here's how the call to
> display-buffer-assq-regexp looked like in Emacs 28:
>
>       (let* ((user-action
>               (display-buffer-assq-regexp
>                (buffer-name buffer) display-buffer-alist action))
>
> And here's how it looks like in Emacs 29:
>
>     (let* ((user-action
>             (display-buffer-assq-regexp
>              buffer display-buffer-alist action))
>
> Your change modified display-buffer-assq-regexp to pass to
> buffer-match-p the name of the buffer if display-buffer-assq-regexp
> was called with a buffer object.  This is not TRT, since, among other
> issues, it changes the API of display-buffer-assq-regexp, which is a
> public function.

Is it?  Not documented in the manual.  Looked a lot like bog-standard
generic data-accessing implementation detail to me, and I can't
see any advantage of using it directly.

But don't let that stop you from doing this change, which, if it
works (I've given a super simple recipe to check), is 120% percent
fine by me.

João

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[Eli Zaretskii]

> From: João Távora <joaotavora@gmail.com>
> Date: Mon, 27 Mar 2023 16:42:05 +0000
> Cc: philipk@posteo.net, 62417@debbugs.gnu.org
> 
> On Mon, Mar 27, 2023 at 5:33 PM Eli Zaretskii <eliz@gnu.org> wrote:
> >
> > To make a long story short: here's how the call to
> > display-buffer-assq-regexp looked like in Emacs 28:
> >
> >       (let* ((user-action
> >               (display-buffer-assq-regexp
> >                (buffer-name buffer) display-buffer-alist action))
> >
> > And here's how it looks like in Emacs 29:
> >
> >     (let* ((user-action
> >             (display-buffer-assq-regexp
> >              buffer display-buffer-alist action))
> >
> > Your change modified display-buffer-assq-regexp to pass to
> > buffer-match-p the name of the buffer if display-buffer-assq-regexp
> > was called with a buffer object.  This is not TRT, since, among other
> > issues, it changes the API of display-buffer-assq-regexp, which is a
> > public function.
> 
> Is it?  Not documented in the manual.  Looked a lot like bog-standard
> generic data-accessing implementation detail to me, and I can't
> see any advantage of using it directly.
> 
> But don't let that stop you from doing this change, which, if it
> works (I've given a super simple recipe to check), is 120% percent
> fine by me.

OK.  Philip, any objections to the following change:

diff --git a/lisp/window.el b/lisp/window.el
index 4bdc265..016d53f 100644
--- a/lisp/window.el
+++ b/lisp/window.el
@@ -7556,19 +7556,16 @@ display-buffer-fallback-action
 `display-buffer'.")
 (put 'display-buffer-fallback-action 'risky-local-variable t)
 
-(defun display-buffer-assq-regexp (buffer-or-name alist action)
-  "Retrieve ALIST entry corresponding to buffer specified by BUFFER-OR-NAME.
+(defun display-buffer-assq-regexp (buffer-name alist action)
+  "Retrieve ALIST entry corresponding to buffer whose name is BUFFER-NAME.
 This returns the cdr of the alist entry ALIST if the entry's
 key (its car) and the name of the buffer designated by
-BUFFER-OR-NAME satisfy `buffer-match-p', using the key as
+BUFFER-NAME satisfy `buffer-match-p', using the key as
 CONDITION argument of `buffer-match-p'.  ACTION should have the
 form of the action argument passed to `display-buffer'."
   (catch 'match
     (dolist (entry alist)
-      (when (buffer-match-p (car entry) (if (stringp buffer-or-name)
-                                            buffer-or-name
-                                          (buffer-name buffer-or-name))
-                                          action)
+      (when (buffer-match-p (car entry) buffer-name action)
         (throw 'match (cdr entry))))))
 
 (defvar display-buffer--same-window-action
@@ -7727,6 +7724,9 @@ display-buffer
   (let ((buffer (if (bufferp buffer-or-name)
 		    buffer-or-name
 		  (get-buffer buffer-or-name)))
+        (buf-name (if (bufferp buffer-or-name)
+                      (buffer-name buffer-or-name)
+                    buffer-or-name))
 	;; Make sure that when we split windows the old window keeps
 	;; point, bug#14829.
 	(split-window-keep-point t)
@@ -7735,7 +7735,7 @@ display-buffer
     (unless (listp action) (setq action nil))
     (let* ((user-action
             (display-buffer-assq-regexp
-             buffer display-buffer-alist action))
+             buf-name display-buffer-alist action))
            (special-action (display-buffer--special-action buffer))
            ;; Extra actions from the arguments to this function:
            (extra-action

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[Philip Kaludercic]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Mon, 27 Mar 2023 16:42:05 +0000
>> Cc: philipk@posteo.net, 62417@debbugs.gnu.org
>> 
>> On Mon, Mar 27, 2023 at 5:33 PM Eli Zaretskii <eliz@gnu.org> wrote:
>> >
>> > To make a long story short: here's how the call to
>> > display-buffer-assq-regexp looked like in Emacs 28:
>> >
>> >       (let* ((user-action
>> >               (display-buffer-assq-regexp
>> >                (buffer-name buffer) display-buffer-alist action))
>> >
>> > And here's how it looks like in Emacs 29:
>> >
>> >     (let* ((user-action
>> >             (display-buffer-assq-regexp
>> >              buffer display-buffer-alist action))
>> >
>> > Your change modified display-buffer-assq-regexp to pass to
>> > buffer-match-p the name of the buffer if display-buffer-assq-regexp
>> > was called with a buffer object.  This is not TRT, since, among other
>> > issues, it changes the API of display-buffer-assq-regexp, which is a
>> > public function.
>> 
>> Is it?  Not documented in the manual.  Looked a lot like bog-standard
>> generic data-accessing implementation detail to me, and I can't
>> see any advantage of using it directly.
>> 
>> But don't let that stop you from doing this change, which, if it
>> works (I've given a super simple recipe to check), is 120% percent
>> fine by me.
>
> OK.  Philip, any objections to the following change:

No objections from my side.

> diff --git a/lisp/window.el b/lisp/window.el
> index 4bdc265..016d53f 100644
> --- a/lisp/window.el
> +++ b/lisp/window.el
> @@ -7556,19 +7556,16 @@ display-buffer-fallback-action
>  `display-buffer'.")
>  (put 'display-buffer-fallback-action 'risky-local-variable t)
>  
> -(defun display-buffer-assq-regexp (buffer-or-name alist action)
> -  "Retrieve ALIST entry corresponding to buffer specified by BUFFER-OR-NAME.
> +(defun display-buffer-assq-regexp (buffer-name alist action)
> +  "Retrieve ALIST entry corresponding to buffer whose name is BUFFER-NAME.
>  This returns the cdr of the alist entry ALIST if the entry's
>  key (its car) and the name of the buffer designated by
> -BUFFER-OR-NAME satisfy `buffer-match-p', using the key as
> +BUFFER-NAME satisfy `buffer-match-p', using the key as
>  CONDITION argument of `buffer-match-p'.  ACTION should have the
>  form of the action argument passed to `display-buffer'."
>    (catch 'match
>      (dolist (entry alist)
> -      (when (buffer-match-p (car entry) (if (stringp buffer-or-name)
> -                                            buffer-or-name
> -                                          (buffer-name buffer-or-name))
> -                                          action)
> +      (when (buffer-match-p (car entry) buffer-name action)
>          (throw 'match (cdr entry))))))
>  
>  (defvar display-buffer--same-window-action
> @@ -7727,6 +7724,9 @@ display-buffer
>    (let ((buffer (if (bufferp buffer-or-name)
>  		    buffer-or-name
>  		  (get-buffer buffer-or-name)))
> +        (buf-name (if (bufferp buffer-or-name)
> +                      (buffer-name buffer-or-name)
> +                    buffer-or-name))
>  	;; Make sure that when we split windows the old window keeps
>  	;; point, bug#14829.
>  	(split-window-keep-point t)
> @@ -7735,7 +7735,7 @@ display-buffer
>      (unless (listp action) (setq action nil))
>      (let* ((user-action
>              (display-buffer-assq-regexp
> -             buffer display-buffer-alist action))
> +             buf-name display-buffer-alist action))
>             (special-action (display-buffer--special-action buffer))
>             ;; Extra actions from the arguments to this function:
>             (extra-action

-- 
Philip Kaludercic

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: João Távora <joaotavora@gmail.com>,
>   62417@debbugs.gnu.org
> Date: Mon, 27 Mar 2023 19:26:35 +0000
> 
> > OK.  Philip, any objections to the following change:
> 
> No objections from my side.

Thanks, installed on the emacs-29 branch, and closing the bug.

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[João Távora]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: João Távora <joaotavora@gmail.com>
>> Date: Mon, 27 Mar 2023 14:08:17 +0000
>> Cc: philipk@posteo.net, 62417@debbugs.gnu.org
>> 
>> > since buffer-match-p accepts
>> > both buffers and their names.  Please explain.
>> 
>> In the patch I showed, which you and Philip approved, the docstring of
>> the variable display-buffer-alist was clarified to state that it is a buffer
>> name string, and _not_ a buffer object, that is passed to buffer-match-p.
>> This is absolutely necessary, and we've already been through this.
>
> I don't understand why this is necessary, and I didn't intend to limit
> buffer-match-p to accepting only buffer names.

I'm _not_ doing that.  Buffer-match-p continues to accept buffer objects
or buffer names.  It's only that _when given a buffer name and a
function, it will call that function on the buffer name string_. 

> Please explain why is it necessary.

I've tried already 3 or 4 times.  I don't understand how better to
explain than to show you some perfectly code that worked on Emacs 28 and
doesn't work in Emacs 29 from yesterday.

If you want I will revert the patch, but _some_ solution should be
found.  Unforeseen, unmotivated, backward-incompatible lisp changes this
late in the game would be a sad thing.

> What I did say was that _if_ buffer-match-p will be able to accept
> _both_ buffer names and objects _and_ will pass to the function
> exactly the argument it was passed, i.e. either a buffer object or a
> name of a buffer, _then_ the backward-incompatibility will be solved.

And this is _exactly_ what happens.  But that, by itself is not enough,
becase display-buffer-assq-regexp was _also_ changed to always pass
buffer objects along with functions that expect buffer names.

>
> which prepares the alist that will be passed to display-buffer.
>
>> But naturally it's not enough to simply state that fact in a docstring.
>> You have to actually make good on the promise by actually passing a
>> buffer name to buffer-match-p, and not a buffer.  Otherwise, the user
>> functions that the user places in display-buffer-alist WILL be called with a
>> buffer _object_ always.  And for people programming against Emacs < 29,
>> those functions are always passed a buffer name _string_.
>
> Yes, but why in display-buffer-assq-regexp?  That function is not
> supposed to have any knowledge about functions in
> display-buffer-alist.  With the change you made you basically preclude
> display-buffer-alist from having functions that want to accept buffer
> objects.  That is not right.

Because that function was changed in order for buffer-match-p.  It was
_that_ change that broken compatiblity.

But it can be fixed anywhere else.

> So why cannot the code which prepares the alist make sure the function
> accepts only buffer names, not buffer objects?

The alist is open to the public, it is the user-facing interface.  The
"code that prepares the alist" is out in the wild and has worked fine
from Emacs 24 on, maybe even earlier.

>> I think there is still confusion.  It's understandable, as this new
>> buffer-match-p protocol makes what was previously a relatively simple
>> protocol is much harder to understand, because there's an added level
>> of indirection.  Presumably added in the name of flexibility, but that
>> flexibility actually already existed in Emacs 28, the buffer-match-p
>> mini-language just adds so-called syntactic sugar.
>> 
>> As I said: there are other perfectly plausible ways to address this
>> problem, including removing buffer-match-p from display-buffer-alist
>> logic and losing this particular sugar.
>
> Please don't fight "rearguard fights".  We are not going back on this
> change which introduced buffer-match-p.  So suggesting that is a
> non-starter.

Just a suggestion.  It is one of the many things that would fix this.

>> > (And I wish you
>> > explained this before pushing, since there's no special rush anyway.)
>> 
>> There are people with broken SLYs in the Emacs 29 builds and master
>> for a long time.  See the original link. I wish I didn't let it get
>> this far, that was my bad, but this is hurting users today.
>
> The users can easily make local changes.  That is not a reason to rush
> changes which were not agreed upon, and leave some of us with bad
> taste.

The bad tasting thing here was introducing the bug in the first place.
I'm just trying to solve it.  I've proposed two ways already.  If you
have better one, go ahead, I don't care, I really don't.  I just want
SLY users not to have to worry about this.

João

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[Philip Kaludercic]

[-- Attachment #1: Type: text/plain, Size: 1932 bytes --]

Eli Zaretskii <eliz@gnu.org> writes:

>> Cc: 62417@debbugs.gnu.org
>> From: João Távora <joaotavora@gmail.com>
>> Date: Fri, 24 Mar 2023 19:48:35 +0000
>> 
>> If the previous explanation is somehow hard to understand, here's a
>> hopefully simpler one with a repro which doesn't require SLY.  In Emacs
>> 28 the docstring for `display-buffer-alist` states (emphasis mine):
>> 
>>    If non-nil, this is an alist of elements (CONDITION . ACTION),
>>    where:
>>     
>>     CONDITION is either a regexp matching buffer names, or a
>>      function that takes two arguments - a buffer name and the
>>      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
>>      ACTION argument of `display-buffer' - and returns a boolean.
>> 
>> In Emacs 29, the docstring was changed to state:
>> 
>>     If non-nil, this is an alist of elements (CONDITION . ACTION),
>>     where:
>>      
>>      CONDITION is passed to `buffer-match-p', along with the buffer
>>       that is to be displayed and the ACTION argument of
>>       `display-buffer', to check if ACTION should be used.
>> 
>> Any code that was written for the Emacs 28 contract in mind like, for
>> example:
>> 
>>    (defun foop (buffer-name _alist) (string-match "foop" buffer-name))
>> 
>>    (add-to-list 'display-buffer-alist '(foop . display-buffer-other-frame))
>> 
>> Will now fail with an obscure error message.  I've checked "Incompatible
>> Lisp Changes in Emacs 29.1" in etc/NEWS and could not find a mention to
>> this, so I assume it was not intentional.
>> 
>> So it is most clearly a regression.
>
> There's something missing in the above description, since
> buffer-match-p accepts a function as its CONDITION argument, and calls
> that function with the buffer and ACTION.  

We would have to call the function with the buffer name instead of the
buffer object.  So the `buffer-match-p' fix would look like this:


[-- Attachment #2: Type: text/plain, Size: 727 bytes --]

diff --git a/lisp/subr.el b/lisp/subr.el
index 99ddd813867..3210ab05702 100644
--- a/lisp/subr.el
+++ b/lisp/subr.el
@@ -7140,8 +7140,8 @@ buffer-match-p
                        (string-match-p condition (buffer-name buffer)))
                       ((pred functionp)
                        (if (eq 1 (cdr (func-arity condition)))
-                           (funcall condition buffer)
-                         (funcall condition buffer arg)))
+                           (funcall condition (buffer-name buffer))
+                         (funcall condition (buffer-name buffer) arg)))
                       (`(major-mode . ,mode)
                        (eq
                         (buffer-local-value 'major-mode buffer)

[-- Attachment #3: Type: text/plain, Size: 464 bytes --]


I don't think I am a fan of this, as most of the time a buffer is more
immediately useful.  Perhaps João's initial change would be better in
that case, for the sake of backwards compatibility?  Or does it make
sense to mention this as an incompatible lisp change?

>                                            So it sounds like code
> written for Emacs 28 should still work.  What is missing here that
> explains the breakage?

-- 
Philip Kaludercic

bug#62417: ; Regression: 59ecf25fc860 is the first bad commit

[Eli Zaretskii]

> From: Philip Kaludercic <philipk@posteo.net>
> Cc: João Távora <joaotavora@gmail.com>,
>   62417@debbugs.gnu.org
> Date: Sat, 25 Mar 2023 13:17:40 +0000
> 
> We would have to call the function with the buffer name instead of the
> buffer object.  So the `buffer-match-p' fix would look like this:
> 
> diff --git a/lisp/subr.el b/lisp/subr.el
> index 99ddd813867..3210ab05702 100644
> --- a/lisp/subr.el
> +++ b/lisp/subr.el
> @@ -7140,8 +7140,8 @@ buffer-match-p
>                         (string-match-p condition (buffer-name buffer)))
>                        ((pred functionp)
>                         (if (eq 1 (cdr (func-arity condition)))
> -                           (funcall condition buffer)
> -                         (funcall condition buffer arg)))
> +                           (funcall condition (buffer-name buffer))
> +                         (funcall condition (buffer-name buffer) arg)))
>                        (`(major-mode . ,mode)
>                         (eq
>                          (buffer-local-value 'major-mode buffer)

No, I think we should pass to the function the original buffer-or-name
argument.  It makes no sense to me to have buffer-match-p second-guess
what a caller-defined function should get as its argument.

> I don't think I am a fan of this, as most of the time a buffer is more
> immediately useful.  Perhaps João's initial change would be better in
> that case, for the sake of backwards compatibility?  Or does it make
> sense to mention this as an incompatible lisp change?

The best solution is the one that completely removes the backward
incompatibility, and I think what I suggested does just that.