Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Stefan Monnier]

>     Ignore buffers whose name begins with a space in save-some-buffers
>     * lisp/files.el (save-some-buffers): Consider these buffers
>       "internal", and don't prompt the user to save them.
>     * doc/lispref/files.texi: Document.

Why?  Currently we only prompt for buffers with a leading space
in 2 case:
- the buffer is associated with a file: rather unlikely, but if it
  occurs, it's probably a good idea not to ignore this buffer.
- the buffer has buffer-offer-save set to a non-nil value: here again if
  someone bothered to set this, it's a good idea not to ignore this buffer.


        Stefan

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eric Abrahamsen]

Stefan Monnier <monnier@IRO.UMontreal.CA> writes:

>>     Ignore buffers whose name begins with a space in save-some-buffers
>>     * lisp/files.el (save-some-buffers): Consider these buffers
>>       "internal", and don't prompt the user to save them.
>>     * doc/lispref/files.texi: Document.
>
> Why?  Currently we only prompt for buffers with a leading space
> in 2 case:
> - the buffer is associated with a file: rather unlikely, but if it
>   occurs, it's probably a good idea not to ignore this buffer.
> - the buffer has buffer-offer-save set to a non-nil value: here again if
>   someone bothered to set this, it's a good idea not to ignore this buffer.

This was fallout from 9b980e269, aka bug #28412, where now we prompt for
buffers that have functions in the `write-contents-functions' hook. So
that's one more case to add to the list, and this interfered with some
package behavior, and this seemed like the simplest solution.

See the bug report -- if there's a better way of handling, this, that
would be great.

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Kaushal Modi]

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

On Mon, Sep 18, 2017, 10:44 PM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> >     Ignore buffers whose name begins with a space in save-some-buffers
> >     * lisp/files.el (save-some-buffers): Consider these buffers
> >       "internal", and don't prompt the user to save them.
> >     * doc/lispref/files.texi: Document.
>
> Why?  Currently we only prompt for buffers with a leading space
> in 2 case:
> - the buffer is associated with a file: rather unlikely, but if it
>   occurs, it's probably a good idea not to ignore this buffer.
> - the buffer has buffer-offer-save set to a non-nil value: here again if
>   someone bothered to set this, it's a good idea not to ignore this buffer.


The previous commit that started promoting for file save if
write-file-functions was non-nil starting breaking my use case (ggtags.el +
verilog-mode).

Details here: https://github.com/leoliu/ggtags/issues/157

Summary:
- ggtags.el create an internal buffer with name starting with space "
*Code-Fontify*"
- verilog-mode is enabled in that buffer
- verilog-mode sets write-file-functions to a non-nil value
- So now I was promoted to save that " *Code-Fontify*" each time I quit
emacs.

ANDing the internal buffer check with OR of the 2 cases you mentioned will
work fine (won't undo the fix that Eric made in this commit).
-- 

Kaushal Modi

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

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Stefan Monnier]

>>>>> "Kaushal" == Kaushal Modi <kaushal.modi@gmail.com> writes:

> On Mon, Sep 18, 2017, 10:44 PM Stefan Monnier <monnier@iro.umontreal.ca>
> wrote:

>> >     Ignore buffers whose name begins with a space in save-some-buffers
>> >     * lisp/files.el (save-some-buffers): Consider these buffers
>> >       "internal", and don't prompt the user to save them.
>> >     * doc/lispref/files.texi: Document.
>> 
>> Why?  Currently we only prompt for buffers with a leading space
>> in 2 case:
>> - the buffer is associated with a file: rather unlikely, but if it
>> occurs, it's probably a good idea not to ignore this buffer.
>> - the buffer has buffer-offer-save set to a non-nil value: here again if
>> someone bothered to set this, it's a good idea not to ignore this buffer.


> The previous commit that started promoting for file save if
> write-file-functions was non-nil starting breaking my use case (ggtags.el +
> verilog-mode).

> Details here: https://github.com/leoliu/ggtags/issues/157

> Summary:
> - ggtags.el create an internal buffer with name starting with space "
> *Code-Fontify*"
> - verilog-mode is enabled in that buffer
> - verilog-mode sets write-file-functions to a non-nil value
> - So now I was promoted to save that " *Code-Fontify*" each time I quit
> emacs.

But, how does your example satisfy this part of the code:

                    (or
                     (buffer-file-name buffer)
                     (and pred
                          (progn
                            (set-buffer buffer)
                            (and buffer-offer-save (> (buffer-size) 0)))))

does " *Code-Fontify*" have buffer-file-name non-nil or does it have
buffer-offer-save non-nil?  If not, I can't see how you could get
prompted, but maybe I'm missing something.

> ANDing the internal buffer check with OR of the 2 cases you mentioned will
> work fine (won't undo the fix that Eric made in this commit).

But it will stop prompting for buffers with a leading space but where
buffer-offer-save is explicitly set to a non-nil value, which I think is
a bug.


        Stefan

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Kaushal Modi]

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

On Tue, Sep 19, 2017 at 8:08 AM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

>
> But, how does your example satisfy this part of the code:
>
>                     (or
>                      (buffer-file-name buffer)
>                      (and pred
>                           (progn
>                             (set-buffer buffer)
>                             (and buffer-offer-save (> (buffer-size) 0)))))
>

That is the old code pre-commit 9b980e2[1]. That worked fine.


> does " *Code-Fontify*" have buffer-file-name non-nil or does it have
> buffer-offer-save non-nil?


No, but write-contents-functions is non-nil in " *Code-Fontify*" buffer
(see below).


> If not, I can't see how you could get
> prompted, but maybe I'm missing something.


Post-commit 9b980e2[1], the newly-added 3rd segment to that OR starting
evalling true for my use case:

(or
 (buffer-file-name buffer)
 (and pred
      (progn
        (set-buffer buffer)
        (and buffer-offer-save (> (buffer-size) 0))))
 (buffer-local-value                                  ;THIS was the newly
added condition
  'write-contents-functions buffer))


> > ANDing the internal buffer check with OR of the 2 cases you mentioned
> will
> > work fine (won't undo the fix that Eric made in this commit).
>
> But it will stop prompting for buffers with a leading space but where
> buffer-offer-save is explicitly set to a non-nil value, which I think is
> a bug.
>

How about changing the location of that internal-buffer check?

i.e. change:

(buffer-local-value
  'write-contents-functions buffer)

to:

(and (buffer-local-value
      'write-contents-functions buffer)
     (not (eq (aref (buffer-name buffer) 0) ?\s)))

@Eric: That makes me wonder.. is it possible to not add this 3rd condition
in the or and instead just set buffer-offer-save in the special buffers in
your use-case?

[1]:
http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=9b980e2691afa3a7a967011fc004d352750fe618

-- 

Kaushal Modi

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

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Stefan Monnier]

> This was fallout from 9b980e269, aka bug #28412, where now we prompt for
> buffers that have functions in the `write-contents-functions' hook.

Ah, I see I was looking at the wrong version of the code, sorry.
So the question becomes: why does " *Code-Fontify*" have a non-nil
write-contents-functions?

Looking at ggtags.el, I also wonder why the buffer survives the call to
ggtags-fontify-code.  I would have expected ggtags-fontify-code to look
more like:

(cl-defun ggtags-fontify-code (code &optional (mode major-mode))
  (cl-check-type mode function)
  (cl-typecase code        ;; FIXME: Using `stringp` is more efficient.
    ((not string) code)
    (string
     (with-temp-buffer
       (insert code)
       ;; FIXME: `delay-mode-hooks' needs a comment
       (delay-mode-hooks (funcall mode))
       (font-lock-ensure)
       (buffer-string)))))


-- Stefan

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Kaushal Modi]

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

On Tue, Sep 19, 2017 at 9:00 AM Stefan Monnier <monnier@iro.umontreal.ca>
wrote:

> > This was fallout from 9b980e269, aka bug #28412, where now we prompt for
> > buffers that have functions in the `write-contents-functions' hook.
>
> Ah, I see I was looking at the wrong version of the code, sorry.
> So the question becomes: why does " *Code-Fontify*" have a non-nil
> write-contents-functions?
>

It's because ggtags.el enables the major mode from the actual file in this
temp " *Code-Fontify*" buffer by calling (funcall mode) in
ggtags-fontify-code.

And in my case, the verilog-mode is setting the write-contents-hooks (old
name of write-contents-functions)[1].


Looking at ggtags.el, I also wonder why the buffer survives the call to
> ggtags-fontify-code.  I would have expected ggtags-fontify-code to look
> more like:
>
> (cl-defun ggtags-fontify-code (code &optional (mode major-mode))
>   (cl-check-type mode function)
>   (cl-typecase code        ;; FIXME: Using `stringp` is more efficient.
>     ((not string) code)
>     (string
>      (with-temp-buffer
>        (insert code)
>        ;; FIXME: `delay-mode-hooks' needs a comment
>        (delay-mode-hooks (funcall mode))
>        (font-lock-ensure)
>        (buffer-string)))))
>

I will start this discussion on the ggtags.el repo. Thanks.

[1]:
https://github.com/veripool/verilog-mode/blob/ecdaea3386bee9d38520914e458d4860da644773/verilog-mode.el#L3969

-- 

Kaushal Modi

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

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Leo Liu]

On 2017-09-19 08:42 -0400, Stefan Monnier wrote:
> (cl-defun ggtags-fontify-code (code &optional (mode major-mode))
>   (cl-check-type mode function)
>   (cl-typecase code        ;; FIXME: Using `stringp` is more efficient.
>     ((not string) code)
>     (string
>      (with-temp-buffer
>        (insert code)
>        ;; FIXME: `delay-mode-hooks' needs a comment
>        (delay-mode-hooks (funcall mode))
>        (font-lock-ensure)
>        (buffer-string)))))

Thanks. delay-mode-hooks was removed a few months ago because it causes
errors for eldoc. Use font-lock-ensure is a good idea but will have to
wait until emacs 25+ is required.

Leo

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eric Abrahamsen]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> On Tue, Sep 19, 2017 at 9:00 AM Stefan Monnier <monnier@iro.umontreal.ca> wrote:
>
>  > This was fallout from 9b980e269, aka bug #28412, where now we prompt for
>  > buffers that have functions in the `write-contents-functions' hook.
>
>  Ah, I see I was looking at the wrong version of the code, sorry.
>  So the question becomes: why does " *Code-Fontify*" have a non-nil
>  write-contents-functions?
>
> It's because ggtags.el enables the major mode from the actual file in this temp " *Code-Fontify*" buffer by calling (funcall mode) in ggtags-fontify-code.

I do think ggtags is doing something weird here. But on the other hand,
it does seem better if `save-some-buffers' required `buffer-offer-save'
to be explicitly set non-nil, rather than drawing an implicit conclusion
from the presence of `write-contents-functions'.

Ie maybe I should just revert the change to this function, and make sure
to set `buffer-offer-save' to t in my use-case. No one will get
surprised by save prompts they didn't ask for. That way I could also
remove the buffer name check.

WDYT?

Eric

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Kaushal Modi]

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

On Tue, Sep 19, 2017 at 11:44 AM Eric Abrahamsen <eric@ericabrahamsen.net>
wrote:

>
> I do think ggtags is doing something weird here. But on the other hand,
> it does seem better if `save-some-buffers' required `buffer-offer-save'
> to be explicitly set non-nil, rather than drawing an implicit conclusion
> from the presence of `write-contents-functions'.
>
> Ie maybe I should just revert the change to this function, and make sure
> to set `buffer-offer-save' to t in my use-case. No one will get
> surprised by save prompts they didn't ask for. That way I could also
> remove the buffer name check.
>
> WDYT?
>

That's my proposal, of course if that resolves the issue you needed to make
that change in the first place! Thanks for considering this.
-- 

Kaushal Modi

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

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Stefan Monnier]

> Ie maybe I should just revert the change to this function, and make sure
> to set `buffer-offer-save' to t in my use-case.  No one will get
> surprised by save prompts they didn't ask for.  That way I could also
> remove the buffer name check.

If that works, then it sounds good.  You could add a note in the
docstring of write-contents-function about the use of buffer-offer-save
to make sure someone else doesn't follow the same longwinded path you did.


        Stefan

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Stefan Monnier]

> Thanks. delay-mode-hooks was removed a few months ago because it causes
> errors for eldoc. Use font-lock-ensure is a good idea but will have to
> wait until emacs 25+ is required.

You can

    (if (fboundp 'font-lock-ensure)
        (font-lock-ensure)
      ...oldcode...)

in the mean time,


        Stefan

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eric Abrahamsen]

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

>> Ie maybe I should just revert the change to this function, and make sure
>> to set `buffer-offer-save' to t in my use-case.  No one will get
>> surprised by save prompts they didn't ask for.  That way I could also
>> remove the buffer name check.
>
> If that works, then it sounds good.  You could add a note in the
> docstring of write-contents-function about the use of buffer-offer-save
> to make sure someone else doesn't follow the same longwinded path you did.

I'll kick the tires a bit (we've got tests!) and push a fix when it
seems ready.

Thanks,
Eric

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eric Abrahamsen]

Kaushal Modi <kaushal.modi@gmail.com> writes:

[...]

> (or
>  (buffer-file-name buffer)
>  (and pred
>       (progn
>         (set-buffer buffer)
>         (and buffer-offer-save (> (buffer-size) 0))))
>  (buffer-local-value                                  ;THIS was the newly added condition
>   'write-contents-functions buffer))

[...]

> @Eric: That makes me wonder.. is it possible to not add this 3rd condition in the or and instead just set buffer-offer-save in the special buffers in your use-case?

I've played with this a bit, and am getting hung up on the (and pred...
part above. "pred" is nil unless someone's customized
`save-some-buffers-default-predicate', so in the majority of cases the
check won't even reach `buffer-offer-save'. I don't really know why that
(and pred... is in there, and don't feel comfortable just yanking it
out.

Eric

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Kaushal Modi]

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

On Tue, Sep 19, 2017 at 2:14 PM Eric Abrahamsen <eric@ericabrahamsen.net>
wrote:

>
> I've played with this a bit, and am getting hung up on the (and pred...
> part above. "pred" is nil unless someone's customized
> `save-some-buffers-default-predicate', so in the majority of cases the
> check won't even reach `buffer-offer-save'.
>

Wow! That (and pred.. ) section has been there since the genesis of
files.el in 1991 by Roland McGrath:
http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b4da00e92a09a2ee2cfb5df2ec111636c66e1597

(Thanks to vc-region-history.)

It was named exiting.. and at some point got renamed to pred. So it looks
like, by design buffer-offer-save was checked only at the time of exiting
emacs i.e. during the call to save-some-buffers within
save-buffer-kill-emacs.

>  I don't really know why that (and pred... is in there, and don't feel
comfortable just yanking it out.

Yes, given that it had always been there.

The pred will be true automatically at the time of quitting emacs (aptly
because earlier that var was called 'exiting').. but not when you call C-x
s.

So looks like along with setting buffer-offer-save to t, you would also
need to set save-some-buffers-default-predicate to t locally for the
special buffers you need saving.

How about this as a solution? With this, you only need to set
buffer-over-save.

diff --git a/lisp/files.el b/lisp/files.el
index 0c30d40c13..5c05e3168b 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -5188,15 +5188,15 @@ save-some-buffers
        (and (buffer-live-p buffer)
     (buffer-modified-p buffer)
                     (not (buffer-base-buffer buffer))
-                    (not (eq (aref (buffer-name buffer) 0) ?\s))
                     (or
                      (buffer-file-name buffer)
                      (and pred
                           (progn
                             (set-buffer buffer)
                             (and buffer-offer-save (> (buffer-size) 0))))
-                     (buffer-local-value
-                      'write-contents-functions buffer))
+                     (and buffer-offer-save
+                          (buffer-local-value
+                           'write-contents-functions buffer)))
                     (or (not (functionp pred))
                         (with-current-buffer buffer (funcall pred)))
                     (if arg

Stefan? Eli?
-- 

Kaushal Modi

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

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Andreas Schwab]

On Sep 20 2017, Kaushal Modi <kaushal.modi@gmail.com> wrote:

> Wow! That (and pred.. ) section has been there since the genesis of
> files.el in 1991 by Roland McGrath:
> http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b4da00e92a09a2ee2cfb5df2ec111636c66e1597

It was added in 1986:

1986-12-12  Richard M. Stallman  (rms@prep)

        * files.el (buffer-offer-save): New variable, local in all buffers.
        * files.el (save-some-buffers): New 2nd arg EXITING.
        If non-nil, offer to save any nonempty modified buffer
        in which `buffer-offer-save' is non-nil.

Andreas.

-- 
Andreas Schwab, SUSE Labs, schwab@suse.de
GPG Key fingerprint = 0196 BAD8 1CE9 1970 F4BE  1748 E4D4 88E3 0EEA B9D7
"And now for something completely different."

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eric Abrahamsen]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> On Tue, Sep 19, 2017 at 2:14 PM Eric Abrahamsen <eric@ericabrahamsen.net> wrote:
>
>  I've played with this a bit, and am getting hung up on the (and pred...
>  part above. "pred" is nil unless someone's customized
>  `save-some-buffers-default-predicate', so in the majority of cases the
>  check won't even reach `buffer-offer-save'.
>
> Wow! That (and pred.. ) section has been there since the genesis of files.el in 1991 by Roland McGrath: http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=b4da00e92a09a2ee2cfb5df2ec111636c66e1597
>
> (Thanks to vc-region-history.)
>
> It was named exiting.. and at some point got renamed to pred. So it looks like, by design buffer-offer-save was checked only at the time of exiting emacs i.e. during the call to save-some-buffers within save-buffer-kill-emacs.

Thanks for doing the legwork! At least that gives us some confidence in
altering it.

[...]

> How about this as a solution? With this, you only need to set buffer-over-save.
>
> diff --git a/lisp/files.el b/lisp/files.el
> index 0c30d40c13..5c05e3168b 100644
> --- a/lisp/files.el
> +++ b/lisp/files.el
> @@ -5188,15 +5188,15 @@ save-some-buffers
>        (and (buffer-live-p buffer)
>     (buffer-modified-p buffer)
>                      (not (buffer-base-buffer buffer))
> -                    (not (eq (aref (buffer-name buffer) 0) ?\s))
>                      (or
>                       (buffer-file-name buffer)
>                       (and pred
>                            (progn
>                              (set-buffer buffer)
>                              (and buffer-offer-save (> (buffer-size) 0))))
> -                     (buffer-local-value
> -                      'write-contents-functions buffer))
> +                     (and buffer-offer-save
> +                          (buffer-local-value
> +                           'write-contents-functions buffer)))
>                      (or (not (functionp pred))
>                          (with-current-buffer buffer (funcall pred)))
>                      (if arg
>
> Stefan? Eli?

As far as I can see, that's a good solution. Let's see what they say.

Eric

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eli Zaretskii]

> From: Eric Abrahamsen <eric@ericabrahamsen.net>
> Date: Wed, 20 Sep 2017 10:14:12 -0700
> 
> > How about this as a solution? With this, you only need to set buffer-over-save.
> >
> > diff --git a/lisp/files.el b/lisp/files.el
> > index 0c30d40c13..5c05e3168b 100644
> > --- a/lisp/files.el
> > +++ b/lisp/files.el
> > @@ -5188,15 +5188,15 @@ save-some-buffers
> >        (and (buffer-live-p buffer)
> >     (buffer-modified-p buffer)
> >                      (not (buffer-base-buffer buffer))
> > -                    (not (eq (aref (buffer-name buffer) 0) ?\s))
> >                      (or
> >                       (buffer-file-name buffer)
> >                       (and pred
> >                            (progn
> >                              (set-buffer buffer)
> >                              (and buffer-offer-save (> (buffer-size) 0))))
> > -                     (buffer-local-value
> > -                      'write-contents-functions buffer))
> > +                     (and buffer-offer-save
> > +                          (buffer-local-value
> > +                           'write-contents-functions buffer)))
> >                      (or (not (functionp pred))
> >                          (with-current-buffer buffer (funcall pred)))
> >                      (if arg
> >
> > Stefan? Eli?
> 
> As far as I can see, that's a good solution. Let's see what they say.

Are there any upsides and downsides to consider wrt this solution?  If
so, what are they?

Thanks.

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eric Abrahamsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>> Date: Wed, 20 Sep 2017 10:14:12 -0700
>> 
>> > How about this as a solution? With this, you only need to set buffer-over-save.
>> >
>> > diff --git a/lisp/files.el b/lisp/files.el
>> > index 0c30d40c13..5c05e3168b 100644
>> > --- a/lisp/files.el
>> > +++ b/lisp/files.el
>> > @@ -5188,15 +5188,15 @@ save-some-buffers
>> >        (and (buffer-live-p buffer)
>> >     (buffer-modified-p buffer)
>> >                      (not (buffer-base-buffer buffer))
>> > -                    (not (eq (aref (buffer-name buffer) 0) ?\s))
>> >                      (or
>> >                       (buffer-file-name buffer)
>> >                       (and pred
>> >                            (progn
>> >                              (set-buffer buffer)
>> >                              (and buffer-offer-save (> (buffer-size) 0))))
>> > -                     (buffer-local-value
>> > -                      'write-contents-functions buffer))
>> > +                     (and buffer-offer-save
>> > +                          (buffer-local-value
>> > +                           'write-contents-functions buffer)))
>> >                      (or (not (functionp pred))
>> >                          (with-current-buffer buffer (funcall pred)))
>> >                      (if arg
>> >
>> > Stefan? Eli?
>> 
>> As far as I can see, that's a good solution. Let's see what they say.
>
> Are there any upsides and downsides to consider wrt this solution?  If
> so, what are they?

It isn't so much upsides and downsides, as being careful to add a single
bit of functionality, without messing up present behavior and
expectations for a highly-trafficked bit of code. I think we can agree:

1. To leave the buffer name out of it (don't handle leading spaces
   differently)
2. To require `buffer-offer-save' to be explicitly set non-nil in order
   to to consider a non-file buffer for potential saving. I think
   Kaushal's right that we should require both `buffer-offer-save' and
   `write-contents-functions' to be non-nil
3. To leave the current behavior of the PRED argument unchanged

So I think Kaushal's solution is good: it won't change anything at all
except to add a clause saying "when `buffer-offer-save' and
`write-contents-functions' have been set non-nil, consider the buffer
for saving". That's only going to happen when someone explicitly
requests it.

Eric

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Kaushal Modi]

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

On Thu, Sep 21, 2017 at 3:58 PM Eric Abrahamsen <eric@ericabrahamsen.net>
wrote:

>
> It isn't so much upsides and downsides, as being careful to add a single
> bit of functionality, without messing up present behavior and
> expectations for a highly-trafficked bit of code. I think we can agree:
>
> 1. To leave the buffer name out of it (don't handle leading spaces
>    differently)
> 2. To require `buffer-offer-save' to be explicitly set non-nil in order
>    to to consider a non-file buffer for potential saving. I think
>    Kaushal's right that we should require both `buffer-offer-save' and
>    `write-contents-functions' to be non-nil
> 3. To leave the current behavior of the PRED argument unchanged
>
> So I think Kaushal's solution is good: it won't change anything at all
> except to add a clause saying "when `buffer-offer-save' and
> `write-contents-functions' have been set non-nil, consider the buffer
> for saving". That's only going to happen when someone explicitly
> requests it.
>

Thanks for the feedback.. I have some more food for thought:

Case 1: We move forward with this AND condition of buffer-offer-save and
write-contents-functions

- Here one would need to set both buffer-offer-save and
write-contents-functions for emacs to offer saving non-file buffers.

Case 2: We revert this change that adds sensitivity to
write-contents-functions

- Here one would need to set both buffer-offer-save and
save-some-buffers-default-predicate (can be set just locally in a buffer?)
for emacs to offer saving non-file buffers.

So in both cases, we would need to set 2 variables to some non-nil value.
Is Case 1 then better than Case 2? In Case 2, we don't need to change any
code (except for reverting 9b980e2[1] and ee512e9[2]).

[1]:
http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=9b980e2691afa3a7a967011fc004d352750fe618
[2]:
http://git.savannah.gnu.org/cgit/emacs.git/commit/?h=emacs-26&id=ee512e9a825a6dbdf438a432b75b7e18d9a983c7

-- 

Kaushal Modi

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

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eric Abrahamsen]

Kaushal Modi <kaushal.modi@gmail.com> writes:

> On Thu, Sep 21, 2017 at 3:58 PM Eric Abrahamsen <eric@ericabrahamsen.net> wrote:
>
>  It isn't so much upsides and downsides, as being careful to add a single
>  bit of functionality, without messing up present behavior and
>  expectations for a highly-trafficked bit of code. I think we can agree:
>
>  1. To leave the buffer name out of it (don't handle leading spaces
>     differently)
>  2. To require `buffer-offer-save' to be explicitly set non-nil in order
>     to to consider a non-file buffer for potential saving. I think
>     Kaushal's right that we should require both `buffer-offer-save' and
>     `write-contents-functions' to be non-nil
>  3. To leave the current behavior of the PRED argument unchanged
>
>  So I think Kaushal's solution is good: it won't change anything at all
>  except to add a clause saying "when `buffer-offer-save' and
>  `write-contents-functions' have been set non-nil, consider the buffer
>  for saving". That's only going to happen when someone explicitly
>  requests it.
>
> Thanks for the feedback.. I have some more food for thought:
>
> Case 1: We move forward with this AND condition of buffer-offer-save and write-contents-functions
>
> - Here one would need to set both buffer-offer-save and write-contents-functions for emacs to offer saving non-file buffers.
>
> Case 2: We revert this change that adds sensitivity to write-contents-functions 
>
> - Here one would need to set both buffer-offer-save and save-some-buffers-default-predicate (can be set just locally in a buffer?) for emacs to offer saving non-file buffers.
>
> So in both cases, we would need to set 2 variables to some non-nil value. Is Case 1 then better than Case 2? In Case 2, we don't need to change any code (except for reverting 9b980e2[1] and ee512e9[2]).

I'm still in favor of case 1. `save-some-buffers-default-predicate' is
not buffer-local, and thus shouldn't be used by a single package author
to specify that his/her buffers should be eligible for save. What if two
packages both tried to use it? Though `save-some-buffers' slightly
abuses it as a boolean, I think it's clear that this option should be
left to the user. Also, it's docstring suggests that it's there to
*stop* buffers from being offered for save.

`buffer-offer-save', however, is buffer-local, meaning no one will step
on anyone else's toes.

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eli Zaretskii]

> From: Eric Abrahamsen <eric@ericabrahamsen.net>
> Date: Thu, 21 Sep 2017 12:57:39 -0700
> 
> > Are there any upsides and downsides to consider wrt this solution?  If
> > so, what are they?
> 
> It isn't so much upsides and downsides, as being careful to add a single
> bit of functionality, without messing up present behavior and
> expectations for a highly-trafficked bit of code. I think we can agree:
> 
> 1. To leave the buffer name out of it (don't handle leading spaces
>    differently)
> 2. To require `buffer-offer-save' to be explicitly set non-nil in order
>    to to consider a non-file buffer for potential saving. I think
>    Kaushal's right that we should require both `buffer-offer-save' and
>    `write-contents-functions' to be non-nil
> 3. To leave the current behavior of the PRED argument unchanged
> 
> So I think Kaushal's solution is good: it won't change anything at all
> except to add a clause saying "when `buffer-offer-save' and
> `write-contents-functions' have been set non-nil, consider the buffer
> for saving". That's only going to happen when someone explicitly
> requests it.

Reusing buffer-offer-save for this purpose sounds a strange solution
for me.  That variable already says that Emacs should offer saving the
buffer, and now it will have to do double duty in a convoluted manner.
It sounds like we are making the problem harder, rather than simpler,
for modes to solve.

IOW, if we require buffer-offer-save to be non-nil, why do we need to
also require that write-contents-functions is non-nil?  There are 2
clauses in the condition we are talking about, both require
buffer-offer-save to be non-nil, and the only difference between them
seems to be the condition of buffer size being positive.  Is that what
write-contents-functions is about -- to allow saving "empty" buffers?

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eric Abrahamsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>> Date: Thu, 21 Sep 2017 12:57:39 -0700
>> 
>> > Are there any upsides and downsides to consider wrt this solution?  If
>> > so, what are they?
>> 
>> It isn't so much upsides and downsides, as being careful to add a single
>> bit of functionality, without messing up present behavior and
>> expectations for a highly-trafficked bit of code. I think we can agree:
>> 
>> 1. To leave the buffer name out of it (don't handle leading spaces
>>    differently)
>> 2. To require `buffer-offer-save' to be explicitly set non-nil in order
>>    to to consider a non-file buffer for potential saving. I think
>>    Kaushal's right that we should require both `buffer-offer-save' and
>>    `write-contents-functions' to be non-nil
>> 3. To leave the current behavior of the PRED argument unchanged
>> 
>> So I think Kaushal's solution is good: it won't change anything at all
>> except to add a clause saying "when `buffer-offer-save' and
>> `write-contents-functions' have been set non-nil, consider the buffer
>> for saving". That's only going to happen when someone explicitly
>> requests it.
>
> Reusing buffer-offer-save for this purpose sounds a strange solution
> for me.  That variable already says that Emacs should offer saving the
> buffer, and now it will have to do double duty in a convoluted manner.
> It sounds like we are making the problem harder, rather than simpler,
> for modes to solve.

It's appearing twice (which admittedly probably a bad idea), but it
isn't doing double duty: in both cases it simply means that the buffer
should be unilaterally offered for save, regardless of other checks.

> IOW, if we require buffer-offer-save to be non-nil, why do we need to
> also require that write-contents-functions is non-nil?  There are 2
> clauses in the condition we are talking about, both require
> buffer-offer-save to be non-nil, and the only difference between them
> seems to be the condition of buffer size being positive.  Is that what
> write-contents-functions is about -- to allow saving "empty" buffers?

No, it isn't. `write-contents-functions' doesn't need to be in there --
you're right that `buffer-offer-save' should be sufficient -- but then
we'll need to adjust handling of PRED somehow, which is currently being
slightly mis-used as a boolean when Emacs is exiting:

(and pred
     (progn
       (set-buffer buffer)
       (and buffer-offer-save (> (buffer-size) 0))))

Unless a user has customized `save-some-buffers-default-predicate', PRED
is only t when Emacs is exiting.

I don't understand why it's being used this way, or why
`save-some-buffers' should behave differently when called interactively
vs when Emacs is exiting, but I didn't want to touch it because, as has
been pointed out, it's been this way for decades.

If we can come to a consensus about those two questions, it shouldn't be
hard to come up with a clean solution.

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Stefan Monnier]

> (and pred
>      (progn
>        (set-buffer buffer)
>        (and buffer-offer-save (> (buffer-size) 0))))

> Unless a user has customized `save-some-buffers-default-predicate', PRED
> is only t when Emacs is exiting.

But it's also non-nil in some other cases (e.g. it was non-nil when
the function was called as a part of running a command in PCL-CVS, and
presumably the VC-Dir interface should do the same, where the pred` is
used to only prompt saving of buffers within the relevant directory).

> If we can come to a consensus about those two questions, it shouldn't be
> hard to come up with a clean solution.

How 'bout turning buffer-offer-save into a 3-value variable (instead of
a boolean):
- nil = as before (i.e. never)
- t = as before (i.e. only when pred is non-nil and the buffer is not empty)
- `always` = regardless of pred and buffer's size (i.e. always)


        Stefan

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eric Abrahamsen]

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

>> (and pred
>>      (progn
>>        (set-buffer buffer)
>>        (and buffer-offer-save (> (buffer-size) 0))))
>
>> Unless a user has customized `save-some-buffers-default-predicate', PRED
>> is only t when Emacs is exiting.
>
> But it's also non-nil in some other cases (e.g. it was non-nil when
> the function was called as a part of running a command in PCL-CVS, and
> presumably the VC-Dir interface should do the same, where the pred` is
> used to only prompt saving of buffers within the relevant directory).

Hence my reluctance to go trampling on the logic! All kinds of packages
must make use of this behavior.

>> If we can come to a consensus about those two questions, it shouldn't be
>> hard to come up with a clean solution.
>
> How 'bout turning buffer-offer-save into a 3-value variable (instead of
> a boolean):
> - nil = as before (i.e. never)
> - t = as before (i.e. only when pred is non-nil and the buffer is not empty)
> - `always` = regardless of pred and buffer's size (i.e. always)

So like this?

(or
 (buffer-file-name buffer)
 (eq buffer-offer-save 'always)
 (and pred
      (progn
        (set-buffer buffer)
        (and buffer-offer-save (> (buffer-size) 0)))))

But think about it from the point of view of documentation: how do you
explain the interaction between `buffer-offer-save' and PRED?

"If b-o-s is t and PRED is t the buffer will be saved, unless b-o-s is
'always in which case it doesn't matter what PRED is, unless PRED is a
function in which case the only thing that matters is what that function
returns, so long as buffer has a file name."

I'm exaggerating for effect, but still. I still don't quite understand
the use of passing a boolean in for PRED.

Maybe we should start by writing the docs, then the code.

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eli Zaretskii]

> From: Eric Abrahamsen <eric@ericabrahamsen.net>
> Date: Fri, 22 Sep 2017 08:52:30 -0700
> 
> (and pred
>      (progn
>        (set-buffer buffer)
>        (and buffer-offer-save (> (buffer-size) 0))))
> 
> Unless a user has customized `save-some-buffers-default-predicate', PRED
> is only t when Emacs is exiting.
> 
> I don't understand why it's being used this way, or why
> `save-some-buffers' should behave differently when called interactively
> vs when Emacs is exiting, but I didn't want to touch it because, as has
> been pointed out, it's been this way for decades.

Does VCS history explain why PRED is used like that?

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eric Abrahamsen]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Eric Abrahamsen <eric@ericabrahamsen.net>
>> Date: Fri, 22 Sep 2017 08:52:30 -0700
>> 
>> (and pred
>>      (progn
>>        (set-buffer buffer)
>>        (and buffer-offer-save (> (buffer-size) 0))))
>> 
>> Unless a user has customized `save-some-buffers-default-predicate', PRED
>> is only t when Emacs is exiting.
>> 
>> I don't understand why it's being used this way, or why
>> `save-some-buffers' should behave differently when called interactively
>> vs when Emacs is exiting, but I didn't want to touch it because, as has
>> been pointed out, it's been this way for decades.
>
> Does VCS history explain why PRED is used like that?

Sort of. Kaushal and Andreas pointed out that the argument used to be
(since 1986) called EXITING, and was a plain boolean: so far, so clear.

Then, in 2000 or so (ffc0e1caf1a6c8636d0c6a1f3c1650deee845916), the
argument was changed to PRED, and was allowed to be a function as well.
The (and pred...) part was left alone (that's the boolean test), and
this bit was added:

(or (not (functionp pred))
    (with-current-buffer buffer (funcall pred)))

Then earlier in 2017 the new variable
`save-some-buffers-default-predicate' was added, and this code
introduced:

(unless pred
  (setq pred save-some-buffers-default-predicate))

Mostly it just feels wrong that an argument that was once explicitly for
changing behavior at exit time, now is mixed up with a user option that
will always be in effect.

It seems like there are three things to take into consideration:

1. Is `save-some-buffers-default-predicate' a function? If so, and it
   returns t, should we ignore all the other flags and knobs?
2. Is `buffer-offer-save' t, and if so, should we ignore all the other
   flags and knobs?
3. Should we provide different behavior when exiting Emacs?

Right now 1 and 3 are a bit tangled up.

I'm trying to think of a case where someone has set `buffer-offer-save'
non-nil in a buffer, but *only* wants that buffer to be offered for save
when Emacs is exiting, not when the user hits "C-x s".

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Stefan Monnier]

> "If b-o-s is t and PRED is t the buffer will be saved, unless b-o-s is
> 'always in which case it doesn't matter what PRED is, unless PRED is a
> function in which case the only thing that matters is what that function
> returns, so long as buffer has a file name."
>
> I'm exaggerating for effect, but still. I still don't quite understand
> the use of passing a boolean in for PRED.

It's historical.  `pred = t` means "all possible buffers", whereas
`pred = nil` means "all buffers where we usually want to do it" and
another value of `pred` means just those buffers selected by pred.


        Stefan

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eric Abrahamsen]

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

>> "If b-o-s is t and PRED is t the buffer will be saved, unless b-o-s is
>> 'always in which case it doesn't matter what PRED is, unless PRED is a
>> function in which case the only thing that matters is what that function
>> returns, so long as buffer has a file name."
>>
>> I'm exaggerating for effect, but still. I still don't quite understand
>> the use of passing a boolean in for PRED.
>
> It's historical.  `pred = t` means "all possible buffers", whereas
> `pred = nil` means "all buffers where we usually want to do it" and
> another value of `pred` means just those buffers selected by pred.

My logic circuits have fused. Give me a day or so.

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Stefan Monnier]

>> It's historical.  `pred = t` means "all possible buffers", whereas
>> `pred = nil` means "all buffers where we usually want to do it" and
>> another value of `pred` means just those buffers selected by pred.
> My logic circuits have fused.

Good: they need to be deactivated before you can start to understand
this anyway.


        Stefan

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eric Abrahamsen]

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

>> (and pred
>>      (progn
>>        (set-buffer buffer)
>>        (and buffer-offer-save (> (buffer-size) 0))))
>
>> Unless a user has customized `save-some-buffers-default-predicate', PRED
>> is only t when Emacs is exiting.
>
> But it's also non-nil in some other cases (e.g. it was non-nil when
> the function was called as a part of running a command in PCL-CVS, and
> presumably the VC-Dir interface should do the same, where the pred` is
> used to only prompt saving of buffers within the relevant directory).
>
>> If we can come to a consensus about those two questions, it shouldn't be
>> hard to come up with a clean solution.
>
> How 'bout turning buffer-offer-save into a 3-value variable (instead of
> a boolean):
> - nil = as before (i.e. never)
> - t = as before (i.e. only when pred is non-nil and the buffer is not empty)
> - `always` = regardless of pred and buffer's size (i.e. always)

On second thought, this seems like a good idea (sorry for the
gratuitous snark!).

Looking at the `buffer-offer-save' docstring, it does explicitly say it
only comes into effect at exit time, so my complaints were a bit
unjustified.

A less logically-tangled version might look like:

(or
 (buffer-file-name buffer)
 (with-current-buffer buffer
   (or (eq buffer-offer-save 'always)
       (and pred buffer-offer-save (> (buffer-size) 0)))))

I still think that PRED-the-function and PRED-the-exiting-flag should be
separated out into two different arguments, but that's a different
issue.

How does this look?

Eric

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Stefan Monnier]

> How does this look?

LGTM,


        Stefan

Re: [Emacs-diffs] emacs-26 ee512e9: Ignore buffers whose name begins with a space in save-some-buffers

[Eric Abrahamsen]

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

>> How does this look?
>
> LGTM,

Good! I'll put the changes on emacs-26, since this is a continuation of
(and partial reversion of) work already done there.