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.