From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.io!.POSTED.blaine.gmane.org!not-for-mail From: "J.P." Newsgroups: gmane.emacs.bugs Subject: bug#70840: 29.3; ERC 5.5.0.29.1: erc-kill-buffer-on-part kills server buffer Date: Mon, 13 May 2024 17:55:58 -0700 Message-ID: <8734ql9ppd.fsf__41005.8959898137$1715648243$gmane$org@neverwas.me> References: <7hseysqbu1.fsf@thibaut.dev> Mime-Version: 1.0 Content-Type: text/plain Injection-Info: ciao.gmane.io; posting-host="blaine.gmane.org:116.202.254.214"; logging-data="28610"; mail-complaints-to="usenet@ciao.gmane.io" User-Agent: Gnus/5.13 (Gnus v5.13) Cc: emacs-erc@gnu.org, 70840@debbugs.gnu.org To: Thibaut Meyer Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane-mx.org@gnu.org Tue May 14 02:57:16 2024 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 1s6gTj-0007G3-Vw for geb-bug-gnu-emacs@m.gmane-mx.org; Tue, 14 May 2024 02:57:16 +0200 Original-Received: from localhost ([::1] helo=lists1p.gnu.org) by lists.gnu.org with esmtp (Exim 4.90_1) (envelope-from ) id 1s6gTY-0000I8-V8; Mon, 13 May 2024 20:57:04 -0400 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 1s6gTW-0000Hc-Pn for bug-gnu-emacs@gnu.org; Mon, 13 May 2024 20:57:02 -0400 Original-Received: from debbugs.gnu.org ([2001:470:142:5::43]) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1s6gTW-0000fs-BF for bug-gnu-emacs@gnu.org; Mon, 13 May 2024 20:57:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1s6gTX-00042o-1g for bug-gnu-emacs@gnu.org; Mon, 13 May 2024 20:57:03 -0400 X-Loop: help-debbugs@gnu.org Resent-From: "J.P." Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Tue, 14 May 2024 00:57:03 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 70840 X-GNU-PR-Package: emacs Original-Received: via spool by 70840-submit@debbugs.gnu.org id=B70840.171564817015413 (code B ref 70840); Tue, 14 May 2024 00:57:03 +0000 Original-Received: (at 70840) by debbugs.gnu.org; 14 May 2024 00:56:10 +0000 Original-Received: from localhost ([127.0.0.1]:35903 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1s6gSf-00040V-Up for submit@debbugs.gnu.org; Mon, 13 May 2024 20:56:10 -0400 Original-Received: from mail-108-mta85.mxroute.com ([136.175.108.85]:44921) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1s6gSb-000408-Q4 for 70840@debbugs.gnu.org; Mon, 13 May 2024 20:56:08 -0400 Original-Received: from filter006.mxroute.com ([136.175.111.2] filter006.mxroute.com) (Authenticated sender: mN4UYu2MZsgR) by mail-108-mta85.mxroute.com (ZoneMTA) with ESMTPSA id 18f74996b1b0008ca2.001 for <70840@debbugs.gnu.org> (version=TLSv1.3 cipher=TLS_AES_256_GCM_SHA384); Tue, 14 May 2024 00:56:02 +0000 X-Zone-Loop: 6f8d30b81856edb415fa283736d16b38b51d7c1026a4 X-Originating-IP: [136.175.111.2] DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=neverwas.me ; s=x; h=Content-Type:MIME-Version:Message-ID:Date:References:In-Reply-To: Subject:Cc:To:From:Sender:Reply-To:Content-Transfer-Encoding:Content-ID: Content-Description:Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc :Resent-Message-ID:List-Id:List-Help:List-Unsubscribe:List-Subscribe: List-Post:List-Owner:List-Archive; bh=TMUg0g7UsK/zubUwFXAz6MtPW1NTqAZb4GhmdJQojqI=; b=Mr/UbjuMkutvSE4M7xSnBq7uRH 9DIRl0tNYbFjW2+ohP8QauqhjbxJtiVW4r360+VU5gR9bFCrHjRtyhjWLP6/O4l8+ozf66u7zkjJR obJL+eeZtH2hNBaYesXoXeH5oe0xv0jOJt9G1auJpScz6F2si9zKo9ewo14vtU3KN83E15Dxb/c74 rcy7RRRy4j2f5/i6F3ues/v/E6PMJ18Bk9mk+cMDF6nF1ca5RTnVYL3pRjenCuwziW0Fd8uxAYzy6 +LGyFDLWoVIb56PYqfnAL4YCEaQbwC+nrEzR0kNfDStMwOFB7/CRrQOKRg1qDRLNrc7wFDscWLGc1 tEwKm7Mg==; In-Reply-To: <7hseysqbu1.fsf@thibaut.dev> (Thibaut Meyer via General discussion about's message of "Wed, 08 May 2024 22:37:42 +0200") X-Authenticated-Id: masked@neverwas.me 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:284984 Archived-At: Hi Thibaut, Thibaut Meyer writes: > Hello, > reminiscent of > https://lists.gnu.org/archive/html/emacs-devel/2009-06/msg00064.html, > when `erc-kill-buffer-on-part` is set to t, parting a channel > by killing its buffer, makes erc want to kill the server's buffer. When > parting by issuing the "/part" command, everything is fine. > > I reckon that in `define-erc-response-handler` in the erc-backend.el > file, since we kill the buffer ourselves, (erc-get-buffer chnl proc) > returns the server buffer instead of the channel buffer that has already > been killed. So then it tries to kill that instead, which is not wanted. I believe calling `erc-get-buffer' with a nonexistent target returns nil, which has a similar effect to what you describe. For example, it causes `erc-display-message' to fall back on routing its output to the server buffer. Likewise, in terms of user bookkeeping, the calls to `erc-remove-channel-users' and `erc-remove-channel-member' are effectively suppressed by `buffer' being nil. This is fine because the parted buffer has already been killed, so its `erc-channel-users' table no longer exists, and the buffer has already been removed from the `buffers' slot of the user's `erc-server-users' entry via kill-buffer-hook -> erc-kill-buffer-function -> erc-remove-channel-users -> erc-remove-channel-user -> erc-remove-server-user > To reproduce: > - set erc-kill-buffer-on-part to t > - kill a channel buffer > - notice emacs trying to kill the corresponding server buffer (it should > not happen directly thanks to the "buffer has running process. Kill it?" > confirmation though) Yes, I can confirm this to be the case. Thanks. So, without thinking about this too seriously, I'm guessing the main challenge to overcome when fixing this will likely be `erc-part-hook' and whether to gate it behind the parted buffer being non-nil. Since the corner case of it being called with a nil buffer has been around forever, some users may already account for the possibility. Others who don't may have code that simply doesn't care and yet still needs to run whenever an incoming (self-issued) PART response arrives. However, since this is a clear bug, we're justified in changing the behavior. The question is whether improving it for new users will cause more harm overall. If we do elect to only run the hook in the response handler when the parted buffer still exists, we may want to compensate by running it elsewhere in the code path triggered by the killing of a channel buffer. One way to do this would be via a new function running just before `erc-kill-channel' on `erc-kill-channel-hook'. We'd run `erc-part-hook' there, in the server buffer, passing it the still-live target buffer that's slated to be killed. Such an addition would require us checking how `erc-kill-channel-hook' is used in erc-log.el and erc-status-sidebar.el, etc., and confirming that running `erc-part-hook' in this fashion (within another hook) won't cause any problems. It's probably also worth doing the same with popular third-party packages and configs. We'd of course also have to announce the change in etc/ERC-NEWS. One approach to cutting down on any associated churn would be to add an escape hatch, such as an "internal" compatibility switch in the form of a global variable, like an `erc--run-part-hook-with-nil-buffer-p' or similar. Basically, we'd condition both hook sites on its value. For example, in `erc-server-PART', we'd have something like (when (or buffer erc--run-part-hook-with-nil-buffer-p) (run-hook-with-args 'erc-part-hook buffer)) And in the new `erc-kill-channel-hook' member, we'd have (unless erc--run-part-hook-with-nil-buffer-p (let ((buffer (current-buffer))) (erc-with-server-buffer (run-hook-with-args 'erc-part-hook buffer)))) This would grant a temporary reprieve to users transitioning to the latest version. They could just set the variable to put off dealing with it immediately. BTW, if going this route, the doc string for the hook should probably mention the second call site and possibly the existence of an escape hatch. There may well be other issues here I've not yet considered, so if you can think of any, please share. Also, if you're up for taking this on (not necessarily in the way I've described), please feel free to send a patch. The same goes for anyone else out there. I can definitely help with the leg work and test coverage if that makes things easier. Otherwise, I may not be able to get to this in time for the next release (not that there's any rush). Thanks, J.P.