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/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.

29.1--0001-Make-killing-a-non-last-client-work-the-same-no-matt.patch
Description: Text document

master--0002-Remove-dead-code-from-server-stop-automatically-hand.patch
Description: Text document