[PATCH] files.el: avoid asking whether to kill Emacs multiple times

[PATCH] files.el: avoid asking whether to kill Emacs multiple times

[Michal Nazarewicz]

From: Michal Nazarewicz <mina86@mina86.com>

* lisp/files.el (save-buffers-kill-emacs): If `confirm-kill-emacs' is
set, but user has just been asked whether they really want to kill Emacs
(for example with a ‘Modified buffers exist; exit anyway?’ prompt) , do
not ask them for another confirmation.  However, apply this exception
only if `confirm-kill-emacs' is 'yes-or-no-p or 'y-or-n-p, otherwise this
change might errenously prevent some user defined function from being
run (adding such a function to `kill-emacs-query-functions' is probably
better option, but we don’t want to break any usage even if it’s
incorrect).
---
 lisp/files.el | 64 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 35 insertions(+), 29 deletions(-)

diff --git a/lisp/files.el b/lisp/files.el
index e9632ed..7d94bc1 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -6590,35 +6590,41 @@ (defun save-buffers-kill-emacs (&optional arg)
 if any returns nil.  If `confirm-kill-emacs' is non-nil, calls it."
   (interactive "P")
   (save-some-buffers arg t)
-  (and (or (not (memq t (mapcar (function
-				  (lambda (buf) (and (buffer-file-name buf)
-						     (buffer-modified-p buf))))
-				(buffer-list))))
-	   (yes-or-no-p "Modified buffers exist; exit anyway? "))
-       (or (not (fboundp 'process-list))
-	   ;; process-list is not defined on MSDOS.
-	   (let ((processes (process-list))
-		 active)
-	     (while processes
-	       (and (memq (process-status (car processes)) '(run stop open listen))
-		    (process-query-on-exit-flag (car processes))
-		    (setq active t))
-	       (setq processes (cdr processes)))
-	     (or (not active)
-		 (with-current-buffer-window
-		  (get-buffer-create "*Process List*") nil
-		  #'(lambda (window _value)
-		      (with-selected-window window
-			(unwind-protect
-			    (yes-or-no-p "Active processes exist; kill them and exit anyway? ")
-			  (when (window-live-p window)
-			    (quit-restore-window window 'kill)))))
-		  (list-processes t)))))
-       ;; Query the user for other things, perhaps.
-       (run-hook-with-args-until-failure 'kill-emacs-query-functions)
-       (or (null confirm-kill-emacs)
-	   (funcall confirm-kill-emacs "Really exit Emacs? "))
-       (kill-emacs)))
+  (let (asked)
+    (and
+     (or (not (memq t (mapcar (function
+                               (lambda (buf) (and (buffer-file-name buf)
+                                                  (buffer-modified-p buf))))
+                              (buffer-list))))
+         (progn (setq asked t)
+                (yes-or-no-p "Modified buffers exist; exit anyway? ")))
+     (or (not (fboundp 'process-list))
+         ;; process-list is not defined on MSDOS.
+         (let ((processes (process-list))
+               active)
+           (while processes
+             (and (memq (process-status (car processes)) '(run stop open listen))
+                  (process-query-on-exit-flag (car processes))
+                  (setq active t))
+             (setq processes (cdr processes)))
+           (or (not active)
+               (with-current-buffer-window
+                (get-buffer-create "*Process List*") nil
+                #'(lambda (window _value)
+                    (with-selected-window window
+                      (unwind-protect
+                          (progn
+                            (setq asked t)
+                            (yes-or-no-p "Active processes exist; kill them and exit anyway? "))
+                        (when (window-live-p window)
+                          (quit-restore-window window 'kill)))))
+                (list-processes t)))))
+     ;; Query the user for other things, perhaps.
+     (run-hook-with-args-until-failure 'kill-emacs-query-functions)
+     (or (null confirm-kill-emacs)
+         (and asked (memq confirm-kill-emacs '(yes-or-no-p y-or-n-p)))
+         (funcall confirm-kill-emacs "Really exit Emacs? "))
+     (kill-emacs))))
 
 (defun save-buffers-kill-terminal (&optional arg)
   "Offer to save each buffer, then kill the current connection.
-- 
2.2.0.rc0.207.ga3a616c

RE: [PATCH] files.el: avoid asking whether to kill Emacs multiple times

[Drew Adams]

> If `confirm-kill-emacs' is set, but user has just been asked whether
> they really want to kill Emacs (for example with a ‘Modified buffers
> exist; exit anyway?’ prompt), do not ask them for another confirmation.  
> However, apply this exception only if `confirm-kill-emacs' is
> 'yes-or-no-p or 'y-or-n-p, otherwise this change might errenously
> prevent some user defined function from being run (adding such a
> function to `kill-emacs-query-functions' is probably better option,
> but we don’t want to break any usage even if it’s incorrect).

Related?
http://debbugs.gnu.org/cgi/bugreport.cgi?bug=9577

Re: [PATCH] files.el: avoid asking whether to kill Emacs multiple times

[Stefan Monnier]

>> If `confirm-kill-emacs' is set, but user has just been asked whether
>> they really want to kill Emacs (for example with a ‘Modified buffers
>> exist; exit anyway?’ prompt), do not ask them for another confirmation.

I agree that skipping the second confirmation would be desirable.

>> However, apply this exception only if `confirm-kill-emacs' is
>> 'yes-or-no-p or 'y-or-n-p, otherwise this change might errenously
>> prevent some user defined function from being run.

Is it really worth the trouble?  Comparing functions is just a bad idea
in general, so if we can avoid it, it's preferable.

> Related?
> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=9577

Another painful one is when a file is modified outside of Emacs but you
still want to edit the current buffer and then save it (hence
overwriting the changes made outside of Emacs).  I think in total this
gets you 3 confirmation prompts and at least one of them should go.


        Stefan

Re: [PATCH] files.el: avoid asking whether to kill Emacs multiple times

[David Kastrup]

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

>>> If `confirm-kill-emacs' is set, but user has just been asked whether
>>> they really want to kill Emacs (for example with a ‘Modified buffers
>>> exist; exit anyway?’ prompt), do not ask them for another confirmation.
>
> I agree that skipping the second confirmation would be desirable.
>
>>> However, apply this exception only if `confirm-kill-emacs' is
>>> 'yes-or-no-p or 'y-or-n-p, otherwise this change might errenously
>>> prevent some user defined function from being run.
>
> Is it really worth the trouble?  Comparing functions is just a bad idea
> in general, so if we can avoid it, it's preferable.
>
>> Related?
>> http://debbugs.gnu.org/cgi/bugreport.cgi?bug=9577
>
> Another painful one is when a file is modified outside of Emacs but you
> still want to edit the current buffer and then save it (hence
> overwriting the changes made outside of Emacs).  I think in total this
> gets you 3 confirmation prompts and at least one of them should go.

I think in connection with something like C-x v u some of the later
questions are rather hard to answer.  You just don't have a clue what
will be retained and what will be overwritten.

I have, on occasion, reverted to just doing a manual copy of the file
from the shell before daring to answer Emacs.

-- 
David Kastrup

[PATCHv2] files.el: avoid asking whether to kill Emacs multiple times

[Michal Nazarewicz]

* lisp/files.el (save-buffers-kill-emacs): If `confirm-kill-emacs' is
set, but user has just been asked whether they really want to kill Emacs
(for example with a ‘Modified buffers exist; exit anyway?’ prompt) , do
not ask them for another confirmation.
---
 lisp/files.el | 63 ++++++++++++++++++++++++++++++++---------------------------
 1 file changed, 34 insertions(+), 29 deletions(-)

On Thu, Jan 29 2015, Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:
>>> If `confirm-kill-emacs' is set, but user has just been asked whether
>>> they really want to kill Emacs (for example with a ‘Modified buffers
>>> exist; exit anyway?’ prompt), do not ask them for another confirmation.
>
> I agree that skipping the second confirmation would be desirable.
>
>>> However, apply this exception only if `confirm-kill-emacs' is
>>> 'yes-or-no-p or 'y-or-n-p, otherwise this change might errenously
>>> prevent some user defined function from being run.

> Is it really worth the trouble?  Comparing functions is just a bad
> idea in general, so if we can avoid it, it's preferable.

I’m happy to remove that comparison.  I just wanted to avoid situations
where we break things for someone doing something weird like:

  (setq confirm-kill-emacs
        (lambda (_) …something that really should be 
                     a kill-emacs-query-functions…))

Not having this special case makes code easier though so I can live with
a conclusion that breaking such code is worth it.

diff --git a/lisp/files.el b/lisp/files.el
index 40a4289..5e80cb7 100644
--- a/lisp/files.el
+++ b/lisp/files.el
@@ -6590,35 +6590,40 @@ (defun save-buffers-kill-emacs (&optional arg)
 if any returns nil.  If `confirm-kill-emacs' is non-nil, calls it."
   (interactive "P")
   (save-some-buffers arg t)
-  (and (or (not (memq t (mapcar (function
-				  (lambda (buf) (and (buffer-file-name buf)
-						     (buffer-modified-p buf))))
-				(buffer-list))))
-	   (yes-or-no-p "Modified buffers exist; exit anyway? "))
-       (or (not (fboundp 'process-list))
-	   ;; process-list is not defined on MSDOS.
-	   (let ((processes (process-list))
-		 active)
-	     (while processes
-	       (and (memq (process-status (car processes)) '(run stop open listen))
-		    (process-query-on-exit-flag (car processes))
-		    (setq active t))
-	       (setq processes (cdr processes)))
-	     (or (not active)
-		 (with-current-buffer-window
-		  (get-buffer-create "*Process List*") nil
-		  #'(lambda (window _value)
-		      (with-selected-window window
-			(unwind-protect
-			    (yes-or-no-p "Active processes exist; kill them and exit anyway? ")
-			  (when (window-live-p window)
-			    (quit-restore-window window 'kill)))))
-		  (list-processes t)))))
-       ;; Query the user for other things, perhaps.
-       (run-hook-with-args-until-failure 'kill-emacs-query-functions)
-       (or (null confirm-kill-emacs)
-	   (funcall confirm-kill-emacs "Really exit Emacs? "))
-       (kill-emacs)))
+  (let ((confirm confirm-kill-emacs))
+    (and
+     (or (not (memq t (mapcar (function
+                               (lambda (buf) (and (buffer-file-name buf)
+                                                  (buffer-modified-p buf))))
+                              (buffer-list))))
+         (progn (setq confirm nil)
+                (yes-or-no-p "Modified buffers exist; exit anyway? ")))
+     (or (not (fboundp 'process-list))
+         ;; process-list is not defined on MSDOS.
+         (let ((processes (process-list))
+               active)
+           (while processes
+             (and (memq (process-status (car processes)) '(run stop open listen))
+                  (process-query-on-exit-flag (car processes))
+                  (setq active t))
+             (setq processes (cdr processes)))
+           (or (not active)
+               (with-current-buffer-window
+                (get-buffer-create "*Process List*") nil
+                #'(lambda (window _value)
+                    (with-selected-window window
+                      (unwind-protect
+                          (progn
+                            (setq confirm nil)
+                            (yes-or-no-p "Active processes exist; kill them and exit anyway? "))
+                        (when (window-live-p window)
+                          (quit-restore-window window 'kill)))))
+                (list-processes t)))))
+     ;; Query the user for other things, perhaps.
+     (run-hook-with-args-until-failure 'kill-emacs-query-functions)
+     (or (null confirm)
+         (funcall confirm "Really exit Emacs? "))
+     (kill-emacs))))
 
 (defun save-buffers-kill-terminal (&optional arg)
   "Offer to save each buffer, then kill the current connection.
-- 
2.2.0.rc0.207.ga3a616c

Re: [PATCHv2] files.el: avoid asking whether to kill Emacs multiple times

[Stefan Monnier]

> I’m happy to remove that comparison.  I just wanted to avoid situations
> where we break things for someone doing something weird like:

>   (setq confirm-kill-emacs
>         (lambda (_) …something that really should be 
>                      a kill-emacs-query-functions…))

I think we can take this risk.  Feel free to install your patch.


        Stefan

Re: [PATCHv2] files.el: avoid asking whether to kill Emacs multiple times

[Michal Nazarewicz]

On Fri, Jan 30 2015, Stefan Monnier <monnier@IRO.UMontreal.CA> wrote:
>> I’m happy to remove that comparison.  I just wanted to avoid situations
>> where we break things for someone doing something weird like:
>>
>>   (setq confirm-kill-emacs
>>         (lambda (_) …something that really should be 
>>                      a kill-emacs-query-functions…))

> I think we can take this risk.  Feel free to install your patch.

Done.

-- 
Best regards,                                         _     _
.o. | Liege of Serenely Enlightened Majesty of      o' \,=./ `o
..o | Computer Science,  Michał “mina86” Nazarewicz    (o o)
ooo +--<mpn@google.com>--<xmpp:mina86@jabber.org>--ooO--(_)--Ooo--