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

[Jim Porter <jporterbugs@gmail.com>]

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