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.