bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

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

When killing an emacsclient terminal via C-x C-c, it should prompt to 
save the files initially passed to emacsclient. To see this in action:

   $ emacs -Q --daemon
   $ emacsclient -a "" -c foo.txt
   $ emacsclient -a "" -c bar.txt

   ;; In the first client frame:
   foobar ;; Insert some text
   C-x C-c
   ;; Emacs prompts "Save file /path/to/foo.txt?..."

Now try the above, but call `(server-stop-automatically 'delete-frame)' 
first (or replace `delete-frame' with `kill-terminal'; it doesn't 
matter). In this case, Emacs doesn't prompt to save the file. However, 
the docstring/comments in `server-save-buffers-kill-terminal' say that 
it should: "Offer to save each buffer, then kill the current client. ... 
Only files from emacsclient file list."

Attached is a patch to restore this behavior when stopping the server 
automatically. This puts all of the logic into 
`server-save-buffers-kill-terminal', which allows 
`server-stop-automatically--handle-delete-frame' to be simpler. I've 
also added some more detailed comments explaining the logic here, since 
there are some pretty subtle aspects to it.

There's a further subtlety that I should probably mention here though: 
when killing a nowait frame, it would kill Emacs entirely if that were 
the last frame (even in Emacs 28, and probably earlier). The only way 
(that I can think of) that this could come up would be to run:

   $ emacs -Q --eval '(start-server)'
   $ emacsclient -n
   C-x 5 0 ;; in the non-client frame
   C-x C-c ;; in the emacsclient frame

However, when doing this with a regular (non-nowait) client, the last 
step would report the error "Attempt to delete the sole visible or 
iconified frame". Even more oddly, it would work the *second* time you 
tried to kill the client terminal, since `server-delete-client' would 
set the `client' frame-parameter to nil before deleting it; on the 
second attempt, Emacs thinks the frame is a non-client frame (even 
though it is).

I've fixed this in the second patch by following the nowait behavior: if 
you kill a client and *all* the existing frames belong to that client, 
it kills Emacs entirely. I'm not sure this will come up often in 
practice, but it's a fairly simple change.

Some tests would be nice to prevent this from regressing, but I'm not 
sure how to write a test that starts up a daemon and connects clients to 
it...

[-- Attachment #2: 0001-Ensure-killing-an-emacsclient-terminal-prompts-to-sa.patch --]
[-- Type: text/plain, Size: 5802 bytes --]

From 563036eaf0924f4777f25e74e9f611419fc031cf Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 19 Nov 2021 19:49:38 -0800
Subject: [PATCH 1/2] Ensure killing an emacsclient terminal prompts to save
 files

Previously, when using 'server-stop-automatically', killing an
emacsclient terminal wouldn't prompt the user to save the files
initially passed to emacsclient (unless it was the last client).

* lisp/server.el (server-save-buffers-kill-terminal): Check
'server-stop-automatically' and handle killing Emas as needed.
(server-stop-automatically--handle-delete-frame): Remove logic
specific to 'save-buffers-kill-terminal'.
---
 lisp/server.el | 94 +++++++++++++++++++++++++++-----------------------
 1 file changed, 51 insertions(+), 43 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 2f003a380a..e2d20b1b02 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1727,52 +1727,60 @@ server-save-buffers-kill-terminal
 
 If emacsclient was started with a list of filenames to edit, then
 only these files will be asked to be saved."
-  (if server-stop-automatically
-      (server-stop-automatically--handle-delete-frame (selected-frame))
-    (let ((proc (frame-parameter nil 'client)))
-      (cond ((eq proc 'nowait)
-	     ;; Nowait frames have no client buffer list.
-	     (if (cdr (frame-list))
-	         (progn (save-some-buffers arg)
-		        (delete-frame))
-	       ;; If we're the last frame standing, kill Emacs.
-	       (save-buffers-kill-emacs arg)))
-	    ((processp proc)
-	     (let ((buffers (process-get proc 'buffers)))
-	       (save-some-buffers
-	        arg (if buffers
-                        ;; Only files from emacsclient file list.
-		        (lambda () (memq (current-buffer) buffers))
-                      ;; No emacsclient file list: don't override
-                      ;; `save-some-buffers-default-predicate' (unless
-                      ;; ARG is non-nil), since we're not killing
-                      ;; Emacs (unlike `save-buffers-kill-emacs').
-		      (and arg t)))
-	       (server-delete-client proc)))
-	    (t (error "Invalid client frame"))))))
+  (let ((proc (frame-parameter nil 'client)))
+    (cond ((eq proc 'nowait)
+           ;; Nowait frames have no client buffer list.
+           (if (if server-stop-automatically
+                   (cddr (frame-list))
+                 (cdr (frame-list)))
+               ;; If there are any other frames (excluding the daemon
+               ;; frame when `server-stop-automatically' is non-nil),
+               ;; only delete this frame.  FIXME: It would be nice to
+               ;; delete any other frames created by this frame (as
+               ;; when killing the terminal of an ordinary client
+               ;; below), but we can't distinguish separate groups of
+               ;; nowait frames currently.
+               (progn (save-some-buffers arg)
+                      (delete-frame))
+             ;; If we're the last frame standing, kill Emacs.
+             (save-buffers-kill-emacs arg)))
+          ((processp proc)
+           (if (seq-some
+                (lambda (frame)
+                  (let ((p (frame-parameter frame 'client)))
+                    (unless (and server-stop-automatically (null p))
+                      (not (equal proc p)))))
+                (frame-list))
+               ;; If there are any frames from other clients, only
+               ;; delete this client.  If `server-stop-automatically'
+               ;; is nil, check for frames from the server process as
+               ;; well.
+               (let ((buffers (process-get proc 'buffers)))
+                 (save-some-buffers
+                  arg (if buffers
+                          ;; Only files from emacsclient file list.
+                          (lambda () (memq (current-buffer) buffers))
+                        ;; No emacsclient file list: don't override
+                        ;; `save-some-buffers-default-predicate'
+                        ;; (unless ARG is non-nil), since we're not
+                        ;; killing Emacs (unlike
+                        ;; `save-buffers-kill-emacs').
+                        (and arg t)))
+                 (server-delete-client proc))
+             ;; If all frames are from this client, kill Emacs.
+             (save-buffers-kill-emacs arg)))
+          (t (error "Invalid client frame")))))
 
 (defun server-stop-automatically--handle-delete-frame (frame)
   "Handle deletion of FRAME when `server-stop-automatically' is used."
-  (when server-stop-automatically
-    (if (if (and (processp (frame-parameter frame 'client))
-		 (eq this-command 'save-buffers-kill-terminal))
-	    (progn
-	      (dolist (f (frame-list))
-		(when (and (eq (frame-parameter frame 'client)
-			       (frame-parameter f 'client))
-			   (not (eq frame f)))
-		  (set-frame-parameter f 'client nil)
-		  (let ((server-stop-automatically nil))
-		    (delete-frame f))))
-	      (if (cddr (frame-list))
-		  (let ((server-stop-automatically nil))
-		    (delete-frame frame)
-		    nil)
-		t))
-	  (null (cddr (frame-list))))
-	(let ((server-stop-automatically nil))
-	  (save-buffers-kill-emacs)
-	  (delete-frame frame)))))
+  (when (and ;; Check that the frame is a client frame.
+             ;; Note: `server-delete-client' sets `client' to nil
+             ;; before calling `delete-frame'. That's good, since we
+             ;; don't want to call `save-buffers-kill-emacs' in that
+             ;; case anyway.
+             (frame-parameter frame 'client)
+             (null (cddr (frame-list))))
+    (save-buffers-kill-emacs)))
 
 (defun server-stop-automatically--maybe-kill-emacs ()
   "Handle closing of Emacs daemon when `server-stop-automatically' is used."
-- 
2.25.1


[-- Attachment #3: 0002-Don-t-explicitly-delete-client-frames-when-killing-E.patch --]
[-- Type: text/plain, Size: 2315 bytes --]

From cccba7784d7a5af41b651ab0c29d2339bebc6732 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 19 Nov 2021 20:14:33 -0800
Subject: [PATCH 2/2] Don't explicitly delete client frames when killing Emacs
 anyway

This resolves an obscure issue where killing an emacsclient terminal
when there were no other frames would fail. Now, killing the terminal
properly kills Emacs.

* lisp/server (server-start): Add 'noframe' option to avoid deleting
frames.
(server-force-stop): Pass 'noframe' to 'server-stop'.
---
 lisp/server.el | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index e2d20b1b02..ec32da4fdb 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -609,7 +609,7 @@ server-get-auth-key
     (server-generate-key)))
 
 ;;;###autoload
-(defun server-start (&optional leave-dead inhibit-prompt)
+(defun server-start (&optional leave-dead inhibit-prompt noframe)
   "Allow this Emacs process to be a server for client processes.
 This starts a server communications subprocess through which client
 \"editors\" can send your editing commands to this Emacs job.
@@ -619,6 +619,10 @@ server-start
 Optional argument LEAVE-DEAD (interactively, a prefix arg) means just
 kill any existing server communications subprocess.
 
+If NOFRAME is non-nil, let any existing frames associated with a
+client process live.  This is useful, for example, when killing
+Emacs, in which case the frames will die anyway.
+
 If a server is already running, restart it.  If clients are
 running, ask the user for confirmation first, unless optional
 argument INHIBIT-PROMPT is non-nil.
@@ -681,7 +685,7 @@ server-start
          (setq leave-dead t)))
       ;; If this Emacs already had a server, clear out associated status.
       (while server-clients
-	(server-delete-client (car server-clients)))
+	(server-delete-client (car server-clients) noframe))
       ;; Now any previous server is properly stopped.
       (if leave-dead
 	  (progn
@@ -740,7 +744,7 @@ server-start
 (defun server-force-stop ()
   "Kill all connections to the current server.
 This function is meant to be called from `kill-emacs-hook'."
-  (server-start t t))
+  (server-start t t 'noframe))
 
 ;;;###autoload
 (defun server-force-delete (&optional name)
-- 
2.25.1

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Eli Zaretskii]

> From: Jim Porter <jporterbugs@gmail.com>
> Date: Fri, 19 Nov 2021 20:29:43 -0800
> 
> When killing an emacsclient terminal via C-x C-c, it should prompt to 
> save the files initially passed to emacsclient. To see this in action:
> 
>    $ emacs -Q --daemon
>    $ emacsclient -a "" -c foo.txt
>    $ emacsclient -a "" -c bar.txt
> 
>    ;; In the first client frame:
>    foobar ;; Insert some text
>    C-x C-c
>    ;; Emacs prompts "Save file /path/to/foo.txt?..."
> 
> Now try the above, but call `(server-stop-automatically 'delete-frame)' 
> first (or replace `delete-frame' with `kill-terminal'; it doesn't 
> matter). In this case, Emacs doesn't prompt to save the file. However, 
> the docstring/comments in `server-save-buffers-kill-terminal' say that 
> it should: "Offer to save each buffer, then kill the current client. ... 
> Only files from emacsclient file list."

Gregory, any comments?

Thanks.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Gregory Heytings]

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


>> When killing an emacsclient terminal via C-x C-c, it should prompt to 
>> save the files initially passed to emacsclient. To see this in action:
>>
>>    $ emacs -Q --daemon
>>    $ emacsclient -a "" -c foo.txt
>>    $ emacsclient -a "" -c bar.txt
>>
>>    ;; In the first client frame:
>>    foobar ;; Insert some text
>>    C-x C-c
>>    ;; Emacs prompts "Save file /path/to/foo.txt?..."
>>
>> Now try the above, but call `(server-stop-automatically 'delete-frame)' 
>> first (or replace `delete-frame' with `kill-terminal'; it doesn't 
>> matter). In this case, Emacs doesn't prompt to save the file. However, 
>> the docstring/comments in `server-save-buffers-kill-terminal' say that 
>> it should: "Offer to save each buffer, then kill the current client. 
>> ... Only files from emacsclient file list."
>
> Gregory, any comments?
>

This is not a bug, this is the intented behavior of that feature, which 
was discussed on emacs-devel and in bug#51377.

But in commit 997ca88ef44 the word "last" disappeared in the explanation 
of the meaning of the symbol 'kill-terminal': "when the last frame is 
being closed" became "when the terminal is killed".  Hence the confusion.

I attached a patch which preserves the intended behavior of that feature, 
and adds a fourth possible behavior, the one Jim now wants.

[-- Attachment #2: Type: text/x-diff, Size: 7496 bytes --]

From 4a3b8cecfc091cb48f77f8a5a3a3934ec383d22d Mon Sep 17 00:00:00 2001
From: Gregory Heytings <gregory@heytings.org>
Date: Tue, 23 Nov 2021 09:17:39 +0000
Subject: [PATCH] Improve and extend server-stop-automatically.

* lisp/server.el (server-stop-automatically): Add another allowed
symbol argument, and rename two existing ones.  Update the docstring.
(server-stop-automatically--handle-delete-frame): Handle the new
symbol argument.  Improve the handling of nowait frames.  Use the
prefix argument of server-save-buffers-kill-terminal.
(server-save-buffers-kill-terminal): Pass the prefix argument to
server-stop-automatically--handle-delete-frame.

* doc/emacs/misc.texi (Emacs Server): Update the documentation with
the new allowed symbol argument.

* etc/NEWS: Update the documentation.
---
 doc/emacs/misc.texi | 26 ++++++++++++++--------
 etc/NEWS            |  2 +-
 lisp/server.el      | 53 ++++++++++++++++++++++++++++++---------------
 3 files changed, 54 insertions(+), 27 deletions(-)

diff --git a/doc/emacs/misc.texi b/doc/emacs/misc.texi
index 1f2c852fac..159e50a6c1 100644
--- a/doc/emacs/misc.texi
+++ b/doc/emacs/misc.texi
@@ -1787,17 +1787,25 @@ Emacs Server
 anymore.
 
 @item
-With the argument @code{delete-frame}, when the last client frame is
-being closed, you are asked whether each unsaved file-visiting buffer
-must be saved and each unfinished process can be stopped, and if so,
-the server is stopped.
+With the argument @code{delete-last-frame}, when the last client frame
+is being closed, you are asked whether each unsaved file-visiting
+buffer must be saved and each unfinished process can be stopped, and
+if so, the server is stopped.
 
 @item
-With the argument @code{kill-terminal}, when the last client frame is
-being closed with @kbd{C-x C-c} (@code{save-buffers-kill-terminal}),
-you are asked whether each unsaved file-visiting buffer must be saved
-and each unfinished process can be stopped, and if so, the server is
-stopped.
+With the argument @code{kill-last-terminal} or @code{kill-terminal},
+when the last client terminal is being closed with @kbd{C-x C-c}
+(@code{save-buffers-kill-terminal}), you are asked whether each
+unsaved file-visiting buffer must be saved and each unfinished process
+can be stopped, and if so, the server is stopped.
+
+@item
+With the argument @code{kill-terminal}, when each client terminal but
+the last one is being closed with @kbd{C-x C-c}
+(@code{save-buffers-kill-terminal}), you are also asked whether
+unsaved file-visiting buffers must be saved, or, if
+@command{emacsclient} was started with a list of files to edit,
+whether these files must be saved.
 @end itemize
 
 @findex server-eval-at
diff --git a/etc/NEWS b/etc/NEWS
index bfea4da8b9..2af40106fe 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -197,7 +197,7 @@ frame if one exists; otherwise it creates a new frame.
 *** 'server-stop-automatically' can be used to automatically stop the server.
 The Emacs server will be automatically stopped when certain conditions
 are met.  The conditions are given by the argument, which can be
-'empty', 'delete-frame' or 'kill-terminal'.
+'empty', 'delete-last-frame', 'kill-last-terminal' or 'kill-terminal'.
 
 * Editing Changes in Emacs 29.1
 
diff --git a/lisp/server.el b/lisp/server.el
index 2f003a380a..e2ada697f9 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1728,7 +1728,8 @@ server-save-buffers-kill-terminal
 If emacsclient was started with a list of filenames to edit, then
 only these files will be asked to be saved."
   (if server-stop-automatically
-      (server-stop-automatically--handle-delete-frame (selected-frame))
+      (server-stop-automatically--handle-delete-frame
+       (selected-frame) arg)
     (let ((proc (frame-parameter nil 'client)))
       (cond ((eq proc 'nowait)
 	     ;; Nowait frames have no client buffer list.
@@ -1751,7 +1752,7 @@ server-save-buffers-kill-terminal
 	       (server-delete-client proc)))
 	    (t (error "Invalid client frame"))))))
 
-(defun server-stop-automatically--handle-delete-frame (frame)
+(defun server-stop-automatically--handle-delete-frame (frame arg)
   "Handle deletion of FRAME when `server-stop-automatically' is used."
   (when server-stop-automatically
     (if (if (and (processp (frame-parameter frame 'client))
@@ -1765,13 +1766,22 @@ server-stop-automatically--handle-delete-frame
 		  (let ((server-stop-automatically nil))
 		    (delete-frame f))))
 	      (if (cddr (frame-list))
-		  (let ((server-stop-automatically nil))
-		    (delete-frame frame)
-		    nil)
+		  (let ((kill-terminal
+			 (eq server-stop-automatically 'kill-terminal)))
+		    (let ((server-stop-automatically nil))
+		      (if kill-terminal
+			  (server-save-buffers-kill-terminal arg)
+			(delete-frame frame))
+		      nil))
 		t))
-	  (null (cddr (frame-list))))
+	  (if (and (eq (frame-parameter frame 'client) 'nowait)
+		   (cddr (frame-list)))
+	      (let ((server-stop-automatically nil))
+		(delete-frame frame)
+		nil)
+	    (null (cddr (frame-list)))))
 	(let ((server-stop-automatically nil))
-	  (save-buffers-kill-emacs)
+	  (save-buffers-kill-emacs arg)
 	  (delete-frame frame)))))
 
 (defun server-stop-automatically--maybe-kill-emacs ()
@@ -1797,17 +1807,25 @@ server-stop-automatically
 remaining clients, no remaining unsaved file-visiting buffers,
 and no running processes with a `query-on-exit' flag.
 
-If ARG is the symbol `delete-frame', ask the user when the last
-frame is deleted whether each unsaved file-visiting buffer must
-be saved and each running process with a `query-on-exit' flag
-can be stopped, and if so, stop the server itself.
-
-If ARG is the symbol `kill-terminal', ask the user when the
-terminal is killed with \\[save-buffers-kill-terminal] \
-whether each unsaved file-visiting
-buffer must be saved and each running process with a `query-on-exit'
+If ARG is the symbol `delete-last-frame', ask the user when the
+last frame is deleted whether each unsaved file-visiting buffer
+must be saved and each running process with a `query-on-exit'
 flag can be stopped, and if so, stop the server itself.
 
+If ARG is the symbol `kill-last-terminal' or `kill-terminal',
+ask the user when the last terminal is killed with \
+\\[save-buffers-kill-terminal]
+whether each unsaved file-visiting buffer must be saved and each
+running process with a `query-on-exit' flag can be stopped, and
+if so, stop the server itself.
+
+If ARG is the symbol `kill-terminal', also ask the user when each
+but the last terminal is killed with \\[save-buffers-kill-terminal] \
+whether unsaved
+file-visiting buffers must be saved, or, if emacsclient was
+started with a list of files to edit, whether these files must be
+saved.
+
 Any other value of ARG will cause this function to signal an error.
 
 This function is meant to be called from the user init file."
@@ -1818,9 +1836,10 @@ server-stop-automatically
       (setq server-stop-automatically nil)
       (run-with-timer 10 2
 		      #'server-stop-automatically--maybe-kill-emacs))
-     ((eq arg 'delete-frame)
+     ((eq arg 'delete-last-frame)
       (add-hook 'delete-frame-functions
 		#'server-stop-automatically--handle-delete-frame))
+     ((eq arg 'kill-last-terminal))
      ((eq arg 'kill-terminal))
      (t
       (error "Unexpected argument")))))
-- 
2.33.0

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

(Cc'ing Lars since he merged the previous patch, and I want to be sure 
everyone's on the same page here.)

On 11/23/2021 1:48 AM, Gregory Heytings wrote:
> This is not a bug, this is the intented behavior of that feature, which 
> was discussed on emacs-devel and in bug#51377.

I started that discussion[1] (and participated throughout it), and I 
don't think we actually agreed that this was the intended behavior. The 
closest I see is this:

On 10/24/2021 11:08 AM, Jim Porter wrote[2]:
> I don't think this is true in general. The docstring for
> `server-save-buffers-kill-terminal' says: "If emacsclient was started
> with a list of filenames to edit, then only these files will be asked to
> be saved." As a result, some files with unsaved changes may still exist,
> so we'd want to prompt about those *before* the last frame is closed.

However, I should stress that the case I brought up above is just a 
counterexample to show a problem with a previous implementation 
strategy, not a full specification. That's part of why I posted patches 
in bug#51377 in the hopes that an implementation would explain the 
behavior I intend more precisely than prose.

The current behavior on Emacs 29 certainly isn't what I personally 
intended when bringing the idea up on emacs-devel.

On 11/23/2021 1:48 AM, Gregory Heytings wrote:
> I attached a patch which preserves the intended behavior of that 
> feature, and adds a fourth possible behavior, the one Jim now wants.

I'm concerned that we're now up to 4 different behaviors, when I think 
two of them are just the result of a miscommunication between the two of 
us. The way I've interpreted our prior discussion is that you would 
prefer the daemon to be killed invisibly if there's no reason to keep it 
alive; this is the `empty' option in your patches. On the other hand, 
I'd prefer to the daemon to be killed "loudly" when there are no 
non-daemon frames left, including being prompted to save buffers, kill 
processes, etc in all the "usual" cases; this is `delete-last-frame' in 
your patch, plus a couple of other tweaks I have pending. (Note: Just to 
be clear, this isn't a specification; it's only a brief summary.)

I think your message to me in bug#51377 encapsulates this well:

On 10/24/2021 2:37 PM, Gregory Heytings wrote[3]:
> I see.  We have different mental models, I guess.  From my viewpoint
> the Emacs server should stay there until it's not necessary, and I'd be
> surprised to be queried about what to do with buffers opened of
> processes started in a frame I already closed when I want to close
> another frame. But of course I do not object to have both behaviors.

However, in your next two messages in the bug, you added:

On 10/26/2021 3:37 AM, Gregory Heytings wrote[4]:
> If it's now also necessary to kill the daemon when you close the
> last Emacs frame with the window manager close button (I did not see
> this requirement in your original post)...

On 10/26/2021 4:59 AM, Gregory Heytings wrote[5]:
> It just occurred to me that it's very easy to add a third behavior,
> namely the one you expect...

(The "third behavior" is the `delete-frame' option.) As I understand 
things, both the `kill-terminal' and `delete-frame' options were added 
to support my mental model in particular (we were the only two people 
commenting on the bug at that time). From my perspective, 
`kill-terminal` (and now `kill-last-terminal' as well) are just the 
result of some miscommunication between the two of us in bug#51377. 
Unless there's a strong argument for keeping them around, I think it 
would be best to remove them so that there are only 2 options (well, 3 
if you include the default behavior).

I hope the above explains things reasonably thoroughly for everyone 
(hence all the citations to previous discussions). If pressed, I could 
probably create a full specification of the behavior I'd like to see, 
but I find it quite a bit easier to explain by way of a patch. If anyone 
needs clarification on any of the above, just let me know and I'll try 
to elaborate.

[1] https://lists.gnu.org/archive/html/emacs-devel/2021-10/msg01465.html
[2] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-10/msg02163.html
[3] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-10/msg02207.html
[4] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-10/msg02367.html
[5] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-10/msg02373.html

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Gregory Heytings]

>> This is not a bug, this is the intented behavior of that feature
>
> I started that discussion (and participated throughout it), and I don't 
> think we actually agreed that this was the intended behavior.
>

This is the behavior I intended (and described in the docstring and 
manual), if you prefer.  And you did not make further comments in 
bug#51377, which can be interpreted as a kind of agreement.

>
> I should stress that the case I brought up above is just a 
> counterexample to show a problem with a previous implementation strategy
>

Which problem?

>
> The current behavior on Emacs 29 certainly isn't what I personally 
> intended when bringing the idea up on emacs-devel.
>

Is the current behavior of Emacs 29 with my patch and 
(server-stop-automatically 'kill-terminal) still not what you want?  If 
not, what is missing?

>
> I'm concerned that we're now up to 4 different behaviors, when I think 
> two of them are just the result of a miscommunication between the two of 
> us.
>

They are not, AFAICS.  The four behaviors are four reasonable options, 
each of which can (and is) described in a short paragraph, and corresponds 
to a different user preference.  I see no reason to remove any of the 
current three behaviors because of an unspecified "problem".  Especially 
given that all these behaviors are implemented in only ~50 lines of Lisp.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

On 11/23/2021 12:37 PM, Gregory Heytings wrote:
> 
>>> This is not a bug, this is the intented behavior of that feature
>>
>> I started that discussion (and participated throughout it), and I 
>> don't think we actually agreed that this was the intended behavior.
>>
> 
> This is the behavior I intended (and described in the docstring and 
> manual), if you prefer.  And you did not make further comments in 
> bug#51377, which can be interpreted as a kind of agreement.

Unfortunately, I was sidetracked by other things and didn't have a 
chance to comment before Lars merged the patch. Since it had already 
been merged, I thought it best to follow up in a separate bug once I had 
made concise steps to reproduce the issue and a patch to fix it.

>> I should stress that the case I brought up above is just a 
>> counterexample to show a problem with a previous implementation strategy
>>
> 
> Which problem?

Prior to that comment, your proposed implementation would kill Emacs on 
a timer when there were no non-daemon frames left, which could result in 
unsaved changes to files being lost. I replied to point that out and 
showed some steps to reproduce it: 
<https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-10/msg02163.html>.

>> The current behavior on Emacs 29 certainly isn't what I personally 
>> intended when bringing the idea up on emacs-devel.
>>
> 
> Is the current behavior of Emacs 29 with my patch and 
> (server-stop-automatically 'kill-terminal) still not what you want?  If 
> not, what is missing?

If I'm understanding your patch, the behavior I'm looking for is 
essentially a combination of `kill-terminal' and `delete-last-frame'. I 
may be misunderstanding it though, since the call tree in your patch 
confuses me a bit: with `kill-terminal', 
`server-save-buffers-kill-terminal` calls 
`server-stop-automatically--handle-delete-frame', which then calls 
`server-save-buffers-kill-terminal' again.

One of my other goals in my patch was to simplify the logic in 
`server-save-buffers-kill-terminal' and 
`server-stop-automatically--handle-delete-frame' somewhat. Rather than 
to have `server-stop-automatically--handle-delete-frame' check if it was 
called by `save-buffers-kill-terminal', I found that the implementation 
was simpler (to me, anyway) if that logic was lifted up into 
`server-save-buffers-kill-terminal'.

One benefit of this simplification is that it causes fewer changes in 
behavior compared to not using `server-stop-automatically'. For example, 
normally when a user kills an emacsclient terminal, Emacs will prompt 
about saving files *before* deleting any frames. This is nice because it 
allows the user to back out by pressing C-g, leaving Emacs in (almost) 
the same state it was previously. My patch handles that and allows the 
user to press C-g and leave all the current frames open.

With your patch in this bug, using `kill-terminal' and pressing C-x C-c 
will close all frames for the current client but the current one, and 
only then prompt the user to save buffers. Thus, pressing C-g will leave 
the user with only that last client frame still open.

(Note: to test this behavior, you probably need multiple clients open as 
I outlined in the first post to this bug.)

>> I'm concerned that we're now up to 4 different behaviors, when I think 
>> two of them are just the result of a miscommunication between the two 
>> of us.
> 
> They are not, AFAICS.  The four behaviors are four reasonable options, 
> each of which can (and is) described in a short paragraph, and 
> corresponds to a different user preference.  I see no reason to remove 
> any of the current three behaviors because of an unspecified "problem".  
> Especially given that all these behaviors are implemented in only ~50 
> lines of Lisp.

I've specified the problems. I can try to clarify if there's any 
confusion though. This bug is one such problem.

I don't think that a user who opts in to stopping the Emacs daemon 
automatically is *also* opting in to changing the behavior of whether 
Emacs will prompt about saving files when killing a (non-last) client. 
Since there are other clients, the daemon won't be killed, and so the 
behavior should be identical to what happens without 
`server-stop-automatically'. As a user, I would find it very strange 
that enabling `server-stop-automatically' would change Emacs' behavior 
in ways *other than* stopping the server in certain cases.

Of course, a user may indeed want to be able to kill a client (but not 
the daemon) without being prompted to save files, but I think that's 
independent of whether the daemon should be stopped when the last client 
exits. If users *do* want this behavior, we could add a totally separate 
option for it; this would allow users who don't want to be prompted but 
also don't want `server-stop-automatically' to use it.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Gregory Heytings]

>>> I should stress that the case I brought up above is just a 
>>> counterexample to show a problem with a previous implementation 
>>> strategy
>> 
>> Which problem?
>
> Prior to that comment, your proposed implementation would kill Emacs on 
> a timer when there were no non-daemon frames left, which could result in 
> unsaved changes to files being lost. I replied to point that out and 
> showed some steps to reproduce it: 
> <https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-10/msg02163.html>.
>

I don't think it's useful to discuss a problem that was raised and fixed a 
month ago.

>
> If I'm understanding your patch, the behavior I'm looking for is 
> essentially a combination of `kill-terminal' and `delete-last-frame'.
>

If so, a fifth behavior could be implemented.  But you would have to 
describe what you want, and it should be something reasonable.  Could you 
please do that, and explain what you want when:

1. You delete a frame that is not the last one
2. You delete the last frame
3. You kill a terminal that is not the last one
4. You kill a terminal that is the last one

>
> I've specified the problems. I can try to clarify if there's any 
> confusion though. This bug is one such problem.
>

As I said earlier, the problem you described in this bug report was not a 
bug, at least in the sense that it was not something that was not 
explicitly intended by the one who wrote the code (and documented).  And 
the behavior you wanted is handled by the patch I sent, without removing 
any of the other existing behaviors.  But now you apparently want 
something else again.

>
> I don't think that a user who opts in to stopping the Emacs daemon 
> automatically is *also* opting in to changing the behavior of whether 
> Emacs will prompt about saving files when killing a (non-last) client. 
> Since there are other clients, the daemon won't be killed, and so the 
> behavior should be identical to what happens without 
> `server-stop-automatically'. As a user, I would find it very strange 
> that enabling `server-stop-automatically' would change Emacs' behavior 
> in ways *other than* stopping the server in certain cases.
>

Yet this is what you're asking.  If you want Emacs to prompt you whether 
the files should be saved and the process killed when you delete the last 
frame, you want to change the way Emacs prompts about saving files and 
killing processes, because this (namely prompting the user when the last 
frame is deleted) isn't happening without server-stop-automatically.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

Eli, Lars: I'm not sure what either of you would like to do about this 
bug, but it seems that most of the conflict is due to a miscommunication 
between me and Gregory (that's my impression, anyway). I hope my 
previous messages have explained my thoughts on the matter fairly 
thoroughly (if not, just let me know). However, I'm not sure there's 
much more I can add to the discussion beyond this message.

On 11/23/2021 2:49 PM, Gregory Heytings wrote:
> As I said earlier, the problem you described in this bug report was not 
> a bug, at least in the sense that it was not something that was not 
> explicitly intended by the one who wrote the code (and documented).  And 
> the behavior you wanted is handled by the patch I sent, without removing 
> any of the other existing behaviors.  But now you apparently want 
> something else again.

I've only wanted one behavior since I started this discussion on Oct 
19[1]. However, rather than making sure we understand each other's 
descriptions (or consulting the patches I've posted throughout the 
discussion) and have properly identified the corner cases to handle, 
you've instead implemented the behavior "for" me, even though I said 
from the beginning that I was looking to write the patch myself. I never 
posted a rigorous specification of the behavior I wanted for that 
reason: I was soliciting feedback to develop a patch that meets my needs 
(along with the needs of anyone who spoke up at the time, if possible).

The fact that you opted to help by authoring your own patches is 
appreciated, but it ultimately doesn't help me because we seem to be 
talking at cross purposes and your impressions of what I want aren't 
what I actually want. Moreover, if our interpretations *don't* match up 
and I bring up an issue with a proposed patch, that doesn't mean that I 
want an additional option or that I've changed my mind; it just means 
that we haven't reached an understanding yet.

I certainly don't expect you to do any additional work here. I'm 
perfectly happy to provide patches implementing the behavior I have in 
mind, and to adjust them as needed if you or anyone else has feedback on 
them. While I could probably construct a rigorous specification for the 
behavior I want so that someone else (e.g. you) could implement it, that 
would probably end up just being a set of test cases extracted from the 
patch I already have.

As an aside, I mentioned this previously, but I think it would be 
valuable to write some automated test cases to verify that things work 
as expected. However, I didn't see a way to test creating/destroying 
Emacs servers/clients via ERT. I'm certainly open to doing so if someone 
points me in the right direction though.

>> I don't think that a user who opts in to stopping the Emacs daemon 
>> automatically is *also* opting in to changing the behavior of whether 
>> Emacs will prompt about saving files when killing a (non-last) client. 
>> Since there are other clients, the daemon won't be killed, and so the 
>> behavior should be identical to what happens without 
>> `server-stop-automatically'. As a user, I would find it very strange 
>> that enabling `server-stop-automatically' would change Emacs' behavior 
>> in ways *other than* stopping the server in certain cases.
>>
> 
> Yet this is what you're asking.  If you want Emacs to prompt you whether 
> the files should be saved and the process killed when you delete the 
> last frame, you want to change the way Emacs prompts about saving files 
> and killing processes, because this (namely prompting the user when the 
> last frame is deleted) isn't happening without server-stop-automatically.

That's not relevant to the case I'm discussing above. I specifically 
said I'm talking about the behavior when killing the *non-last* client. 
In that case, the server won't be stopped, no matter how 
`server-stop-automatically' is configured.

[1] https://lists.gnu.org/archive/html/emacs-devel/2021-10/msg01465.html

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Gregory Heytings]

>
> I never posted a rigorous specification of the behavior I wanted for 
> that reason: I was soliciting feedback to develop a patch that meets my 
> needs.
>

That's not what you did, you posted a patch claiming that one of the 
existing behaviors had a bug and should be replaced with another one.

>
> you've instead implemented the behavior "for" me
>

That's not what I did, I tried to implement a function that provides 
different possible behaviors corresponding to different needs and 
expectations, not just the behavior you expected.  And I did this based on 
your feedback, what others said on emacs-devel, and my own feelings.  I 
hoped (and thought until today) that the behavior you expected was one of 
them.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

On 11/23/2021 3:59 PM, Gregory Heytings wrote:
>>
>> I never posted a rigorous specification of the behavior I wanted for 
>> that reason: I was soliciting feedback to develop a patch that meets 
>> my needs.
>>
> 
> That's not what you did, you posted a patch claiming that one of the 
> existing behaviors had a bug and should be replaced with another one.

That's in reference to my original message on Oct 19[1], which I cited 
earlier in the paragraph that you excerpted the quote from. At the time, 
I said:

> I didn't see any options to configure this behavior in Emacs, but 
> looking over the code, it shouldn't be that hard for me to write a patch 
> to add this as an option. Before I started though, I wanted to see what 
> others thought. Is this a behavior others would be interested in? If so, 
> are there any other particulars I should take into account in my patch?

--------------------

On 11/23/2021 3:59 PM, Gregory Heytings wrote:
>> you've instead implemented the behavior "for" me
>>
> 
> That's not what I did, I tried to implement a function that provides 
> different possible behaviors corresponding to different needs and 
> expectations, not just the behavior you expected.  And I did this based 
> on your feedback, what others said on emacs-devel, and my own feelings.  
> I hoped (and thought until today) that the behavior you expected was one 
> of them.

And that's fine. I have no issue supporting behaviors other than the one 
I personally want. (For example, I have no problem with the `empty' 
behavior, even though I wouldn't personally find it useful.) However, I 
also want to be sure that we don't provide *unnecessarily* many options 
here, since that adds to the maintenance burden and may produce 
unexpected behavior for users if they don't quite understand the 
distinction between each possible setting.

In this case, I think it's better to continue prompting users when 
killing a non-last client just like Emacs 28[2] does. There might be 
some value in avoiding that prompt, but I think a user who *doesn't* use 
`server-stop-automatically' is just as likely to want that behavior, so 
if we want to support that, we should provide a separate configuration 
option to do so.

In particular, I think it's valuable to prompt users in this case 
because it comes up when using emacsclient and committing code. If I set 
`EDITOR="emacsclient -c"' and run `git commit', then under Emacs 28, I 
can close the client with `C-x C-c'[3] and it will prompt me if I forgot 
to save the commit message. That's useful because without the prompt, it 
would (usually) just abort the commit due to an empty message.

I think that behavior is worth preserving. (Likewise, I think it's worth 
preserving the ability to keep all your open frames by pressing C-g if 
Emacs prompts you about saving files when you try to kill a client.) 
Thus, I implemented the patch that you can see in the original message.

--------------------

In addition, I'm not certain that we need both `delete-frame' (or 
`delete-last-frame' as it's called in your latest patch) and 
`kill-terminal'. The only difference between the two is that in the 
former, individually closing the last non-daemon frame (e.g. by `C-x 5 
0' or clicking the X on the frame's titlebar) also kills the daemon. I 
prefer the behavior of `delete-frame', since my mental model is that the 
daemon should be killed *whenever* the last non-daemon frame is closed, 
no matter *how* it's closed. Maybe someone has a strong opinion in the 
other direction and actively wants the `kill-terminal' behavior instead. 
I don't have a problem with keeping that option around if someone would 
actually use it, but I'm skeptical of that. In this case though, I'm 
happy to present my argument and then defer to the Emacs maintainers to 
make the final call, whatever that may be. If it stays, that's fine with 
me; if it goes, that's fine too.

--------------------

Since, as you mentioned before, we have different mental models for how 
this works, I'm mainly trying to explain through both prose and patches 
how this should work under my mental model. If I understand your prior 
messages correctly, your mental model is best encapsulated by the 
`empty' configuration. I'm 100% happy to keep that around as-is, even 
though it's not my preferred solution. However, my preferred solution is 
"something close to `delete-frame'".

Your patch in bug#51377 gets most of the way to what I wanted, but there 
are a couple of corner cases that don't work like I expect, hence this 
bug. Whether the behavior is a true "bug", a miscommunication, or 
something else doesn't matter much to me. All I'm looking for is 
something along the following lines (note: this isn't 100% rigorous, so 
don't take it as a precise specification):

1) When killing a non-last client (including by closing the last of its 
frames), the behavior should be exactly the same as the default 
emacsclient behavior. (That's what this bug is about.)

2) When killing the last client (including by closing the last of its 
frames), the behavior should be exactly the same as killing Emacs when 
*not* using a server. (Roughly speaking, that means calling 
`save-buffers-kill-emacs'. It would also be nice to have an option to 
silence the "This Emacs session has clients; exit anyway?" prompt, 
though I can certainly understand others wanting to keep it around in 
this case.)

Again, that may not be 100% precise, but it's the mental model I've used 
while implementing patches for this on my end. The specific 
implementation I use has shifted somewhat over time as I find corner 
cases, and also through consulting your patches. Using 
`delete-frame-functions' as in your code greatly simplified my 
implementation, for example. Thanks for that.

Hopefully my delays in following up on bug#51377 haven't been *too* 
disruptive. As mentioned, I was unfortunately too busy to devote 
sufficient time to it back then (my plan when posting to emacs-devel was 
just to collect feedback and to work on the implementation slowly over 
the next several weeks). Now my schedule is a bit less hectic, and I'm 
hoping to address the last few concerns I had after taking the time to 
test things out more thoroughly.

I'm certainly not trying to step on your toes or undo your patch. Just 
to account for certain corner cases that I didn't have a chance to 
investigate (or resolve) at the time.

[1] https://lists.gnu.org/archive/html/emacs-devel/2021-10/msg01465.html
[2] Or without `server-stop-automatically'
[3] You could also use `C-x #' here, but I find `C-x C-c' easier to 
type, so prefer the latter in cases where either would do what I want.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

To help move things along, I've drafted an initial version of a proposal 
describing possible behaviors of `server-stop-automatically' at a high 
level. Hopefully that will make it easier to discuss how each setting 
should work, and we can refine the proposal until we're happy with it. 
Then any implementation changes should "just" be a matter of following 
the final proposal.

I welcome any comments or suggested additions to this proposal. I 
covered things how I see them, but I may not have provided enough detail 
for everyone to understand all the nuances, and other people might have 
their own use cases they'd like to see addressed.

I can also post this proposal to emacs-devel if people think it would be 
better to get a wider range of feedback on it. Once we can get some 
level of consensus on the proposal, then hopefully we'll know the right 
thing to do about this bug.

--------------------

Automatically Shutting Down the Emacs Daemon
Revision 1

* Introduction

Since Emacs 23, `emacsclient' has had the ability to automatically start 
an Emacs daemon by passing `--alternate-editor=""' (or setting the 
ALTERNATE_EDITOR environment variable to the empty string). In Emacs 29, 
the ability to automatically shut down the daemon was added for 
symmetry. This allows users who prefer to start the Emacs daemon on an 
as-needed basis to configure the daemon to stop when finished. However, 
"finished" is a vague term and doesn't precisely describe the conditions 
that would cause the Emacs daemon to be shut down.

Note: for the discussion below, "clients" refers to both "true clients" 
(members of `server-clients') and "pseudo-clients" (each frame with the 
`client' parameter set to `nowait'). This is essentially the logic used 
by `server-save-buffers-kill-terminal' since Emacs 23.

* Proposed Behaviors

I propose two main ways of determining when the Emacs daemon should be 
shut down automatically. Each has different pros and cons, and so there 
are arguments for supporting one, both, or neither in Emacs going 
forward. However, I think each are distinct enough that supporting both 
would be reasonable.

** Implicit Shutdown

For this behavior, the Emacs daemon will be silently stopped once 
nothing would be lost by stopping it. In particular, this means that the 
Emacs daemon should continue so long as there's at least one a) active 
client, b) unsaved file, or c) running subprocess (with non-nil 
`process-query-on-exit-flag'). If none of these exist, then the Emacs 
daemon will be stopped.

One open question is how this should interact with options that control 
how `save-buffers-kill-emacs' prompts the user. If 
`confirm-kill-processes' is nil, should the Emacs daemon then be stopped 
even if there are active subprocesses?

*** Pros

A benefit of this behavior is that because the shutdown occurs 
implicitly, there's no change to how users interact with Emacs. For 
example, `save-buffers-kill-terminal' will prompt the user to save only 
those files passed to `emacsclient'.

*** Cons

However, the behavior's benefit has another side to it: it can be 
difficult for a user to predict whether the Emacs daemon will be 
stopped. For example, a stray unsaved file that the user forgot about 
could be the difference between the daemon continuing to run or being 
stopped.

Furthermore, since an Emacs daemon with no clients is out of sight, it 
may be some time before the user realizes their mistake, leading to 
confusion about why some other program appears to be reading an 
out-of-date version of the file. That said, this aspect is equally a 
concern when *not* automatically stopping the Emacs daemon at all.

** Explicit Shutdown

(Note: this is the behavior I personally prefer.) In this case, Emacs 
will be stopped as soon as there are no active clients, prompting the 
user to save their work as needed. In other words, when a client is 
about to be deleted for any reason (calling 
`server-save-buffers-kill-terminal', deleting the last frame associated 
with the client, etc), then:

- If this is not the last client, behave as in Emacs 28; that is, prompt 
to save any files passed to `emacsclient' before deleting the client.

- If this is the last client, prompt the user to save all their work 
before killing Emacs entirely; that is, call `save-buffers-kill-emacs'. 
In this case, the behavior should be identical (or as close as possible) 
to killing a "regular", non-server Emacs in a similar state.

One open question is: should Emacs warn the user that it will the daemon 
when deleting the last client? Currently it does so in 
`server-kill-emacs-query-function', asking, "This Emacs session has 
clients; exit anyway?" It may be useful to keep this warning, possibly 
rewording it in this case to explain the situation better. It's also 
possible that it's just extra noise and should be eliminate in this 
case. (It may even make sense to provide a separate configuration option.)

*** Pros

One of the main goals with Explicit Shutdown is to resolve a concern 
mentioned above: an Emacs daemon with no clients is out of sight, so 
it's easy to forget about it. If the daemon is holding onto unsaved 
work, it may take a while until the user realizes this. By prompting to 
save *all* the user's work before closing the last client, it's much 
harder to make this mistake.

*** Cons

This behavior makes it more difficult for a user to know ahead of time 
whether `save-buffers-kill-terminal' (bound by default to `C-x C-c') 
will kill the client or Emacs entirely. The prompt mentioned above in 
`server-kill-emacs-query-function' does let the user bail out though, if 
they didn't mean to kill Emacs entirely.

However, `save-buffers-kill-terminal' already has this complexity 
(almost), since it will kill Emacs entirely when executed from a 
non-client frame. That said, it's possible to tell what will happen in 
this case by looking at the mode line: if it starts with something like 
"U:@---", it's a client frame and thus an observant user will know what 
`save-buffers-kill-terminal' will do.

* Delayed Shutdown

It may also be worth considering whether the Emacs daemon should be 
stopped immediately when the conditions are met, or whether it would be 
better to delay it by a short time. Some operations, such as `git rebase 
-i', can open the $EDITOR, close it, and then reopen it in rapid 
succession. In a case like that, it would be more efficient if the Emacs 
daemon weren't stopped right away.

That said, I don't think this is a critical problem, and believe it 
would be ok to decide what to do about this later.

* Current Behaviors

Currently, Emacs 29 supports Implicit Shutdown (called `empty') and has 
two slightly-different variations on Explicit Shutdown (called 
`kill-terminal' and `delete-frame'). `empty' is implemented as a timer 
that periodically calls `server-stop-automatically--maybe-kill-emacs' to 
check if there's any need to keep the daemon running (non-daemon frames, 
unsaved files, or running processes); if not, it stops the daemon. This 
differs very slightly from the proposed spec above, since it checks for 
frames, not active clients. Some clients (e.g. `emacsclient --eval FOO') 
don't create any frames, so it may be useful to enhance the current 
implementation of Implicit Shutdown to account for this.

`kill-terminal' is implemented in 
`server-stop-automatically--handle-delete-frame' (called by 
`server-save-buffers-kill-terminal'). It first deletes all frames 
associated with the current client *except* the current one. Then, if 
there are any non-daemon frames aside from the current one, it just 
deletes that frame; if it's the last non-daemon frame, it calls 
`save-buffers-kill-emacs'.

`delete-frame' works like `kill-terminal' above, but will also call 
`save-buffers-kill-emacs' when closing the last non-daemon frame using 
other means, such as clicking the "X" in the frame's title bar on a GUI 
system.

As mentioned above, `kill-terminal' and `delete-frame' work similarly to 
the Explicit Shutdown behavior, but there are some differences. For 
example, when killing a non-last client, `kill-terminal' and 
`delete-frame' don't prompt to save files passed to `emacsclient'. When 
killing the last client, they delete all the non-current frames before 
calling `save-buffers-kill-emacs', meaning that pressing `C-g' to cancel 
when prompted will still result in all but one frame going away.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Eli Zaretskii]

> From: Jim Porter <jporterbugs@gmail.com>
> Cc: eliz@gnu.org, larsi@gnus.org, gregory@heytings.org
> Date: Sun, 28 Nov 2021 21:39:46 -0800
> 
> To help move things along, I've drafted an initial version of a proposal 
> describing possible behaviors of `server-stop-automatically' at a high 
> level. Hopefully that will make it easier to discuss how each setting 
> should work, and we can refine the proposal until we're happy with it. 
> Then any implementation changes should "just" be a matter of following 
> the final proposal.
> 
> I welcome any comments or suggested additions to this proposal. I 
> covered things how I see them, but I may not have provided enough detail 
> for everyone to understand all the nuances, and other people might have 
> their own use cases they'd like to see addressed.
> 
> I can also post this proposal to emacs-devel if people think it would be 
> better to get a wider range of feedback on it. Once we can get some 
> level of consensus on the proposal, then hopefully we'll know the right 
> thing to do about this bug.

Thanks.

However, I'm not sure I understand the way forward, as you see it.
AFAIU, there are fundamental disagreements between you and Gregory,
and I don't think the disagreements are there because one of you
doesn't understand the proposals of the other.

So how could these disagreements be reconciled?  Either you-two come
up with some compromise that is acceptable by both of you, or Lars and
myself make the decision for you (and we don't guarantee you will like
it), or things are left as they are now.  What will it be?

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Gregory Heytings]

>
> However, I'm not sure I understand the way forward, as you see it. 
> AFAIU, there are fundamental disagreements between you and Gregory, and 
> I don't think the disagreements are there because one of you doesn't 
> understand the proposals of the other.
>
> So how could these disagreements be reconciled?
>

I'm in the process of implementing something that should satisfy everyone, 
but it's not finished yet.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

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

On 11/29/2021 5:40 AM, Gregory Heytings wrote:
> 
> I'm in the process of implementing something that should satisfy 
> everyone, but it's not finished yet.

Thanks. Hopefully my proposal helps to explain more precisely my mental 
model and how I'd like things to work. If there's anything that's not 
clear, just let me know and I'll try to elaborate on it.

In case it would help, I've also attached the relevant bits of my Emacs 
configuration, which implements the Explicit Shutdown behavior I've 
described. Overall, it's similar to the first patch I posted in this 
bug, but it's usable in Emacs 27+ (maybe even earlier).

However, I think it has (at least) one problem: if `server-buffer-done' 
(indirectly called by `server-edit' / `C-x #') deletes the last client, 
it doesn't stop the daemon. I haven't had a chance to look into this in 
detail yet though. I think the most consistent behavior would be to stop 
the daemon in this case, since then the logic is simply "stop the daemon 
when the last client would be deleted", and we don't have to worry about 
exceptions to that rule.

[-- Attachment #2: server-advice.el --]
[-- Type: text/plain, Size: 3840 bytes --]

(use-package server
  :defer t
  :preface
  (defun user--server-save-buffers-kill-terminal (arg)
    "Offer to save each buffer, then kill the current client.
With ARG non-nil, silently save all file-visiting buffers, then kill.

If emacsclient was started with a list of filenames to edit, then
only these files will be asked to be saved.

If Emacs was started as a daemon and this is the last client
connected to it, this will call `save-buffers-kill-emacs'."
    (let ((proc (frame-parameter nil 'client)))
      (cond ((eq proc 'nowait)
	     ;; Nowait frames have no client buffer list.
	     (if (if (daemonp) (cddr (frame-list)) (cdr (frame-list)))
                 ;; If there's another (non-daemon) frame, only delete this
                 ;; frame. FIXME: It would be nice to delete any other frames
                 ;; created by this frame (as when killing the terminal of an
                 ;; ordinary client below), but we can't distinguish separate
                 ;; groups of nowait frames currently.
	         (progn (save-some-buffers arg)
		        (delete-frame))
	       ;; If we're the last frame standing, kill Emacs.
	       (save-buffers-kill-emacs arg)))
	    ((processp proc)
             (if (seq-some
                  (lambda (frame)
                    (let ((p (frame-parameter frame 'client)))
                      (unless (and (daemonp) (null p))
                        (not (equal proc p)))))
                  (frame-list))
                 ;; If there's a frame not from this client, only delete the
                 ;; client.
	         (let ((buffers (process-get proc 'buffers)))
	           (save-some-buffers
	            arg (if buffers
                            ;; Only files from emacsclient file list.
		            (lambda () (memq (current-buffer) buffers))
                          ;; No emacsclient file list: don't override
                          ;; `save-some-buffers-default-predicate' (unless
                          ;; ARG is non-nil), since we're not killing
                          ;; Emacs (unlike `save-buffers-kill-emacs').
		          (and arg t)))
	           (server-delete-client proc))
               ;; If all frames are from this client, kill Emacs.
               (save-buffers-kill-emacs arg)))
	    (t (error "Invalid client frame")))))

  (defun user--server-kill-emacs-query-function ()
    "Ask before exiting Emacs if it has live clients.
If Emacs was started as a daemon and the only live client is the
current frame's client, don't bother asking."
    (let ((ignored-proc (and (daemonp) (frame-parameter nil 'client))))
      (or (not (seq-some (lambda (proc)
                           (unless (eq ignored-proc proc)
                             (seq-some #'buffer-live-p
                                       (process-get proc 'buffers))))
                         server-clients))
          (yes-or-no-p "This Emacs session has clients; exit anyway? "))))

  (defun user--handle-delete-frame (frame)
    "When deleting the last non-daemon frame, kill Emacs.
\(To be used from `delete-frame-functions'.)"
    (when (and (daemonp)
               ;; Check that the frame is a client frame.
               ;; Note: `server-delete-client' sets `client' to nil before
               ;; calling `delete-frame', but that's good, since we want to call
               ;; `save-buffers-kill-emacs' before all that anyway.
               (frame-parameter frame 'client)
               (null (cddr (frame-list))))
      (save-buffers-kill-emacs)))

  :config
  (advice-add #'server-save-buffers-kill-terminal :override
              #'user--server-save-buffers-kill-terminal)
  (advice-add #'server-kill-emacs-query-function :override
              #'user--server-kill-emacs-query-function)
  (add-hook 'delete-frame-functions #'user--handle-delete-frame))

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

On 11/29/2021 11:31 AM, Jim Porter wrote:
> On 11/29/2021 5:40 AM, Gregory Heytings wrote:
>>
>> I'm in the process of implementing something that should satisfy 
>> everyone, but it's not finished yet.
> 
> Thanks. Hopefully my proposal helps to explain more precisely my mental 
> model and how I'd like things to work. If there's anything that's not 
> clear, just let me know and I'll try to elaborate on it.

Just checking to see if there have been any updates here. As before, I'm 
happy to assist wherever needed, be it writing patches or just providing 
feedback. Of course, no worries if you're busy with other things; I 
don't think there's any urgent need to merge a fix.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Lars Ingebrigtsen]

Gregory Heytings <gregory@heytings.org> writes:

>> However, I'm not sure I understand the way forward, as you see
>> it. AFAIU, there are fundamental disagreements between you and
>> Gregory, and I don't think the disagreements are there because one
>> of you doesn't understand the proposals of the other.
>>
>> So how could these disagreements be reconciled?

> I'm in the process of implementing something that should satisfy
> everyone, but it's not finished yet.

This was half a year ago, and I'm not sure whether anything further was
done here?

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

On 9/9/2022 10:55 AM, Lars Ingebrigtsen wrote:
> Gregory Heytings <gregory@heytings.org> writes:
> 
>>> However, I'm not sure I understand the way forward, as you see
>>> it. AFAIU, there are fundamental disagreements between you and
>>> Gregory, and I don't think the disagreements are there because one
>>> of you doesn't understand the proposals of the other.
>>>
>>> So how could these disagreements be reconciled?
> 
>> I'm in the process of implementing something that should satisfy
>> everyone, but it's not finished yet.
> 
> This was half a year ago, and I'm not sure whether anything further was
> done here?

For what it's worth, I've been brainstorming some ideas for how to 
abstract things out a bit further to make it easy for users to tune the 
behavior just to their liking. I haven't gotten very far on that yet, 
though.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

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

On 9/9/2022 11:04 AM, Jim Porter wrote:
> On 9/9/2022 10:55 AM, Lars Ingebrigtsen wrote:
>> Gregory Heytings <gregory@heytings.org> writes:
>>
>>>> However, I'm not sure I understand the way forward, as you see
>>>> it. AFAIU, there are fundamental disagreements between you and
>>>> Gregory, and I don't think the disagreements are there because one
>>>> of you doesn't understand the proposals of the other.
>>>>
>>>> So how could these disagreements be reconciled?
>>
>>> I'm in the process of implementing something that should satisfy
>>> everyone, but it's not finished yet.
>>
>> This was half a year ago, and I'm not sure whether anything further was
>> done here?
> 
> For what it's worth, I've been brainstorming some ideas for how to 
> abstract things out a bit further to make it easy for users to tune the 
> behavior just to their liking. I haven't gotten very far on that yet, 
> though.

Attached is a very WIP patch that hopefully gets across the idea I had. 
Hopefully the explanation below is sufficiently-clear; if anyone has 
questions, or if I missed some detail, let me know.

(Note: the first patch is just a bugfix so that 'server-delete-client' 
doesn't get called when Emacs *creates* the client.)

The patch adds two new hooks: 'server-before-delete-client-functions' 
and 'server-after-delete-client-functions'. These should give people the 
ability to add whatever behaviors they think make sense when closing an 
emacsclient connection. The default behavior is the same as the current 
default (call 'save-some-buffers' before deleting the client, and do 
nothing after). To do something like the 'delete-frame' configuration of 
the current 'server-stop-automatically' API, you might do something like 
this:

   (add-hook
    'server-before-delete-client-functions
    (lambda (proc arg)
      (when (>= 1 (seq-count
                   (lambda (frame)
                     "Return non-nil if FRAME's client is another process."
                     (not (equal (frame-parameter frame 'client) proc)))
                   (frame-list)))
        ;; If there are no non-daemon frames other than ones owned by
        ;; this client, we want to do all the prompting that
        ;; 'save-buffers-kill-emacs' does, but without really killing.
        ;; HACK ALERT: This is a pretty clumsy way to do this...
        (cl-letf (((symbol-function 'kill-emacs) #'ignore))
          (save-buffers-kill-emacs arg))
        ;; Stop running other hooks.
        t)))

   (add-hook 'server-after-delete-client-functions
             (lambda (proc)
               ;; Kill emacs if the only frame left is the daemon frame.
               (unless (cdr (frame-list))
                 (kill-emacs))))

(Some helper functions might make these hooks simpler, of course.)

This interface has some other potentially-interesting uses. For example, 
the after-deletion hook could start a timer and only kill Emacs if no 
new clients connected after N seconds[1]. That would be useful in cases 
like `git rebase -i` where Git might run your $EDITOR multiple times in 
a row. It would be a waste of CPU to shut down and restart the Emacs 
daemon every time.

One thing I'm not sure about yet: should the 'empty' setting for 
'server-stop-automatically' just become a hook on 
'server-after-delete-client-functions'? It's not *quite* the same, since 
the 'empty' setting will also leave the daemon running until any active 
subprocesses finish too. Maybe it would make sense for 
'server-stop-automatically' to stick around, but just be a boolean (when 
non-nil, use the current 'empty' behavior)? Then we keep all the nice 
features of the 'empty' setting, and the other behaviors can be 
implemented via these new hooks.

[1] This might need an additional hook like 
'server-after-create-client-functions'.

[-- Attachment #2: 0001-Only-delete-server-client-from-the-sentinel-when-the.patch --]
[-- Type: text/plain, Size: 2429 bytes --]

From 620d3df4c016dfaf8c1d764a0516a73638632280 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 8 Oct 2022 19:40:16 -0700
Subject: [PATCH 1/2] Only delete server client from the sentinel when the
 client was closed

* lisp/server.el (server-sentinel): Only call 'server-delete-client'
when closed.
---
 lisp/server.el | 28 +++++++++++++++-------------
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 3caa335c4e..5034a8e8c6 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -408,21 +408,23 @@ server-sentinel
   (when (and (eq (process-status proc) 'open)
 	     (process-query-on-exit-flag proc))
     (set-process-query-on-exit-flag proc nil))
-  ;; Delete the associated connection file, if applicable.
-  ;; Although there's no 100% guarantee that the file is owned by the
-  ;; running Emacs instance, server-start uses server-running-p to check
-  ;; for possible servers before doing anything, so it *should* be ours.
-  (and (process-contact proc :server)
-       (eq (process-status proc) 'closed)
-       ;; If this variable is non-nil, the socket was passed in to
-       ;; Emacs, and not created by Emacs itself (for instance,
-       ;; created by systemd).  In that case, don't delete the socket.
-       (not internal--daemon-sockname)
-       (ignore-errors
-	 (delete-file (process-get proc :server-file))))
   (server-log (format "Status changed to %s: %s"
                       (process-status proc) msg) proc)
-  (server-delete-client proc))
+  (when (eq (process-status proc) 'closed)
+    ;; Delete the associated connection file, if applicable.
+    ;; Although there's no 100% guarantee that the file is owned by
+    ;; the running Emacs instance, server-start uses server-running-p
+    ;; to check for possible servers before doing anything, so it
+    ;; *should* be ours.
+    (and (process-contact proc :server)
+         ;; If this variable is non-nil, the socket was passed in to
+         ;; Emacs, and not created by Emacs itself (for instance,
+         ;; created by systemd).  In that case, don't delete the
+         ;; socket.
+         (not internal--daemon-sockname)
+         (ignore-errors
+           (delete-file (process-get proc :server-file))))
+    (server-delete-client proc)))
 
 (defun server--on-display-p (frame display)
   (and (equal (frame-parameter frame 'display) display)
-- 
2.25.1


[-- Attachment #3: 0002-WIP-Add-hooks-before-after-deleting-Emacs-clients.patch --]
[-- Type: text/plain, Size: 5292 bytes --]

From 7b2563acc064586b674f68abd4eebd11b568e76b Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 9 Oct 2022 14:50:06 -0700
Subject: [PATCH 2/2] [WIP] Add hooks before/after deleting Emacs clients

* lisp/server.el (server-before-delete-client-functions)
(server-after-delete-client-functions): New defcustoms.
(server-delete-client): New argument ARG.  Handle 'nowait' value for
PROC.  Run the above hooks.
(server-save-buffers-kill-terminal): Extract buffer-saving code from
here...
(server-save-some-buffers): ... to this new function.
---
 lisp/server.el | 78 +++++++++++++++++++++++++++++++++-----------------
 1 file changed, 51 insertions(+), 27 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 5034a8e8c6..c1a64d6f32 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -191,6 +191,18 @@ server-done-hook
   "Hook run when done editing a buffer for the Emacs server."
   :type 'hook)
 
+;;;###autoload
+(defcustom server-before-delete-client-functions '(server-save-some-buffers)
+  "Functions to run before deleting an Emacs client.
+The functions are run with two arguments: the client process
+being deleted and an arg indicating whether to silently save."
+  :type 'hook)
+
+(defcustom server-after-delete-client-functions nil
+  "Functions to run after deleting an Emacs client.
+Functions are run with one argument: the client process just deleted."
+  :type 'hook)
+
 (defvar server-process nil
   "The current server process.")
 
@@ -318,13 +330,25 @@ server-with-environment
                  process-environment)))
        (progn ,@body))))
 
-(defun server-delete-client (proc &optional noframe)
+(defun server-delete-client (proc &optional noframe arg)
   "Delete PROC, including its buffers, terminals and frames.
 If NOFRAME is non-nil, let the frames live.
 Updates `server-clients'."
   (server-log (concat "server-delete-client" (if noframe " noframe")) proc)
-  ;; Force a new lookup of client (prevents infinite recursion).
-  (when (memq proc server-clients)
+  (cond
+   ((eq proc 'nowait)
+    (run-hook-with-args-until-success
+     'server-before-delete-client-functions proc arg)
+    (if (cdr (frame-list))
+	(delete-frame)
+      ;; If we're the last frame standing, kill Emacs.
+      (kill-emacs arg))
+    (run-hook-with-args 'server-after-delete-client-functions proc))
+
+   ;; Force a new lookup of client (prevents infinite recursion).
+   ((memq proc server-clients)
+    (run-hook-with-args-until-success
+     'server-before-delete-client-functions proc arg)
     (let ((buffers (process-get proc 'buffers)))
 
       ;; Kill the client's buffers.
@@ -373,7 +397,8 @@ server-delete-client
       (if (eq (process-status proc) 'open)
 	  (delete-process proc))
 
-      (server-log "Deleted" proc))))
+      (server-log "Deleted" proc)
+      (run-hook-with-args 'server-after-delete-client-functions proc)))))
 
 (defvar server-log-time-function #'current-time-string
   "Function to generate timestamps for `server-buffer'.")
@@ -1738,29 +1763,28 @@ server-save-buffers-kill-terminal
 
 If emacsclient was started with a list of filenames to edit, then
 only these files will be asked to be saved."
-  (if server-stop-automatically
-      (server-stop-automatically--handle-delete-frame (selected-frame))
-    (let ((proc (frame-parameter nil 'client)))
-      (cond ((eq proc 'nowait)
-	     ;; Nowait frames have no client buffer list.
-	     (if (cdr (frame-list))
-	         (progn (save-some-buffers arg)
-		        (delete-frame))
-	       ;; If we're the last frame standing, kill Emacs.
-	       (save-buffers-kill-emacs arg)))
-	    ((processp proc)
-	     (let ((buffers (process-get proc 'buffers)))
-	       (save-some-buffers
-	        arg (if buffers
-                        ;; Only files from emacsclient file list.
-		        (lambda () (memq (current-buffer) buffers))
-                      ;; No emacsclient file list: don't override
-                      ;; `save-some-buffers-default-predicate' (unless
-                      ;; ARG is non-nil), since we're not killing
-                      ;; Emacs (unlike `save-buffers-kill-emacs').
-		      (and arg t)))
-	       (server-delete-client proc)))
-	    (t (error "Invalid client frame"))))))
+  (let ((proc (frame-parameter nil 'client)))
+    (unless (or (eq proc 'nowait)
+                (processp proc))
+      (error "Invalid client frame"))
+    (server-delete-client proc nil arg)))
+
+(defun server-save-some-buffers (proc arg)
+  "Save buffers associated with PROC.
+A non-nil ARG means save all without questions (see
+`save-some-buffers')."
+  (if (eq proc 'nowait)
+      (save-some-buffers arg)
+    (let ((buffers (process-get proc 'buffers)))
+      (save-some-buffers
+       arg (if buffers
+               ;; Only files from emacsclient file list.
+	       (lambda () (memq (current-buffer) buffers))
+             ;; No emacsclient file list: don't override
+             ;; `save-some-buffers-default-predicate' (unless ARG is
+             ;; non-nil), since we're not killing Emacs (unlike
+             ;; `save-buffers-kill-emacs').
+	     (and arg t))))))
 
 (defun server-stop-automatically--handle-delete-frame (frame)
   "Handle deletion of FRAME when `server-stop-automatically' is used."
-- 
2.25.1

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Eli Zaretskii]

> Date: Sun, 9 Oct 2022 15:09:15 -0700
> From: Jim Porter <jporterbugs@gmail.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, 51993@debbugs.gnu.org
> 
> The patch adds two new hooks: 'server-before-delete-client-functions' 
> and 'server-after-delete-client-functions'. These should give people the 
> ability to add whatever behaviors they think make sense when closing an 
> emacsclient connection. The default behavior is the same as the current 
> default (call 'save-some-buffers' before deleting the client, and do 
> nothing after). To do something like the 'delete-frame' configuration of 
> the current 'server-stop-automatically' API, you might do something like 
> this:

Isn't this a bit of over-engineering for such a simple problem?  Why
couldn't we have a user option to decide what to do, and then just do
it?  The place where we delete client frames is well determined, so
doing something sensible there should be easy.

Hooks make sense when some Lisp program needs to turn on or off some
aspects of Emacs behavior, not when a user needs to control that
behavior.  For users, controlling behavior with hooks should be
reserved to relatively complex and/or obscure aspects of the behavior.

Thanks.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

On 10/9/2022 11:04 PM, Eli Zaretskii wrote:
>> Date: Sun, 9 Oct 2022 15:09:15 -0700
>> From: Jim Porter <jporterbugs@gmail.com>
>> Cc: Eli Zaretskii <eliz@gnu.org>, 51993@debbugs.gnu.org
>>
>> The patch adds two new hooks: 'server-before-delete-client-functions'
>> and 'server-after-delete-client-functions'. These should give people the
>> ability to add whatever behaviors they think make sense when closing an
>> emacsclient connection. The default behavior is the same as the current
>> default (call 'save-some-buffers' before deleting the client, and do
>> nothing after). To do something like the 'delete-frame' configuration of
>> the current 'server-stop-automatically' API, you might do something like
>> this:
> 
> Isn't this a bit of over-engineering for such a simple problem?  Why
> couldn't we have a user option to decide what to do, and then just do
> it?  The place where we delete client frames is well determined, so
> doing something sensible there should be easy.

I'd be happy with a simple user option, provided we can all agree how 
things should work (or if we can't *all* agree, that you and Lars can 
decide at least). I thought adding a hook might get around the problem 
of not being able to agree, though.

I proposed a couple of behaviors that I described in as much detail as I 
could in the hopes of avoiding confusion and coming to an agreement 
here: 
<https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-11/msg02245.html>.

To summarize it briefly, the behavior I would personally prefer is this. 
When deleting an emacs client by any means (e.g. 'C-x C-c', clicking the 
X on the last frame of a client, etc):

   a) if this is not the last client, behave the same as Emacs 28: 
prompt to save files specified when starting "emacsclient", and then 
delete that client.

   b) if this *is* the last client, prompt the user to save everything 
(as with 'save-buffers-kill-emacs'), and then delete the client + kill 
the Emacs daemon.

I'm certainly open to supporting other options if people have different 
preferences, and I can work on a patch to support these so long as we 
come to an agreement/decision/compromise about the expected behavior(s).

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Eli Zaretskii]

> Date: Wed, 19 Oct 2022 20:14:38 -0700
> Cc: larsi@gnus.org, 51993@debbugs.gnu.org, gregory@heytings.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> I proposed a couple of behaviors that I described in as much detail as I 
> could in the hopes of avoiding confusion and coming to an agreement 
> here: 
> <https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-11/msg02245.html>.
> 
> To summarize it briefly, the behavior I would personally prefer is this. 
> When deleting an emacs client by any means (e.g. 'C-x C-c', clicking the 
> X on the last frame of a client, etc):
> 
>    a) if this is not the last client, behave the same as Emacs 28: 
> prompt to save files specified when starting "emacsclient", and then 
> delete that client.
> 
>    b) if this *is* the last client, prompt the user to save everything 
> (as with 'save-buffers-kill-emacs'), and then delete the client + kill 
> the Emacs daemon.

You mean, in b), instead of just deleting the frame and leaving the
daemon run, you want to shut down Emacs in its entirety, as if the
user invoked kill-emacs?  I'm okay with that as an optional behavior,
although I myself won't use it, as it's too dangerous.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

On 10/19/2022 11:23 PM, Eli Zaretskii wrote:
>> Date: Wed, 19 Oct 2022 20:14:38 -0700
>> Cc: larsi@gnus.org, 51993@debbugs.gnu.org, gregory@heytings.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>>     a) if this is not the last client, behave the same as Emacs 28:
>> prompt to save files specified when starting "emacsclient", and then
>> delete that client.
>>
>>     b) if this *is* the last client, prompt the user to save everything
>> (as with 'save-buffers-kill-emacs'), and then delete the client + kill
>> the Emacs daemon.
> 
> You mean, in b), instead of just deleting the frame and leaving the
> daemon run, you want to shut down Emacs in its entirety, as if the
> user invoked kill-emacs?  I'm okay with that as an optional behavior,
> although I myself won't use it, as it's too dangerous.

Almost. I'd like it to be as if the user invoked 
'save-buffers-kill-emacs'; that is, before killing Emacs, prompt the 
user about everything[1] that might be lost by killing Emacs.

This already exists as an option -- (server-stop-automatically 
'delete-frame)[2], but I also find the current behavior too dangerous. 
My original message outlines one of the problems with the current 
implementation: it changes the behavior of (a) in my description above.

>   $ emacs -Q --daemon
>   $ emacsclient -a "" -c foo.txt
>   $ emacsclient -a "" -c bar.txt
> 
>   ;; In the first client frame:
>   foobar ;; Insert some text
>   C-x C-c
>   ;; Emacs prompts "Save file /path/to/foo.txt?..."
> 
> Now try the above, but call `(server-stop-automatically 'delete-frame)' first (or replace `delete-frame' with `kill-terminal'; it doesn't matter). In this case, Emacs doesn't prompt to save the file.

As mentioned earlier in this bug, I'm open to providing a patch to 
support options beyond the one I suggested (quoted at the beginning of 
this message), but I want to be sure we agree on what the options should 
look like so that I'm not breaking Gregory's (or anyone else's) 
preferred behavior here.

(Of course, all of this would remain opt-in. I don't want to change the 
default behavior here, since it would break years of established precedent.)

[1] Well, everything that Emacs usually asks about in a non-daemon 
configuration, anyway.

[2] It's not hooked up to Customize, though I don't think that would be 
too hard to do.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Eli Zaretskii]

> Date: Thu, 20 Oct 2022 22:51:42 -0700
> Cc: larsi@gnus.org, 51993@debbugs.gnu.org, gregory@heytings.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> >>     b) if this *is* the last client, prompt the user to save everything
> >> (as with 'save-buffers-kill-emacs'), and then delete the client + kill
> >> the Emacs daemon.
> > 
> > You mean, in b), instead of just deleting the frame and leaving the
> > daemon run, you want to shut down Emacs in its entirety, as if the
> > user invoked kill-emacs?  I'm okay with that as an optional behavior,
> > although I myself won't use it, as it's too dangerous.
> 
> Almost. I'd like it to be as if the user invoked 
> 'save-buffers-kill-emacs'; that is, before killing Emacs, prompt the 
> user about everything[1] that might be lost by killing Emacs.

That should already happen, if you just call save-buffers-kill-emacs
in that case, right?

> This already exists as an option -- (server-stop-automatically 
> 'delete-frame)[2], but I also find the current behavior too dangerous. 
> My original message outlines one of the problems with the current 
> implementation: it changes the behavior of (a) in my description above.
> 
> >   $ emacs -Q --daemon
> >   $ emacsclient -a "" -c foo.txt
> >   $ emacsclient -a "" -c bar.txt
> > 
> >   ;; In the first client frame:
> >   foobar ;; Insert some text
> >   C-x C-c
> >   ;; Emacs prompts "Save file /path/to/foo.txt?..."
> > 
> > Now try the above, but call `(server-stop-automatically 'delete-frame)' first (or replace `delete-frame' with `kill-terminal'; it doesn't matter). In this case, Emacs doesn't prompt to save the file.

I'm not sure I see the direct relevance, and I don't think I see a bug
in the above behavior.  I'm probably missing something, but what?

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

On 10/20/2022 11:38 PM, Eli Zaretskii wrote:
>> Date: Thu, 20 Oct 2022 22:51:42 -0700
>> Cc: larsi@gnus.org, 51993@debbugs.gnu.org, gregory@heytings.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>>>>      b) if this *is* the last client, prompt the user to save everything
>>>> (as with 'save-buffers-kill-emacs'), and then delete the client + kill
>>>> the Emacs daemon.
>>>
>>> You mean, in b), instead of just deleting the frame and leaving the
>>> daemon run, you want to shut down Emacs in its entirety, as if the
>>> user invoked kill-emacs?  I'm okay with that as an optional behavior,
>>> although I myself won't use it, as it's too dangerous.
>>
>> Almost. I'd like it to be as if the user invoked
>> 'save-buffers-kill-emacs'; that is, before killing Emacs, prompt the
>> user about everything[1] that might be lost by killing Emacs.
> 
> That should already happen, if you just call save-buffers-kill-emacs
> in that case, right?

Yeah. My expectation is that I can type 'C-x C-c' (or 'M-x 
save-buffers-kill-terminal') to kill a client, and if it's the last 
client, instead kill Emacs entirely (like 'M-x 
save-buffers-kill-emacs'). So roughly speaking, the change would be that 
you can set 'save-buffer-kill-terminal' to work like 
'save-buffer-kill-emacs' when there's only 1 client left.

>> This already exists as an option -- (server-stop-automatically
>> 'delete-frame)[2], but I also find the current behavior too dangerous.
>> My original message outlines one of the problems with the current
>> implementation: it changes the behavior of (a) in my description above.
>>
>>>    $ emacs -Q --daemon
>>>    $ emacsclient -a "" -c foo.txt
>>>    $ emacsclient -a "" -c bar.txt
>>>
>>>    ;; In the first client frame:
>>>    foobar ;; Insert some text
>>>    C-x C-c
>>>    ;; Emacs prompts "Save file /path/to/foo.txt?..."
>>>
>>> Now try the above, but call `(server-stop-automatically 'delete-frame)' first (or replace `delete-frame' with `kill-terminal'; it doesn't matter). In this case, Emacs doesn't prompt to save the file.
> 
> I'm not sure I see the direct relevance, and I don't think I see a bug
> in the above behavior.  I'm probably missing something, but what?

The issue in the quote above is that if you enable automatic server 
shutdown in Emacs 29, it changes the behavior of exiting an emacsclient 
even when it wouldn't stop the server (i.e. when there are other active 
clients). That's surprising to me, I wouldn't expect that setting to 
affect cases when it decides *not* to kill the Emacs daemon.

(This is relevant to the previous discussion since fixing this would get 
Emacs's automatic server shutdown - aka killing the daemon - close to 
the way I described there. Gregory mentioned[1] that the current 
behavior is intended, although we've had some difficulty coming to an 
agreement on how all this should work. Hence why I thought a hook might 
help here: if there are strong opinions in various directions, maybe a 
simple option isn't enough. Or maybe this is a case where you and/or 
Lars would be in a better position to make a final decision...)

[1] https://debbugs.gnu.org/cgi/bugreport.cgi?bug=51993

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Eli Zaretskii]

> Date: Fri, 21 Oct 2022 20:46:47 -0700
> Cc: larsi@gnus.org, 51993@debbugs.gnu.org, gregory@heytings.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> >>>    $ emacs -Q --daemon
> >>>    $ emacsclient -a "" -c foo.txt
> >>>    $ emacsclient -a "" -c bar.txt
> >>>
> >>>    ;; In the first client frame:
> >>>    foobar ;; Insert some text
> >>>    C-x C-c
> >>>    ;; Emacs prompts "Save file /path/to/foo.txt?..."
> >>>
> >>> Now try the above, but call `(server-stop-automatically 'delete-frame)' first (or replace `delete-frame' with `kill-terminal'; it doesn't matter). In this case, Emacs doesn't prompt to save the file.
> > 
> > I'm not sure I see the direct relevance, and I don't think I see a bug
> > in the above behavior.  I'm probably missing something, but what?
> 
> The issue in the quote above is that if you enable automatic server 
> shutdown in Emacs 29, it changes the behavior of exiting an emacsclient 
> even when it wouldn't stop the server (i.e. when there are other active 
> clients). That's surprising to me, I wouldn't expect that setting to 
> affect cases when it decides *not* to kill the Emacs daemon.

Sounds like a bug to me, because it contradicts what the doc string
says.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

On 10/21/2022 11:57 PM, Eli Zaretskii wrote:
>> Date: Fri, 21 Oct 2022 20:46:47 -0700
>> Cc: larsi@gnus.org, 51993@debbugs.gnu.org, gregory@heytings.org
>> From: Jim Porter <jporterbugs@gmail.com>
>>
>> The issue in the quote above is that if you enable automatic server
>> shutdown in Emacs 29, it changes the behavior of exiting an emacsclient
>> even when it wouldn't stop the server (i.e. when there are other active
>> clients). That's surprising to me, I wouldn't expect that setting to
>> affect cases when it decides *not* to kill the Emacs daemon.
> 
> Sounds like a bug to me, because it contradicts what the doc string
> says.

That's how it seems to me too. In that case, I can update the patch I 
attached in my original message[1] to fix the bug. If we want to 
preserve the current behavior that Emacs 29 has exactly, we could also 
add a separate setting for how to handle killing the non-last client; 
then the two would be independently customizable. I'm not sure this is 
necessary, but if others think it is, I'm happy to write a patch for it.

[1] https://lists.gnu.org/archive/html/bug-gnu-emacs/2021-11/msg01702.html

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

On 10/24/2022 8:10 PM, Jim Porter wrote:
> That's how it seems to me too. In that case, I can update the patch I 
> attached in my original message[1] to fix the bug. If we want to 
> preserve the current behavior that Emacs 29 has exactly, we could also 
> add a separate setting for how to handle killing the non-last client; 
> then the two would be independently customizable. I'm not sure this is 
> necessary, but if others think it is, I'm happy to write a patch for it.

While working through this, I found a tangentially-related bug that 
occurs regardless of whether 'server-stop-automatically' is set: 
bug#58909. Since the discussion here is pretty long already, I opted to 
file a new issue for it, but in practice, it might be easier to write a 
patch that fixes both this bug and that one; there's a fair amount of 
crossover in the code for these.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

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

On 10/24/2022 8:10 PM, Jim Porter wrote:
> On 10/21/2022 11:57 PM, Eli Zaretskii wrote:
>>> Date: Fri, 21 Oct 2022 20:46:47 -0700
>>> Cc: larsi@gnus.org, 51993@debbugs.gnu.org, gregory@heytings.org
>>> From: Jim Porter <jporterbugs@gmail.com>
>>>
>>> The issue in the quote above is that if you enable automatic server
>>> shutdown in Emacs 29, it changes the behavior of exiting an emacsclient
>>> even when it wouldn't stop the server (i.e. when there are other active
>>> clients). That's surprising to me, I wouldn't expect that setting to
>>> affect cases when it decides *not* to kill the Emacs daemon.
>>
>> Sounds like a bug to me, because it contradicts what the doc string
>> says.
> 
> That's how it seems to me too. ...
Ok, after quite a delay, here's a patch for this. Previously, the 
function 'server-stop-automatically--handle-delete-frame' responded to 
both 'C-x C-c' ('save-buffers-kill-terminal') and 'delete-frame', which 
made it more complex. I've moved the 'C-x C-c' case into 
'server-save-buffers-kill-terminal', which simplifies 
'server-stop-automatically--handle-delete-frame'.

The updated 'server-save-buffers-kill-terminal' should now make sure 
that the new stop-automatically behavior only happens when there are no 
other client processes (or nowait frames).

I've also attached a version of the diff excluding the whitespace 
changes to make reviewing the code easier.

[-- Attachment #2: 0001-Make-killing-a-non-last-client-work-the-same-no-matt.patch --]
[-- Type: text/plain, Size: 5307 bytes --]

From 2c0940a86cd11ebad0d1ea7fcf8fe2e9cfbc0fb3 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 28 Nov 2022 21:17:59 -0800
Subject: [PATCH] Make killing a non-last client work the same no matter the
 auto-stop setting

Previously, if 'server-stop-automatically' was configured for
'kill-terminal' or 'delete-frame', killing a client via
'save-buffers-kill-terminal' wouldn't prompt about the saving files in
the client's buffer list (as it does when not using those settings).
This change ensures that those settings only apply when killing the
last client, as described in the manual (bug#51993).

* lisp/server.el (server-save-buffers-kill-terminal): Handle
'server-stop-automatically' behavior in this function, rather than
calling 'server-stop-automatically--handle-delete-frame'.
(server-stop-automatically--handle-delete-frame): Remove code that
handled the 'save-buffers-kill-terminal' case.
---
 lisp/server.el | 83 ++++++++++++++++++++++++--------------------------
 1 file changed, 40 insertions(+), 43 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 1b027f88ce..e88cffa8fb 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1780,52 +1780,49 @@ server-save-buffers-kill-terminal
 
 If emacsclient was started with a list of filenames to edit, then
 only these files will be asked to be saved."
-  (if server-stop-automatically
-      (server-stop-automatically--handle-delete-frame (selected-frame))
-    (let ((proc (frame-parameter nil 'client)))
-      (cond ((eq proc 'nowait)
-	     ;; Nowait frames have no client buffer list.
-	     (if (cdr (frame-list))
-	         (progn (save-some-buffers arg)
-		        (delete-frame))
-	       ;; If we're the last frame standing, kill Emacs.
-	       (save-buffers-kill-emacs arg)))
-	    ((processp proc)
-	     (let ((buffers (process-get proc 'buffers)))
-	       (save-some-buffers
-	        arg (if buffers
-                        ;; Only files from emacsclient file list.
-		        (lambda () (memq (current-buffer) buffers))
-                      ;; No emacsclient file list: don't override
-                      ;; `save-some-buffers-default-predicate' (unless
-                      ;; ARG is non-nil), since we're not killing
-                      ;; Emacs (unlike `save-buffers-kill-emacs').
-		      (and arg t)))
-	       (server-delete-client proc)))
-	    (t (error "Invalid client frame"))))))
+  (let ((proc (frame-parameter nil 'client)))
+    (cond ((eq proc 'nowait)
+	   ;; Nowait frames have no client buffer list.
+	   (if (length> (frame-list) (if server-stop-automatically 2 1))
+               ;; If there are any other frames, only delete this one.
+               ;; When `server-stop-automatically' is set, don't count
+               ;; the daemon frame.
+	       (progn (save-some-buffers arg)
+		      (delete-frame))
+	     ;; If we're the last frame standing, kill Emacs.
+	     (save-buffers-kill-emacs arg)))
+	  ((processp proc)
+           (if (or (not server-stop-automatically)
+                   (length> server-clients 1)
+                   (seq-some
+                    (lambda (frame)
+                      (when-let ((p (frame-parameter frame 'client)))
+                        (not (eq proc p))))
+                    (frame-list)))
+	       (let ((buffers (process-get proc 'buffers)))
+	         (save-some-buffers
+	          arg (if buffers
+                          ;; Only files from emacsclient file list.
+		          (lambda () (memq (current-buffer) buffers))
+                        ;; No emacsclient file list: don't override
+                        ;; `save-some-buffers-default-predicate' (unless
+                        ;; ARG is non-nil), since we're not killing
+                        ;; Emacs (unlike `save-buffers-kill-emacs').
+		        (and arg t)))
+	         (server-delete-client proc))
+             ;; If `server-stop-automatically' is set, there are no
+             ;; other client processes, and no other client frames
+             ;; (e.g. `nowait' frames), kill Emacs.
+             (save-buffers-kill-emacs arg)))
+	  (t (error "Invalid client frame")))))
 
 (defun server-stop-automatically--handle-delete-frame (frame)
   "Handle deletion of FRAME when `server-stop-automatically' is used."
-  (when server-stop-automatically
-    (if (if (and (processp (frame-parameter frame 'client))
-		 (eq this-command 'save-buffers-kill-terminal))
-	    (progn
-	      (dolist (f (frame-list))
-		(when (and (eq (frame-parameter frame 'client)
-			       (frame-parameter f 'client))
-			   (not (eq frame f)))
-		  (set-frame-parameter f 'client nil)
-		  (let ((server-stop-automatically nil))
-		    (delete-frame f))))
-	      (if (cddr (frame-list))
-		  (let ((server-stop-automatically nil))
-		    (delete-frame frame)
-		    nil)
-		t))
-	  (null (cddr (frame-list))))
-	(let ((server-stop-automatically nil))
-	  (save-buffers-kill-emacs)
-	  (delete-frame frame)))))
+  (when (and server-stop-automatically
+             (null (cddr (frame-list))))
+    (let ((server-stop-automatically nil))
+      (save-buffers-kill-emacs)
+      (delete-frame frame))))
 
 (defun server-stop-automatically--maybe-kill-emacs ()
   "Handle closing of Emacs daemon when `server-stop-automatically' is used."
-- 
2.25.1


[-- Attachment #3: no-whitespace-change.diff --]
[-- Type: text/plain, Size: 3078 bytes --]

diff --git a/lisp/server.el b/lisp/server.el
index 1b027f88ce..e88cffa8fb 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1780,17 +1780,25 @@ server-save-buffers-kill-terminal
 
 If emacsclient was started with a list of filenames to edit, then
 only these files will be asked to be saved."
-  (if server-stop-automatically
-      (server-stop-automatically--handle-delete-frame (selected-frame))
   (let ((proc (frame-parameter nil 'client)))
     (cond ((eq proc 'nowait)
 	   ;; Nowait frames have no client buffer list.
-	     (if (cdr (frame-list))
+	   (if (length> (frame-list) (if server-stop-automatically 2 1))
+               ;; If there are any other frames, only delete this one.
+               ;; When `server-stop-automatically' is set, don't count
+               ;; the daemon frame.
 	       (progn (save-some-buffers arg)
 		      (delete-frame))
 	     ;; If we're the last frame standing, kill Emacs.
 	     (save-buffers-kill-emacs arg)))
 	  ((processp proc)
+           (if (or (not server-stop-automatically)
+                   (length> server-clients 1)
+                   (seq-some
+                    (lambda (frame)
+                      (when-let ((p (frame-parameter frame 'client)))
+                        (not (eq proc p))))
+                    (frame-list)))
 	       (let ((buffers (process-get proc 'buffers)))
 	         (save-some-buffers
 	          arg (if buffers
@@ -1801,31 +1809,20 @@ server-save-buffers-kill-terminal
                         ;; ARG is non-nil), since we're not killing
                         ;; Emacs (unlike `save-buffers-kill-emacs').
 		        (and arg t)))
-	       (server-delete-client proc)))
-	    (t (error "Invalid client frame"))))))
+	         (server-delete-client proc))
+             ;; If `server-stop-automatically' is set, there are no
+             ;; other client processes, and no other client frames
+             ;; (e.g. `nowait' frames), kill Emacs.
+             (save-buffers-kill-emacs arg)))
+	  (t (error "Invalid client frame")))))
 
 (defun server-stop-automatically--handle-delete-frame (frame)
   "Handle deletion of FRAME when `server-stop-automatically' is used."
-  (when server-stop-automatically
-    (if (if (and (processp (frame-parameter frame 'client))
-		 (eq this-command 'save-buffers-kill-terminal))
-	    (progn
-	      (dolist (f (frame-list))
-		(when (and (eq (frame-parameter frame 'client)
-			       (frame-parameter f 'client))
-			   (not (eq frame f)))
-		  (set-frame-parameter f 'client nil)
-		  (let ((server-stop-automatically nil))
-		    (delete-frame f))))
-	      (if (cddr (frame-list))
-		  (let ((server-stop-automatically nil))
-		    (delete-frame frame)
-		    nil)
-		t))
+  (when (and server-stop-automatically
              (null (cddr (frame-list))))
     (let ((server-stop-automatically nil))
       (save-buffers-kill-emacs)
-	  (delete-frame frame)))))
+      (delete-frame frame))))
 
 (defun server-stop-automatically--maybe-kill-emacs ()
   "Handle closing of Emacs daemon when `server-stop-automatically' is used."

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Eli Zaretskii]

> Date: Mon, 28 Nov 2022 21:31:02 -0800
> From: Jim Porter <jporterbugs@gmail.com>
> Cc: larsi@gnus.org, 51993@debbugs.gnu.org, gregory@heytings.org
> 
> >>> The issue in the quote above is that if you enable automatic server
> >>> shutdown in Emacs 29, it changes the behavior of exiting an emacsclient
> >>> even when it wouldn't stop the server (i.e. when there are other active
> >>> clients). That's surprising to me, I wouldn't expect that setting to
> >>> affect cases when it decides *not* to kill the Emacs daemon.
> >>
> >> Sounds like a bug to me, because it contradicts what the doc string
> >> says.
> > 
> > That's how it seems to me too. ...
> Ok, after quite a delay, here's a patch for this. Previously, the 
> function 'server-stop-automatically--handle-delete-frame' responded to 
> both 'C-x C-c' ('save-buffers-kill-terminal') and 'delete-frame', which 
> made it more complex. I've moved the 'C-x C-c' case into 
> 'server-save-buffers-kill-terminal', which simplifies 
> 'server-stop-automatically--handle-delete-frame'.
> 
> The updated 'server-save-buffers-kill-terminal' should now make sure 
> that the new stop-automatically behavior only happens when there are no 
> other client processes (or nowait frames).

We want this on the release branch, right?  Then please make it the minimal
change which fixes the immediate cause of the bug, and does nothing else: no
refactoring, no reshuffling of the code or making it nicer or less
complicated -- all that just makes the risk of new bugs higher and the job
of reviewing the patch harder.

> +	   (if (length> (frame-list) (if server-stop-automatically 2 1))
> +               ;; If there are any other frames, only delete this one.
> +               ;; When `server-stop-automatically' is set, don't count
> +               ;; the daemon frame.
>  	       (progn (save-some-buffers arg)
>  		      (delete-frame))
>  	     ;; If we're the last frame standing, kill Emacs.
>  	     (save-buffers-kill-emacs arg)))

This part is easily understood.

>  	  ((processp proc)
> +           (if (or (not server-stop-automatically)
> +                   (length> server-clients 1)
> +                   (seq-some
> +                    (lambda (frame)
> +                      (when-let ((p (frame-parameter frame 'client)))
> +                        (not (eq proc p))))
> +                    (frame-list)))
>  	       (let ((buffers (process-get proc 'buffers)))
>  	         (save-some-buffers
>  	          arg (if buffers

This part is also easily understood.

> @@ -1801,31 +1809,20 @@ server-save-buffers-kill-terminal
>                          ;; ARG is non-nil), since we're not killing
>                          ;; Emacs (unlike `save-buffers-kill-emacs').
>  		        (and arg t)))
> -	       (server-delete-client proc)))
> -	    (t (error "Invalid client frame"))))))
> +	         (server-delete-client proc))
> +             ;; If `server-stop-automatically' is set, there are no
> +             ;; other client processes, and no other client frames
> +             ;; (e.g. `nowait' frames), kill Emacs.
> +             (save-buffers-kill-emacs arg)))
> +	  (t (error "Invalid client frame")))))

But this one is problematic: it adds save-buffers-kill-emacs which wasn't in
the original code, and I don't understand why.  The bug wasn't about this,
was it?

>  (defun server-stop-automatically--handle-delete-frame (frame)
>    "Handle deletion of FRAME when `server-stop-automatically' is used."
> -  (when server-stop-automatically
> -    (if (if (and (processp (frame-parameter frame 'client))
> -		 (eq this-command 'save-buffers-kill-terminal))
> -	    (progn
> -	      (dolist (f (frame-list))
> -		(when (and (eq (frame-parameter frame 'client)
> -			       (frame-parameter f 'client))
> -			   (not (eq frame f)))
> -		  (set-frame-parameter f 'client nil)
> -		  (let ((server-stop-automatically nil))
> -		    (delete-frame f))))
> -	      (if (cddr (frame-list))
> -		  (let ((server-stop-automatically nil))
> -		    (delete-frame frame)
> -		    nil)
> -		t))
> +  (when (and server-stop-automatically
>               (null (cddr (frame-list))))
>      (let ((server-stop-automatically nil))
>        (save-buffers-kill-emacs)
> -	  (delete-frame frame)))))
> +      (delete-frame frame))))

And here you completely rewrote a function.

I'm okay with installing the original changes on master, if you indeed
believe the new code is much cleaner (but then please explain why you think
so, because I don't think I see that just by looking at the diffs).  But for
the release branch, I'm not comfortable with making such serious changes in
a part of server.el that is already way too complicated, what with all the
fancy shutdown options we strive to support.  There be dragons, and I have
no intention to release Emacs 29 with buggy server-client editing.  So for
the release branch, please prepare a safer version of the change, which only
changes the code which is the immediate cause of the incorrect behavior.

Thanks.

bug#51993: 29.0.50; [PATCH for 29.1] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

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

On 12/1/2022 9:29 AM, Eli Zaretskii wrote:
>> Date: Mon, 28 Nov 2022 21:31:02 -0800
>> From: Jim Porter <jporterbugs@gmail.com>
>> Cc: larsi@gnus.org, 51993@debbugs.gnu.org, gregory@heytings.org
>>
>>>>> The issue in the quote above is that if you enable automatic server
>>>>> shutdown in Emacs 29, it changes the behavior of exiting an emacsclient
>>>>> even when it wouldn't stop the server (i.e. when there are other active
>>>>> clients). That's surprising to me, I wouldn't expect that setting to
>>>>> affect cases when it decides *not* to kill the Emacs daemon.
>>>>
>>>> Sounds like a bug to me, because it contradicts what the doc string
>>>> says.
>>>
>>> That's how it seems to me too. ...
>> Ok, after quite a delay, here's a patch for this. Previously, the
>> function 'server-stop-automatically--handle-delete-frame' responded to
>> both 'C-x C-c' ('save-buffers-kill-terminal') and 'delete-frame', which
>> made it more complex. I've moved the 'C-x C-c' case into
>> 'server-save-buffers-kill-terminal', which simplifies
>> 'server-stop-automatically--handle-delete-frame'.
>>
>> The updated 'server-save-buffers-kill-terminal' should now make sure
>> that the new stop-automatically behavior only happens when there are no
>> other client processes (or nowait frames).
> 
> We want this on the release branch, right?  Then please make it the minimal
> change which fixes the immediate cause of the bug, and does nothing else: no
> refactoring, no reshuffling of the code or making it nicer or less
> complicated -- all that just makes the risk of new bugs higher and the job
> of reviewing the patch harder.

Thanks for taking a look. I believe we'd want this on the release 
branch. Here's the absolute minimum I could manage, although it doesn't 
quite fix everything that my previous patch does. In particular, when 
server-stop-automatically is set to 'kill-terminal' (or 'delete-frame'):

* If you type 'C-x C-c' ('save-buffers-kill-terminal') in a nowait 
client frame, and there are still other (non-daemon) frames, nothing 
happens. You'd have to use 'C-x 5 0' ('delete-frame') instead. Fixing 
this would basically mean doing 90% of my original patch, so it's 
probably too risky.

* If you type 'C-u C-x C-c', it doesn't silently save all the relevant 
buffers. That's because 'server-save-buffers-kill-terminal' doesn't 
forward the prefix arg to 
'server-stop-automatically--handle-delete-frame'. That's a separate (but 
closely related) bug, so I didn't fix that either.

In a followup message, I'll show the breakdown of my previous patch into 
smaller steps with some more detailed explanation of why I think it 
simplifies things enough to be worth making the change on the master branch.

[-- Attachment #2: 0001-Make-killing-a-non-last-client-work-the-same-no-matt.patch --]
[-- Type: text/plain, Size: 3279 bytes --]

From 198f98b43d5dde34775010ac987d31e90745381f Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 1 Dec 2022 16:46:57 -0800
Subject: [PATCH] Make killing a non-last client work the same no matter the
 auto-stop setting

Do not merge to master.  (This is a minimal change for the release
branch, and will be fixed in a cleaner way on master.)

Previously, if 'server-stop-automatically' was configured for
'kill-terminal' or 'delete-frame', killing a client via
'save-buffers-kill-terminal' wouldn't prompt about the saving files in
the client's buffer list (as it does when not using those settings).
This change ensures that those settings only apply when killing the
last client, as described in the manual (bug#51993).

* lisp/server.el (server-stop-automatically--handle-delete-frame):
When called by 'save-buffers-kill-terminal', first check to see
whether we'll want to kill Emacs, and if not, call 'save-some-buffers'
and 'server-delete-client', as in 'server-save-buffers-kill-terminal'.
---
 lisp/server.el | 39 ++++++++++++++++++++++++++-------------
 1 file changed, 26 insertions(+), 13 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 1b027f88ce..0657af1f98 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1809,19 +1809,32 @@ server-stop-automatically--handle-delete-frame
   (when server-stop-automatically
     (if (if (and (processp (frame-parameter frame 'client))
 		 (eq this-command 'save-buffers-kill-terminal))
-	    (progn
-	      (dolist (f (frame-list))
-		(when (and (eq (frame-parameter frame 'client)
-			       (frame-parameter f 'client))
-			   (not (eq frame f)))
-		  (set-frame-parameter f 'client nil)
-		  (let ((server-stop-automatically nil))
-		    (delete-frame f))))
-	      (if (cddr (frame-list))
-		  (let ((server-stop-automatically nil))
-		    (delete-frame frame)
-		    nil)
-		t))
+            (let* ((proc (frame-parameter frame 'client))
+                   ;; Keep the server alive if...
+                   (keep-server-alive-p
+                    (or
+                     ;; a) there are any other clients, or...
+                     (length> server-clients 1)
+                     ;; b) there are any frames not owned by this
+                     ;; client.
+                     (seq-some
+                      (lambda (frame)
+                        (when-let ((p (frame-parameter frame 'client)))
+                          (not (eq proc p))))
+                      (frame-list)))))
+              (when keep-server-alive-p
+                ;; If we want to keep the server alive, don't kill
+                ;; Emacs.  Instead, just call `save-some-buffers' and
+                ;; delete the client, as in
+                ;; `server-save-buffers-kill-terminal' above.
+                (let ((buffers (process-get proc 'buffers)))
+	          (save-some-buffers
+                   nil (if buffers
+                           (lambda () (memq (current-buffer) buffers))))
+	          (server-delete-client proc)))
+              ;; Tell the outer `if' block whether we want to kill
+              ;; Emacs.
+              (not keep-server-alive-p))
 	  (null (cddr (frame-list))))
 	(let ((server-stop-automatically nil))
 	  (save-buffers-kill-emacs)
-- 
2.25.1

bug#51993: 29.0.50; [PATCH for 29.1] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Eli Zaretskii]

[Please in the future avoid changing the Subject when discussing bugs: it
makes it harder for me and others to follow the discussion by grouping
messages by Subject.]

> Date: Thu, 1 Dec 2022 17:09:24 -0800
> Cc: larsi@gnus.org, 51993@debbugs.gnu.org, gregory@heytings.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> > We want this on the release branch, right?  Then please make it the minimal
> > change which fixes the immediate cause of the bug, and does nothing else: no
> > refactoring, no reshuffling of the code or making it nicer or less
> > complicated -- all that just makes the risk of new bugs higher and the job
> > of reviewing the patch harder.
> 
> Thanks for taking a look. I believe we'd want this on the release 
> branch. Here's the absolute minimum I could manage, although it doesn't 
> quite fix everything that my previous patch does.

Thanks.  Surprisingly, the previous version was easier to review and agree
to in some of its parts, because it kept more of the original code, and only
changed the conditions when save-some-buffers or save-buffers-kill-emacs
were called.  This version of the patch completely rewrites the affected
code.  Was that really necessary?

> In particular, when 
> server-stop-automatically is set to 'kill-terminal' (or 'delete-frame'):
> 
> * If you type 'C-x C-c' ('save-buffers-kill-terminal') in a nowait 
> client frame, and there are still other (non-daemon) frames, nothing 
> happens. You'd have to use 'C-x 5 0' ('delete-frame') instead. Fixing 
> this would basically mean doing 90% of my original patch, so it's 
> probably too risky.
> 
> * If you type 'C-u C-x C-c', it doesn't silently save all the relevant 
> buffers. That's because 'server-save-buffers-kill-terminal' doesn't 
> forward the prefix arg to 
> 'server-stop-automatically--handle-delete-frame'. That's a separate (but 
> closely related) bug, so I didn't fix that either.

Please tell more about why these two situations cannot be handled by
similarly simple changes.

What made the original patch risky from my POV is that it rewrote too much
of the original code.  Would it be possible to retain as much as possible of
the original code, and just change the conditions for calling
save-some-buffers etc., and/or add calls to it where it previously wasn't
called for some reason?

Please understand where I stand: use of emacsclient is very popular and very
widespread, so I'd like to avoid adding regressions to it as result of
enhancements we make.  If I conclude that the new features installed as part
of Emacs 29 development are too complicated and/or include semi-buggy
aspects, I'd rather revert those new features than risk regressions in
emacsclient and in the server behavior.

bug#51993: 29.0.50; [PATCH for 29.1] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

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

On 12/2/2022 6:10 AM, Eli Zaretskii wrote:
> [Please in the future avoid changing the Subject when discussing bugs: it
> makes it harder for me and others to follow the discussion by grouping
> messages by Subject.]

Sorry about that. I thought it would be easier to see that I'd split the 
topic into two separate tracks. I'll keep that in mind for the future 
though.

> Thanks.  Surprisingly, the previous version was easier to review and agree
> to in some of its parts, because it kept more of the original code, and only
> changed the conditions when save-some-buffers or save-buffers-kill-emacs
> were called.

I think I must have misinterpreted where your main concerns were, and 
went down the wrong avenue as a result. I think your other comments here 
(and re-reading your previous message) have helped me get a better idea 
of your concerns, so I'll try to address your earlier message better.

On 12/1/2022 9:29 AM, Eli Zaretskii wrote:
>>  (defun server-stop-automatically--handle-delete-frame (frame)
>>    "Handle deletion of FRAME when `server-stop-automatically' is used."
[snip -- long diff removed]
> 
> And here you completely rewrote a function.

Thinking about this some more, that change wasn't strictly necessary for 
29.1, since it should really just be dead code, and won't hurt anything. 
I've split this part out into a separate commit that we could apply to 
only the master branch. That simplifies the first commit (for 29.1) a 
fair bit.

>> @@ -1801,31 +1809,20 @@ server-save-buffers-kill-terminal
>>                          ;; ARG is non-nil), since we're not killing
>>                          ;; Emacs (unlike `save-buffers-kill-emacs').
>>  		        (and arg t)))
>> -	       (server-delete-client proc)))
>> -	    (t (error "Invalid client frame"))))))
>> +	         (server-delete-client proc))
>> +             ;; If `server-stop-automatically' is set, there are no
>> +             ;; other client processes, and no other client frames
>> +             ;; (e.g. `nowait' frames), kill Emacs.
>> +             (save-buffers-kill-emacs arg)))
>> +	  (t (error "Invalid client frame")))))
> 
> But this one is problematic: it adds save-buffers-kill-emacs which wasn't in
> the original code, and I don't understand why.  The bug wasn't about this,
> was it?

I believe this change is the most important part of the patch, so I'll 
try to explain why I added this. The 'server-stop-automatically' feature 
causes the Emacs daemon to be killed when certain conditions are met. 
For the 'kill-terminal' case[1], that means that, "when the last client 
frame is being closed with C-x C-c, you are asked whether [various 
questions], and if so, the server is stopped." (From the manual section 
on this.)

The "last client frame" check is the conditional just before this hunk 
(you mentioned that this part was fine, so I didn't include it in this 
message). The latter part of the sentence in the manual is just a 
description of 'save-buffers-kill-emacs'. So I've tried to take the text 
of the manual and turn it into code.

Another way of looking at it: the new 'save-buffers-kill-emacs' call 
above is really the result of me moving the handling for this condition 
from 'server-stop-automatically--handle-delete-frame' into 
'server-save-buffers-kill-terminal'. Previously, that function was 
responsible for calling 'save-buffers-kill-emacs' at the right time. 
Doing it this way lets me avoid calling 'SSA--handle-delete-frame' so 
that we get the benefit of using the longstanding implementation of 
'server-save-buffers-kill-terminal', just with the minimum of necessary 
enhancements to make this stop-automatically setting behave as documented.

[1] This also applies to 'delete-frame', but that setting does more 
beyond this.

[-- Attachment #2: 29.1--0001-Make-killing-a-non-last-client-work-the-same-no-matt.patch --]
[-- Type: text/plain, Size: 4324 bytes --]

From 6081e156ba34f9f7d8b212be8201639f2f689219 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 2 Dec 2022 12:14:50 -0800
Subject: [PATCH 1/2] Make killing a non-last client work the same no matter
 the auto-stop setting

Previously, if 'server-stop-automatically' was configured for
'kill-terminal' or 'delete-frame', killing a client via
'save-buffers-kill-terminal' wouldn't prompt about the saving files in
the client's buffer list (as it does when not using those settings).
This change ensures that those settings only apply when killing the
last client, as described in the manual (bug#51993).

* lisp/server.el (server-save-buffers-kill-terminal): Handle
'server-stop-automatically' behavior in this function, rather than
calling 'server-stop-automatically--handle-delete-frame'.
---
 lisp/server.el | 60 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 37 insertions(+), 23 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 1b027f88ce..7e713eaecd 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1780,29 +1780,43 @@ server-save-buffers-kill-terminal
 
 If emacsclient was started with a list of filenames to edit, then
 only these files will be asked to be saved."
-  (if server-stop-automatically
-      (server-stop-automatically--handle-delete-frame (selected-frame))
-    (let ((proc (frame-parameter nil 'client)))
-      (cond ((eq proc 'nowait)
-	     ;; Nowait frames have no client buffer list.
-	     (if (cdr (frame-list))
-	         (progn (save-some-buffers arg)
-		        (delete-frame))
-	       ;; If we're the last frame standing, kill Emacs.
-	       (save-buffers-kill-emacs arg)))
-	    ((processp proc)
-	     (let ((buffers (process-get proc 'buffers)))
-	       (save-some-buffers
-	        arg (if buffers
-                        ;; Only files from emacsclient file list.
-		        (lambda () (memq (current-buffer) buffers))
-                      ;; No emacsclient file list: don't override
-                      ;; `save-some-buffers-default-predicate' (unless
-                      ;; ARG is non-nil), since we're not killing
-                      ;; Emacs (unlike `save-buffers-kill-emacs').
-		      (and arg t)))
-	       (server-delete-client proc)))
-	    (t (error "Invalid client frame"))))))
+  (let ((proc (frame-parameter nil 'client)))
+    (cond ((eq proc 'nowait)
+	   ;; Nowait frames have no client buffer list.
+	   (if (length> (frame-list) (if server-stop-automatically 2 1))
+               ;; If there are any other frames, only delete this one.
+               ;; When `server-stop-automatically' is set, don't count
+               ;; the daemon frame.
+	       (progn (save-some-buffers arg)
+		      (delete-frame))
+	     ;; If we're the last frame standing, kill Emacs.
+	     (save-buffers-kill-emacs arg)))
+	  ((processp proc)
+           (if (or (not server-stop-automatically)
+                   (length> server-clients 1)
+                   (seq-some
+                    (lambda (frame)
+                      (when-let ((p (frame-parameter frame 'client)))
+                        (not (eq proc p))))
+                    (frame-list)))
+               ;; If `server-stop-automatically' is not enabled, there
+               ;; are any other clients, or there are frames not owned
+               ;; by the current client (e.g. `nowait' frames), then
+               ;; we just want to delete this client.
+	       (let ((buffers (process-get proc 'buffers)))
+	         (save-some-buffers
+	          arg (if buffers
+                          ;; Only files from emacsclient file list.
+		          (lambda () (memq (current-buffer) buffers))
+                        ;; No emacsclient file list: don't override
+                        ;; `save-some-buffers-default-predicate' (unless
+                        ;; ARG is non-nil), since we're not killing
+                        ;; Emacs (unlike `save-buffers-kill-emacs').
+		        (and arg t)))
+	         (server-delete-client proc))
+             ;; Otherwise, we want to kill Emacs.
+             (save-buffers-kill-emacs arg)))
+	  (t (error "Invalid client frame")))))
 
 (defun server-stop-automatically--handle-delete-frame (frame)
   "Handle deletion of FRAME when `server-stop-automatically' is used."
-- 
2.25.1


[-- Attachment #3: master--0002-Remove-dead-code-from-server-stop-automatically-hand.patch --]
[-- Type: text/plain, Size: 1991 bytes --]

From a14b042982dc59767495905a9d45e0542124c1e6 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Fri, 2 Dec 2022 12:15:46 -0800
Subject: [PATCH 2/2] ; Remove dead code from
 'server-stop-automatically--handle-delete-frame'

* lisp/server.el (server-stop-automatically--handle-delete-frame):
Since 'this-command' is never 'save-buffers-kill-terminal' in this
function, we can remove the second 'if' block and it's then-form,
leaving only the else-form.  Additionally, remove the 'delete-frame'
call; it's not necessary, since we just killed Emacs on the prior
line.
---
 lisp/server.el | 24 ++++--------------------
 1 file changed, 4 insertions(+), 20 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 7e713eaecd..aa62b25a89 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1820,26 +1820,10 @@ server-save-buffers-kill-terminal
 
 (defun server-stop-automatically--handle-delete-frame (frame)
   "Handle deletion of FRAME when `server-stop-automatically' is used."
-  (when server-stop-automatically
-    (if (if (and (processp (frame-parameter frame 'client))
-		 (eq this-command 'save-buffers-kill-terminal))
-	    (progn
-	      (dolist (f (frame-list))
-		(when (and (eq (frame-parameter frame 'client)
-			       (frame-parameter f 'client))
-			   (not (eq frame f)))
-		  (set-frame-parameter f 'client nil)
-		  (let ((server-stop-automatically nil))
-		    (delete-frame f))))
-	      (if (cddr (frame-list))
-		  (let ((server-stop-automatically nil))
-		    (delete-frame frame)
-		    nil)
-		t))
-	  (null (cddr (frame-list))))
-	(let ((server-stop-automatically nil))
-	  (save-buffers-kill-emacs)
-	  (delete-frame frame)))))
+  (when (and server-stop-automatically
+             (null (cddr (frame-list))))
+    (let ((server-stop-automatically nil))
+      (save-buffers-kill-emacs))))
 
 (defun server-stop-automatically--maybe-kill-emacs ()
   "Handle closing of Emacs daemon when `server-stop-automatically' is used."
-- 
2.25.1

bug#51993: 29.0.50; [PATCH for 29.1] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Eli Zaretskii]

> Date: Fri, 2 Dec 2022 13:33:39 -0800
> Cc: larsi@gnus.org, 51993@debbugs.gnu.org, gregory@heytings.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> > And here you completely rewrote a function.
> 
> Thinking about this some more, that change wasn't strictly necessary for 
> 29.1, since it should really just be dead code, and won't hurt anything. 
> I've split this part out into a separate commit that we could apply to 
> only the master branch. That simplifies the first commit (for 29.1) a 
> fair bit.

Thanks, I'm okay with committing these two patches, each one to its branch.
(But I guess you need to wait with installing the second one until the first
one is merged to master, right?)

bug#51993: 29.0.50; [PATCH for 29.1] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

On 12/4/2022 9:56 AM, Eli Zaretskii wrote:
> Thanks, I'm okay with committing these two patches, each one to its branch.
> (But I guess you need to wait with installing the second one until the first
> one is merged to master, right?)

Thanks again for the reviews. I've merged the first part to the 29 
branch as 4bcdb1cc65bf779b6479f99a7aa767ab83b3bae1.

Like you said, the second patch (for master) should wait until the next 
merge of 29 to master. There's no rush on this though, so I'll just set 
a reminder to do that when it's ready. (I could theoretically merge 29 
to master myself, but since I've never done that, I'll leave it to the 
experts. :))

bug#51993: 29.0.50; [PATCH for 29.1] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

On 12/4/2022 2:26 PM, Jim Porter wrote:
> Thanks again for the reviews. I've merged the first part to the 29 
> branch as 4bcdb1cc65bf779b6479f99a7aa767ab83b3bae1.
> 
> Like you said, the second patch (for master) should wait until the next 
> merge of 29 to master.

And now I've merged the cleanup patch to master as 
bcf4d96db3a61e0d02a584fa9fceb049cdad6fe8. Closing this bug now.

Thanks once again.

bug#51993: 29.0.50; [PATCH explanation] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

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

On 12/1/2022 9:29 AM, Eli Zaretskii wrote:
> I'm okay with installing the original changes on master, if you indeed
> believe the new code is much cleaner (but then please explain why you think
> so, because I don't think I see that just by looking at the diffs).  But for
> the release branch, I'm not comfortable with making such serious changes in
> a part of server.el that is already way too complicated, what with all the
> fancy shutdown options we strive to support.  There be dragons, and I have
> no intention to release Emacs 29 with buggy server-client editing.  So for
> the release branch, please prepare a safer version of the change, which only
> changes the code which is the immediate cause of the incorrect behavior.

Attached is a patch series that explains in more detail how I arrived at 
the previous version of my patch. This is basically a reconstruction of 
the steps I took when writing it originally. I'll describe my overall 
plan and then address the specific comments you had after that. (This 
might be overly-verbose, but I wanted to put as much detail in as I 
could in the hopes of addressing all your concerns.)

Prior to my patch, 'server-stop-automatically--handle-delete-frame' 
(henceforth 'SSA--handle-delete-frame') can get called in two 
situations: when someone calls 'delete-frame' (it's a hook on 
'delete-frame-functions') or when someone calls 
'save-buffers-kill-emacs' ('server-save-buffers-kill-emacs' delegates to 
it when configured to). To help make the logic easier to follow, I split 
it into two: one for handling 'delete-frame', and one for handling 
'save-buffers-kill-terminal'. See patches 0001 and 0002.

>>  (defun server-stop-automatically--handle-delete-frame (frame)
>>    "Handle deletion of FRAME when `server-stop-automatically' is used."
>> -  (when server-stop-automatically
>> -    (if (if (and (processp (frame-parameter frame 'client))
>> -		 (eq this-command 'save-buffers-kill-terminal))
>> -	    (progn
>> -	      (dolist (f (frame-list))
>> -		(when (and (eq (frame-parameter frame 'client)
>> -			       (frame-parameter f 'client))
>> -			   (not (eq frame f)))
>> -		  (set-frame-parameter f 'client nil)
>> -		  (let ((server-stop-automatically nil))
>> -		    (delete-frame f))))
>> -	      (if (cddr (frame-list))
>> -		  (let ((server-stop-automatically nil))
>> -		    (delete-frame frame)
>> -		    nil)
>> -		t))
>> +  (when (and server-stop-automatically
>>               (null (cddr (frame-list))))
>>      (let ((server-stop-automatically nil))
>>        (save-buffers-kill-emacs)
>> -	  (delete-frame frame)))))
>> +      (delete-frame frame))))
> 
> And here you completely rewrote a function.

In patch 0002, you can see (most of) this change that you mentioned: 
since I made one function for each case as described above, the second 
'if' statement's conditional is always false, so I just got rid of the 
conditional and the then-clause, leaving only the else-clause: (null 
(cddr (frame-list))). I also simplified this function a bit in the last 
patch, 0007.

Now for the rest of the patch series.

The original bug is that the behavior of 
'server-save-buffers-kill-terminal' when there are multiple clients 
should be the same regardless of the SSA setting (it wasn't). So next, I 
made 'SSA--handle-kill-terminal' have the same basic structure as 
'server-save-buffers-kill-terminal' (patch 0003). That makes it easier 
to see the differences between the two.

Patch 0005 is where the real fix is: it makes sure that when we *don't* 
want to kill Emacs, 'SSA--handle-kill-terminal' does the same thing as 
'server-save-buffers-kill-terminal'.

>> @@ -1801,31 +1809,20 @@ server-save-buffers-kill-terminal
>>                          ;; ARG is non-nil), since we're not killing
>>                          ;; Emacs (unlike `save-buffers-kill-emacs').
>>  		        (and arg t)))
>> -	       (server-delete-client proc)))
>> -	    (t (error "Invalid client frame"))))))
>> +	         (server-delete-client proc))
>> +             ;; If `server-stop-automatically' is set, there are no
>> +             ;; other client processes, and no other client frames
>> +             ;; (e.g. `nowait' frames), kill Emacs.
>> +             (save-buffers-kill-emacs arg)))
>> +	  (t (error "Invalid client frame")))))
> 
> But this one is problematic: it adds save-buffers-kill-emacs which wasn't in
> the original code, and I don't understand why.  The bug wasn't about this,
> was it?

In patch 0005, you can start to see this block of code take shape: 
because we want to handle the "don't kill Emacs" case in 
'SSA--handle-kill-terminal', we add the 'save-some-buffers' + 
'server-delete-client' code there, resulting in something that looks 
similar to the above hunk.

Then, in patch 0006, I just merge 'server-save-buffers-kill-terminal' 
and 'SSA--handle-kill-terminal', since most of the code is shared at 
this point. Finally, patch 0007 is just a bit of cleanup; with all of 
these applied, server.el should be identical to my previous patch.

Hopefully this explains things reasonably well, and doesn't go into too 
much (or too little) detail. If there are any other bits that you have 
concerns about, just let me know.

[-- Attachment #2: 0001-Duplicate-server-stop-automatically-handle-delete-fr.patch --]
[-- Type: text/plain, Size: 2246 bytes --]

From 74c724a46b16782d6827357dd64efbafe8aeb92f Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 1 Dec 2022 11:33:18 -0800
Subject: [PATCH 1/7] Duplicate
 'server-stop-automatically--handle-delete-frame'

* lisp/server.el (server-stop-automatically--handle-delete-frame):
Copy this...
(server-stop-automatically--handle-kill-terminal) ... to this.
(server-save-buffers-kill-terminal): Use it.
---
 lisp/server.el | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/lisp/server.el b/lisp/server.el
index 1b027f88ce..0a59c8496a 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1781,7 +1781,7 @@ server-save-buffers-kill-terminal
 If emacsclient was started with a list of filenames to edit, then
 only these files will be asked to be saved."
   (if server-stop-automatically
-      (server-stop-automatically--handle-delete-frame (selected-frame))
+      (server-stop-automatically--handle-kill-terminal (selected-frame))
     (let ((proc (frame-parameter nil 'client)))
       (cond ((eq proc 'nowait)
 	     ;; Nowait frames have no client buffer list.
@@ -1827,6 +1827,29 @@ server-stop-automatically--handle-delete-frame
 	  (save-buffers-kill-emacs)
 	  (delete-frame frame)))))
 
+(defun server-stop-automatically--handle-kill-terminal (frame)
+  "Handle deletion of FRAME when `server-stop-automatically' is used."
+  (when server-stop-automatically
+    (if (if (and (processp (frame-parameter frame 'client))
+		 (eq this-command 'save-buffers-kill-terminal))
+	    (progn
+	      (dolist (f (frame-list))
+		(when (and (eq (frame-parameter frame 'client)
+			       (frame-parameter f 'client))
+			   (not (eq frame f)))
+		  (set-frame-parameter f 'client nil)
+		  (let ((server-stop-automatically nil))
+		    (delete-frame f))))
+	      (if (cddr (frame-list))
+		  (let ((server-stop-automatically nil))
+		    (delete-frame frame)
+		    nil)
+		t))
+	  (null (cddr (frame-list))))
+	(let ((server-stop-automatically nil))
+	  (save-buffers-kill-emacs)
+	  (delete-frame frame)))))
+
 (defun server-stop-automatically--maybe-kill-emacs ()
   "Handle closing of Emacs daemon when `server-stop-automatically' is used."
   (unless (cdr (frame-list))
-- 
2.25.1


[-- Attachment #3: 0002-Simplify-server-stop-automatically-handlers.patch --]
[-- Type: text/plain, Size: 2139 bytes --]

From be0b4556b9cb1ab882d7e6ce5e85dd20cfdc9d96 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 1 Dec 2022 11:35:00 -0800
Subject: [PATCH 2/7] Simplify server-stop-automatically handlers

* lisp/server.el (server-stop-automatically--handle-delete-frame):
'this-command' is never 'save-buffers-kill-terminal' here; remove
the whole conditional.
(server-stop-automatically--handle-kill-terminal): 'this-command' is
always 'save-buffers-kill-terminal', so remove that test.
---
 lisp/server.el | 20 ++------------------
 1 file changed, 2 insertions(+), 18 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 0a59c8496a..64332442d3 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1807,22 +1807,7 @@ server-save-buffers-kill-terminal
 (defun server-stop-automatically--handle-delete-frame (frame)
   "Handle deletion of FRAME when `server-stop-automatically' is used."
   (when server-stop-automatically
-    (if (if (and (processp (frame-parameter frame 'client))
-		 (eq this-command 'save-buffers-kill-terminal))
-	    (progn
-	      (dolist (f (frame-list))
-		(when (and (eq (frame-parameter frame 'client)
-			       (frame-parameter f 'client))
-			   (not (eq frame f)))
-		  (set-frame-parameter f 'client nil)
-		  (let ((server-stop-automatically nil))
-		    (delete-frame f))))
-	      (if (cddr (frame-list))
-		  (let ((server-stop-automatically nil))
-		    (delete-frame frame)
-		    nil)
-		t))
-	  (null (cddr (frame-list))))
+    (if (null (cddr (frame-list)))
 	(let ((server-stop-automatically nil))
 	  (save-buffers-kill-emacs)
 	  (delete-frame frame)))))
@@ -1830,8 +1815,7 @@ server-stop-automatically--handle-delete-frame
 (defun server-stop-automatically--handle-kill-terminal (frame)
   "Handle deletion of FRAME when `server-stop-automatically' is used."
   (when server-stop-automatically
-    (if (if (and (processp (frame-parameter frame 'client))
-		 (eq this-command 'save-buffers-kill-terminal))
+    (if (if (processp (frame-parameter frame 'client))
 	    (progn
 	      (dolist (f (frame-list))
 		(when (and (eq (frame-parameter frame 'client)
-- 
2.25.1


[-- Attachment #4: 0003-Restructure-server-stop-automatically-handle-kill-te.patch --]
[-- Type: text/plain, Size: 2820 bytes --]

From db0cffb849c0a0be50db7ad4a3a0d48ff343854b Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 1 Dec 2022 11:42:48 -0800
Subject: [PATCH 3/7] Restructure
 'server-stop-automatically--handle-kill-terminal'

This changes the function to have the same structure as
'server-save-buffers-kill-terminal', but otherwise works exactly the
same as before.  This just takes the nested 'if' statement and brings
it to the top level as 'cond' (note that the 'client' frame parameter
can only be a process or the symbol 'nowait' here, since otherwise
'server-save-buffers-kill-terminal' wouldn't have been called).

* lisp/server.el (server-stop-automatically--handle-kill-terminal):
Restructure.
---
 lisp/server.el | 41 +++++++++++++++++++++++------------------
 1 file changed, 23 insertions(+), 18 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 64332442d3..7191387959 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1815,24 +1815,29 @@ server-stop-automatically--handle-delete-frame
 (defun server-stop-automatically--handle-kill-terminal (frame)
   "Handle deletion of FRAME when `server-stop-automatically' is used."
   (when server-stop-automatically
-    (if (if (processp (frame-parameter frame 'client))
-	    (progn
-	      (dolist (f (frame-list))
-		(when (and (eq (frame-parameter frame 'client)
-			       (frame-parameter f 'client))
-			   (not (eq frame f)))
-		  (set-frame-parameter f 'client nil)
-		  (let ((server-stop-automatically nil))
-		    (delete-frame f))))
-	      (if (cddr (frame-list))
-		  (let ((server-stop-automatically nil))
-		    (delete-frame frame)
-		    nil)
-		t))
-	  (null (cddr (frame-list))))
-	(let ((server-stop-automatically nil))
-	  (save-buffers-kill-emacs)
-	  (delete-frame frame)))))
+    (let ((proc (frame-parameter frame 'client)))
+      (cond ((eq proc 'nowait)
+             (if (null (cddr (frame-list)))
+                 (let ((server-stop-automatically nil))
+	           (save-buffers-kill-emacs)
+	           (delete-frame frame))))
+            ((processp proc)
+             (if (progn
+	           (dolist (f (frame-list))
+		     (when (and (eq (frame-parameter frame 'client)
+			            (frame-parameter f 'client))
+			        (not (eq frame f)))
+		       (set-frame-parameter f 'client nil)
+		       (let ((server-stop-automatically nil))
+		         (delete-frame f))))
+	           (if (cddr (frame-list))
+		       (let ((server-stop-automatically nil))
+		         (delete-frame frame)
+		         nil)
+		     t))
+	         (let ((server-stop-automatically nil))
+	           (save-buffers-kill-emacs)
+	           (delete-frame frame))))))))
 
 (defun server-stop-automatically--maybe-kill-emacs ()
   "Handle closing of Emacs daemon when `server-stop-automatically' is used."
-- 
2.25.1


[-- Attachment #5: 0004-Remove-unnecessary-delete-frame-calls-after-save-buf.patch --]
[-- Type: text/plain, Size: 1538 bytes --]

From 694aad432aee73acd2335f5ea5d0f47d0c8379e2 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 1 Dec 2022 11:48:11 -0800
Subject: [PATCH 4/7] Remove unnecessary 'delete-frame' calls after
 'save-buffers-kill-emacs'

No need to delete a frame if we're killing Emacs entirely anyway.

* lisp/server.el (server-stop-automatically--handle-kill-terminal):
Remove a couple 'delete-frame' calls.
---
 lisp/server.el | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 7191387959..399bf694fd 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1818,9 +1818,7 @@ server-stop-automatically--handle-kill-terminal
     (let ((proc (frame-parameter frame 'client)))
       (cond ((eq proc 'nowait)
              (if (null (cddr (frame-list)))
-                 (let ((server-stop-automatically nil))
-	           (save-buffers-kill-emacs)
-	           (delete-frame frame))))
+	         (save-buffers-kill-emacs)))
             ((processp proc)
              (if (progn
 	           (dolist (f (frame-list))
@@ -1835,9 +1833,7 @@ server-stop-automatically--handle-kill-terminal
 		         (delete-frame frame)
 		         nil)
 		     t))
-	         (let ((server-stop-automatically nil))
-	           (save-buffers-kill-emacs)
-	           (delete-frame frame))))))))
+	         (save-buffers-kill-emacs)))))))
 
 (defun server-stop-automatically--maybe-kill-emacs ()
   "Handle closing of Emacs daemon when `server-stop-automatically' is used."
-- 
2.25.1


[-- Attachment #6: 0005-This-is-the-commit-that-actually-fixes-bug-51993.patch --]
[-- Type: text/plain, Size: 3488 bytes --]

From 3730a0c55ed416ec007397be312c3223ebe078ad Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 1 Dec 2022 12:05:40 -0800
Subject: [PATCH 5/7] This is the commit that actually fixes bug#51993

There are two parts to this:

1. For 'nowait' frames, call 'save-some-buffers' and 'delete-frame'
   when there are other non-daemon frames.

2. For client frames (where the client is a process), don't try to
   delete all the client's frames directly.  Previously, this code
   would delete all *other* frames associated with the client, and
   then either a) delete the current frame if there are still more
   (non-daemon) frames remaining, or b) kill Emacs otherwise.  Now,
   check to see what we should do first: if there are no other clients
   (or frames not owned by the current client), then we should kill
   Emacs; otherwise, only delete this client (after saving).

* lisp/server.el (server-stop-automatically--handle-kill-terminal):
Handle the case when we *don't* kill Emacs, as described above.
---
 lisp/server.el | 41 ++++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 399bf694fd..9b91fbbbdc 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1817,23 +1817,34 @@ server-stop-automatically--handle-kill-terminal
   (when server-stop-automatically
     (let ((proc (frame-parameter frame 'client)))
       (cond ((eq proc 'nowait)
-             (if (null (cddr (frame-list)))
+             (if (cddr (frame-list))
+                 ;; If there are any other non-daemon frames, don't
+                 ;; kill Emacs, but *do* save some buffers, as in
+                 ;; `server-save-buffers-kill-terminal'.
+                 (progn
+                   (save-some-buffers)
+	           (delete-frame))
+                 ;; If we're the last non-daemon frame standing, kill
+                 ;; Emacs.
 	         (save-buffers-kill-emacs)))
             ((processp proc)
-             (if (progn
-	           (dolist (f (frame-list))
-		     (when (and (eq (frame-parameter frame 'client)
-			            (frame-parameter f 'client))
-			        (not (eq frame f)))
-		       (set-frame-parameter f 'client nil)
-		       (let ((server-stop-automatically nil))
-		         (delete-frame f))))
-	           (if (cddr (frame-list))
-		       (let ((server-stop-automatically nil))
-		         (delete-frame frame)
-		         nil)
-		     t))
-	         (save-buffers-kill-emacs)))))))
+             (if ;; Keep the server alive if...
+                 (or
+                  ;; a) there are any other clients, or...
+                  (length> server-clients 1)
+                  ;; b) there are any frames not owned by this client.
+                  (seq-some
+                   (lambda (frame)
+                     (when-let ((p (frame-parameter frame 'client)))
+                       (not (eq proc p))))
+                   (frame-list)))
+                 (let ((buffers (process-get proc 'buffers)))
+                   (save-some-buffers
+                    nil (if buffers
+                            ;; Only files from emacsclient file list.
+		            (lambda () (memq (current-buffer) buffers))))
+                   (server-delete-client))
+               (save-buffers-kill-emacs)))))))
 
 (defun server-stop-automatically--maybe-kill-emacs ()
   "Handle closing of Emacs daemon when `server-stop-automatically' is used."
-- 
2.25.1


[-- Attachment #7: 0006-Merge-kill-terminal-implementations.patch --]
[-- Type: text/plain, Size: 5747 bytes --]

From fe6ff62de3f18c2d8f9c0a0032fff39615380299 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 1 Dec 2022 12:19:44 -0800
Subject: [PATCH 6/7] Merge kill-terminal implementations

Now that 'server-stop-automatically--handle-kill-terminal' and
'server-save-buffers-kill-terminal' are almost the same, merge them
into a single function.

* lisp/server.el (server-stop-automatically--handle-kill-terminal):
Merge this...
(server-save-buffers-kill-terminal): ... into this.
---
 lisp/server.el | 92 +++++++++++++++++++-------------------------------
 1 file changed, 35 insertions(+), 57 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 9b91fbbbdc..6eeaa932a7 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1780,29 +1780,41 @@ server-save-buffers-kill-terminal
 
 If emacsclient was started with a list of filenames to edit, then
 only these files will be asked to be saved."
-  (if server-stop-automatically
-      (server-stop-automatically--handle-kill-terminal (selected-frame))
-    (let ((proc (frame-parameter nil 'client)))
-      (cond ((eq proc 'nowait)
-	     ;; Nowait frames have no client buffer list.
-	     (if (cdr (frame-list))
-	         (progn (save-some-buffers arg)
-		        (delete-frame))
-	       ;; If we're the last frame standing, kill Emacs.
-	       (save-buffers-kill-emacs arg)))
-	    ((processp proc)
-	     (let ((buffers (process-get proc 'buffers)))
-	       (save-some-buffers
-	        arg (if buffers
-                        ;; Only files from emacsclient file list.
-		        (lambda () (memq (current-buffer) buffers))
-                      ;; No emacsclient file list: don't override
-                      ;; `save-some-buffers-default-predicate' (unless
-                      ;; ARG is non-nil), since we're not killing
-                      ;; Emacs (unlike `save-buffers-kill-emacs').
-		      (and arg t)))
-	       (server-delete-client proc)))
-	    (t (error "Invalid client frame"))))))
+  (let ((proc (frame-parameter nil 'client)))
+    (cond ((eq proc 'nowait)
+	   ;; Nowait frames have no client buffer list.
+	   (if (length> (frame-list) (if server-stop-automatically 2 1))
+               ;; If there are any other frames, only delete this one.
+               ;; When `server-stop-automatically' is set, don't count
+               ;; the daemon frame.
+	       (progn (save-some-buffers arg)
+		      (delete-frame))
+	     ;; If we're the last frame standing, kill Emacs.
+	     (save-buffers-kill-emacs arg)))
+	  ((processp proc)
+           (if (or (not server-stop-automatically)
+                   (length> server-clients 1)
+                   (seq-some
+                    (lambda (frame)
+                      (when-let ((p (frame-parameter frame 'client)))
+                        (not (eq proc p))))
+                    (frame-list)))
+	       (let ((buffers (process-get proc 'buffers)))
+	         (save-some-buffers
+	          arg (if buffers
+                          ;; Only files from emacsclient file list.
+		          (lambda () (memq (current-buffer) buffers))
+                        ;; No emacsclient file list: don't override
+                        ;; `save-some-buffers-default-predicate' (unless
+                        ;; ARG is non-nil), since we're not killing
+                        ;; Emacs (unlike `save-buffers-kill-emacs').
+		        (and arg t)))
+	         (server-delete-client proc))
+             ;; If `server-stop-automatically' is set, there are no
+             ;; other client processes, and no other client frames
+             ;; (e.g. `nowait' frames), kill Emacs.
+             (save-buffers-kill-emacs arg)))
+	  (t (error "Invalid client frame")))))
 
 (defun server-stop-automatically--handle-delete-frame (frame)
   "Handle deletion of FRAME when `server-stop-automatically' is used."
@@ -1812,40 +1824,6 @@ server-stop-automatically--handle-delete-frame
 	  (save-buffers-kill-emacs)
 	  (delete-frame frame)))))
 
-(defun server-stop-automatically--handle-kill-terminal (frame)
-  "Handle deletion of FRAME when `server-stop-automatically' is used."
-  (when server-stop-automatically
-    (let ((proc (frame-parameter frame 'client)))
-      (cond ((eq proc 'nowait)
-             (if (cddr (frame-list))
-                 ;; If there are any other non-daemon frames, don't
-                 ;; kill Emacs, but *do* save some buffers, as in
-                 ;; `server-save-buffers-kill-terminal'.
-                 (progn
-                   (save-some-buffers)
-	           (delete-frame))
-                 ;; If we're the last non-daemon frame standing, kill
-                 ;; Emacs.
-	         (save-buffers-kill-emacs)))
-            ((processp proc)
-             (if ;; Keep the server alive if...
-                 (or
-                  ;; a) there are any other clients, or...
-                  (length> server-clients 1)
-                  ;; b) there are any frames not owned by this client.
-                  (seq-some
-                   (lambda (frame)
-                     (when-let ((p (frame-parameter frame 'client)))
-                       (not (eq proc p))))
-                   (frame-list)))
-                 (let ((buffers (process-get proc 'buffers)))
-                   (save-some-buffers
-                    nil (if buffers
-                            ;; Only files from emacsclient file list.
-		            (lambda () (memq (current-buffer) buffers))))
-                   (server-delete-client))
-               (save-buffers-kill-emacs)))))))
-
 (defun server-stop-automatically--maybe-kill-emacs ()
   "Handle closing of Emacs daemon when `server-stop-automatically' is used."
   (unless (cdr (frame-list))
-- 
2.25.1


[-- Attachment #8: 0007-Simplify-server-stop-automatically-handle-delete-fra.patch --]
[-- Type: text/plain, Size: 1263 bytes --]

From a79b4f34faf2c0bd30f8d66f87e4d84832477371 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 1 Dec 2022 12:23:17 -0800
Subject: [PATCH 7/7] Simplify 'server-stop-automatically--handle-delete-frame'

* lisp/server.el (server-stop-automatically--handle-delete-frame):
Combine the 'when' and 'if' into a single conditional.
---
 lisp/server.el | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/lisp/server.el b/lisp/server.el
index 6eeaa932a7..e88cffa8fb 100644
--- a/lisp/server.el
+++ b/lisp/server.el
@@ -1818,11 +1818,11 @@ server-save-buffers-kill-terminal
 
 (defun server-stop-automatically--handle-delete-frame (frame)
   "Handle deletion of FRAME when `server-stop-automatically' is used."
-  (when server-stop-automatically
-    (if (null (cddr (frame-list)))
-	(let ((server-stop-automatically nil))
-	  (save-buffers-kill-emacs)
-	  (delete-frame frame)))))
+  (when (and server-stop-automatically
+             (null (cddr (frame-list))))
+    (let ((server-stop-automatically nil))
+      (save-buffers-kill-emacs)
+      (delete-frame frame))))
 
 (defun server-stop-automatically--maybe-kill-emacs ()
   "Handle closing of Emacs daemon when `server-stop-automatically' is used."
-- 
2.25.1

bug#51993: 29.0.50; [PATCH explanation] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Eli Zaretskii]

> Date: Thu, 1 Dec 2022 17:42:18 -0800
> Cc: larsi@gnus.org, 51993@debbugs.gnu.org, gregory@heytings.org
> From: Jim Porter <jporterbugs@gmail.com>
> 
> Attached is a patch series that explains in more detail how I arrived at 
> the previous version of my patch. This is basically a reconstruction of 
> the steps I took when writing it originally. I'll describe my overall 
> plan and then address the specific comments you had after that. (This 
> might be overly-verbose, but I wanted to put as much detail in as I 
> could in the hopes of addressing all your concerns.)

Thanks, but unfortunately this methodology doesn't make it easier for me to
review the changes.

Personally, I'd prefer to have a single place with the logic of what happens
when the user closes the last client frame or the last frame on a specific
terminal.  But I'd like to make progress in this anyway, at least on master.
So I'm okay with having your code installed on master as it is, if that is
what you prefer.

bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files

[Jim Porter]

On 11/29/2021 4:41 AM, Eli Zaretskii wrote:
> However, I'm not sure I understand the way forward, as you see it.
> AFAIU, there are fundamental disagreements between you and Gregory,
> and I don't think the disagreements are there because one of you
> doesn't understand the proposals of the other.

I think there may be some of both. Since, from Gregory's perspective 
I've been changing what I want, and from my perspective I've always 
wanted the same thing, it seems that there's at least some 
miscommunication happening. Hopefully my proposal helps to explain my 
position more clearly (and in a single message, rather than scattered 
across several).

There may still be some disagreements, and I'm happy to incorporate 
anything I've missed into the proposal if people would find that useful. 
On the other hand, if it's easier to just work on a patch that everyone 
would be happy with (or at least be able to tolerate), that's ok too.

> So how could these disagreements be reconciled?  Either you-two come
> up with some compromise that is acceptable by both of you, or Lars and
> myself make the decision for you (and we don't guarantee you will like
> it), or things are left as they are now.  What will it be?

Hopefully Gregory and I can reach a compromise; the Implicit Shutdown 
case in my proposal is an initial attempt at incorporating Gregory's 
desired behavior (it was the behavior Gregory first proposed in the 
thread on emacs-devel). I'm primarily interested in the Explicit 
Shutdown case though; I may have missed some things in that section as 
well, but like I said, I'm happy to add more to it if anyone has things 
they'd like to be added.

That said, no matter if Gregory and I can reach a compromise, I think it 
would be helpful if you and/or Lars could take a look if you have time 
to be sure the behavior makes sense to you and that it's something you'd 
want to be in Emacs. Since this might have relevance for other Emacs 
features (e.g. emacsclient.desktop[1]), I think it would be good to have 
as many experienced eyes on this as possible.

In the end, so long as everyone is clear what the behavior should be, 
and that it's documented as such so there's no confusion about what's a 
bug and what's intended behavior, then I don't have any (major) 
problems. Obviously, I'd be happy if my preferred behavior were a part 
of Emacs, but if you and Lars don't think it should be, then I can just 
continue what I've been doing: add advice to the necessary server.el 
functions in my config so things work how I like.

[1] https://lists.gnu.org/archive/html/emacs-devel/2021-10/msg01846.html