From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: Eli Zaretskii Newsgroups: gmane.emacs.bugs Subject: bug#51993: 29.0.50; [PATCH] Killing emacsclient terminal with `server-stop-automatically' doesn't prompt to save files Date: Thu, 01 Dec 2022 19:29:31 +0200 Message-ID: <83lenrghhg.fsf@gnu.org> References: <9e47c871-a2c3-d764-bec9-d87abf3efe83@gmail.com> <79a53ecc-dbfc-d088-d80d-96f349be794a@gmail.com> <834k7vw2vb.fsf@gnu.org> <87czc4tosz.fsf@gnus.org> <83lepoi58p.fsf@gnu.org> <83y1tbxbbt.fsf@gnu.org> <83h6zxwujo.fsf@gnu.org> <2738f071-c87f-72cf-226f-6e8597cb07a8@gmail.com> <83a65ouz0e.fsf@gnu.org> Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="15265"; mail-complaints-to="usenet@ciao.gmane.io" Cc: larsi@gnus.org, 51993@debbugs.gnu.org, gregory@heytings.org To: Jim Porter Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Thu Dec 01 18:31:22 2022 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane-mx.org Original-Received: from lists.gnu.org ([209.51.188.17]) by ciao.gmane.io with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1p0nP7-0003jn-S0 for geb-bug-gnu-emacs@m.gmane-mx.org; Thu, 01 Dec 2022 18:31:22 +0100 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1p0nOt-0000A8-Qh; Thu, 01 Dec 2022 12:31:07 -0500 Original-Received: from eggs.gnu.org ([2001:470:142:3::10]) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p0nOo-00009r-U8 for bug-gnu-emacs@gnu.org; Thu, 01 Dec 2022 12:31:02 -0500 Original-Received: from debbugs.gnu.org ([209.51.188.43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1p0nOo-0002Kr-FH for bug-gnu-emacs@gnu.org; Thu, 01 Dec 2022 12:31:02 -0500 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1p0nOo-0008M3-6p for bug-gnu-emacs@gnu.org; Thu, 01 Dec 2022 12:31:02 -0500 X-Loop: help-debbugs@gnu.org Resent-From: Eli Zaretskii Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Thu, 01 Dec 2022 17:31:02 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 51993 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: patch Original-Received: via spool by 51993-submit@debbugs.gnu.org id=B51993.166991581232097 (code B ref 51993); Thu, 01 Dec 2022 17:31:02 +0000 Original-Received: (at 51993) by debbugs.gnu.org; 1 Dec 2022 17:30:12 +0000 Original-Received: from localhost ([127.0.0.1]:40962 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1p0nNx-0008Kq-VP for submit@debbugs.gnu.org; Thu, 01 Dec 2022 12:30:12 -0500 Original-Received: from eggs.gnu.org ([209.51.188.92]:37760) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1p0nNt-0008Jd-7T for 51993@debbugs.gnu.org; Thu, 01 Dec 2022 12:30:08 -0500 Original-Received: from fencepost.gnu.org ([2001:470:142:3::e]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p0nNn-0001jq-LE; Thu, 01 Dec 2022 12:29:59 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=gnu.org; s=fencepost-gnu-org; h=References:Subject:In-Reply-To:To:From:Date: mime-version; bh=/3Akg14VzA/dc7JweEXVYVl4ScPCAtDVcVE/g715zrg=; b=cR40Zg0s9fbc 7y7Pxu3J7VcR2SEaofqF94hCsIlnHWVZPJ6f5A73x8pPBpiwNq38mbQkjaYZF+U1DIyNjLOxBmXBE MGFD6Sh1i0NwKdMmwT75gvpq8wLXwpyM/wIGbqgPcUJ8K+HIyg3NrmMwGRObjmASTpfT5T4pRPZ9b bGKrORmsRP3FkgIV1jfQe+5b3sXZWrbDuS+tHwCwMcqRIm0qXNhmhF3/88o+ihYOa03cK2l+LzynU +9gzm8YKtlKItIki5KtuvTbm6OkAQlDbbV6onUJFDA5M79o0G3c0UyeBIFJS2AxibfxNBO2hxDaI5 dZzuiOcZVRTk4NdAH7aQ3w==; Original-Received: from [87.69.77.57] (helo=home-c4e4a596f7) by fencepost.gnu.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1p0nNm-00048u-Kc; Thu, 01 Dec 2022 12:29:59 -0500 In-Reply-To: (message from Jim Porter on Mon, 28 Nov 2022 21:31:02 -0800) X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Original-Sender: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Xref: news.gmane.io gmane.emacs.bugs:249656 Archived-At: > Date: Mon, 28 Nov 2022 21:31:02 -0800 > From: Jim Porter > 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.