bug#70840: 29.3; ERC 5.5.0.29.1: erc-kill-buffer-on-part kills server buffer

["J.P." <jp@neverwas.me>]

[-- Attachment #1: Type: text/plain, Size: 1699 bytes --]

"J.P." <jp@neverwas.me> writes:

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

I've checked `erc-log', and it indeed requires a tweak in order to
prevent a double save. The use case in `erc-status-sidebar' is that the
module wants `erc-status-sidebar-refresh' to run when parting. Luckily,
it doesn't care about the current buffer, so I'm somewhat confident that
it's safe to run whenever the user kills a live channel buffer.

> It's probably also worth doing the same with popular third-party
> packages and configs.

I've found a couple packages that operate in the affected areas. The
first one, sauron, uses `erc-server-PART-functions' instead of
`erc-part-hook', so that should be safe. The second, ZNC.el, adds advice
around `erc-kill-channel', inhibiting it when its own option is non-nil.
This too should be fine, since it also conditionally suppresses our
running `erc-part-hook'.

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

I've gone ahead with a provisional patch (attached) that incorporates
the points raised so far. Anyone interested, please take a look.

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-5.6-Don-t-kill-server-buffer-with-erc-kill-buffer-on.patch --]
[-- Type: text/x-patch, Size: 13569 bytes --]

From 98d16ec15baeb4803014033de6b1c1d3a488ad87 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Tue, 14 May 2024 21:09:55 -0700
Subject: [PATCH] [5.6] Don't kill server buffer with erc-kill-buffer-on-part

* etc/ERC-NEWS: Mention running `erc-part-hook' on
`erc-kill-channel-hook'.
* lisp/erc/erc-backend.el (erc-server-PART): When buffer doesn't
exist, only run hook when `erc--run-part-hook-with-nil-buffer-p' is
non-nil.  Bind that same variable to t when killing the buffer.
* lisp/erc/erc-log.el (erc-conditional-save-buffer): Don't save logs
when killing the channel buffer because this module already subscribes
to `erc-kill-channel-hook'.
* lisp/erc/erc.el (erc-part-hook): Redo doc string.
(erc--run-part-hook-with-nil-buffer-p): New variable, an escape hatch
to regain pre ERC 5.6 behavior regarding nil buffer arguments passed
to `erc-part-hook' members.
(erc-kill-channel-hook): Use new name for `erc-kill-channel'.
(erc--inhibit-part-channel-on-kill-p): New variable.
(erc-kill-channel, erc-part-channel-on-kill): Rename former to latter.
Run `erc-part-hook' when compat switch is nil.
* test/lisp/erc/erc-scenarios-base-kill-on-part.el: New file.
---
 etc/ERC-NEWS                                  |  10 ++
 lisp/erc/erc-backend.el                       |  13 +-
 lisp/erc/erc-log.el                           |   5 +-
 lisp/erc/erc.el                               |  36 +++--
 .../erc/erc-scenarios-base-kill-on-part.el    | 132 ++++++++++++++++++
 5 files changed, 183 insertions(+), 13 deletions(-)
 create mode 100644 test/lisp/erc/erc-scenarios-base-kill-on-part.el

diff --git a/etc/ERC-NEWS b/etc/ERC-NEWS
index b66ea6a7a02..9d5a26595be 100644
--- a/etc/ERC-NEWS
+++ b/etc/ERC-NEWS
@@ -648,6 +648,16 @@ release lacks a similar solution for detecting "joinedness" directly,
 but users can turn to 'xor'-ing 'erc-default-target' and 'erc-target'
 as a makeshift kludge.
 
+*** The function 'erc-kill-channel' may run the hook 'erc-part-hook'.
+Previously, when a user killed a channel buffer, 'erc-part-hook' would
+run in the response handler upon receipt of the corresponding 'PART'
+response, meaning hook members would receive a nil channel-buffer
+argument.  ERC now runs this hook immediately when a user kills a
+channel, just before sending the 'PART' command to the server.  Those
+disrupted by this change should see the definition of 'erc-part-hook'
+for a temporary escape hatch.  Also, for clarity, the function
+'erc-kill-channel' has been renamed 'erc-part-channel-on-kill'.
+
 *** Channel-mode handling has become stricter and more predictable.
 ERC has always processed channel modes using "standardized" letters
 and popular status prefixes.  Starting with this release, ERC will
diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index ab419d2b018..7e8e597da46 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -1883,12 +1883,17 @@ erc--server-determine-join-display-context
          (buffer (erc-get-buffer chnl proc)))
     (pcase-let ((`(,nick ,login ,host)
                  (erc-parse-user (erc-response.sender parsed))))
+      ;; When `buffer' is nil, `erc-remove-channel-member',
+      ;; `erc-remove-channel-users', and `erc-part-hook', don't run or
+      ;; do nothing, and the message is printed in the server buffer.
       (erc-remove-channel-member buffer nick)
       (erc-display-message parsed 'notice buffer
                            'PART ?n nick ?u login
                            ?h host ?c chnl ?r (or reason ""))
       (when (string= nick (erc-current-nick))
-        (run-hook-with-args 'erc-part-hook buffer)
+        (defvar erc--run-part-hook-with-nil-buffer-p)
+        (when (or buffer erc--run-part-hook-with-nil-buffer-p)
+          (run-hook-with-args 'erc-part-hook buffer))
         (erc-with-buffer
             (buffer)
           (erc-remove-channel-users))
@@ -1896,8 +1901,10 @@ erc--server-determine-join-display-context
           (erc-delete-default-channel chnl buffer))
         (erc-update-mode-line buffer)
         (defvar erc-kill-buffer-on-part)
-        (when erc-kill-buffer-on-part
-          (kill-buffer buffer))))))
+        (when (and erc-kill-buffer-on-part buffer)
+          (defvar erc--inhibit-part-channel-on-kill-p)
+          (let ((erc--inhibit-part-channel-on-kill-p t))
+            (kill-buffer buffer)))))))
 
 (define-erc-response-handler (PING)
   "Handle ping messages." nil
diff --git a/lisp/erc/erc-log.el b/lisp/erc/erc-log.el
index d5c56bcc2b3..f0bd1b45e7c 100644
--- a/lisp/erc/erc-log.el
+++ b/lisp/erc/erc-log.el
@@ -290,7 +290,10 @@ erc-save-query-buffers
 
 (defun erc-conditional-save-buffer (buffer)
   "Save Query BUFFER if `erc-save-queries-on-quit' is t."
-  (when erc-save-buffer-on-part
+  (when (and erc-save-buffer-on-part
+             ;; Don't run this from `erc-part-channel-on-kill' because
+             ;; `erc-save-buffer-in-logs' aready has it covered.
+             (not erc--inhibit-part-channel-on-kill-p))
     (erc-save-buffer-in-logs buffer)))
 
 (defun erc-conditional-save-queries (process)
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index c92fd42322a..a1c94288e8d 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -470,13 +470,23 @@ erc-quit-hook
 
 (defcustom erc-part-hook nil
   "Hook run when processing a PART message directed at our nick.
-
-The hook receives one argument, the current BUFFER.
-See also `erc-server-QUIT-functions', `erc-quit-hook' and
-`erc-disconnected-hook'."
+Called in the server buffer with one argument: the BUFFER being parted.
+Also runs via `erc-part-channel-on-kill' on `erc-kill-channel-hook' if
+killing BUFFER initiated the \"PART\".  Note that beginning with ERC
+5.6, this hook doesn't run upon receiving a \"PART\" from the server if
+BUFFER has already been killed.  See code near this option's definition
+for a possible escape hatch."
   :group 'erc-hooks
   :type 'hook)
 
+(defvar erc--run-part-hook-with-nil-buffer-p nil
+  "When non-nil, allow running `erc-part-hook' with a nil buffer argument.
+Moreover, forgo running the hook before sending a \"PART\" when the user
+kills a channel buffer.  This compat-oriented escape hatch exists for
+third-party code that expects to run unconditionally in the default
+response handler `erc-server-PART' when ERC receives a self-directed
+\"PART\" from the server.")
+
 (defcustom erc-kick-hook nil
   "Hook run when processing a KICK message directed at our nick.
 
@@ -9598,7 +9608,7 @@ erc-kill-server-hook
   :type 'hook)
 
 (defcustom erc-kill-channel-hook
-  '(erc-kill-channel
+  '(erc-part-channel-on-kill
     erc-networks-shrink-ids-and-buffer-names
     erc-networks-rename-surviving-target-buffer)
   "Invoked whenever a channel-buffer is killed via `kill-buffer'."
@@ -9659,10 +9669,18 @@ erc-kill-server
     (setq erc-server-quitting t)
     (erc-server-send (format "QUIT :%s" (funcall erc-quit-reason nil)))))
 
-(defun erc-kill-channel ()
-  "Sends a PART command to the server when the channel buffer is killed.
-This function should be on `erc-kill-channel-hook'."
-  (when (erc-server-process-alive)
+(defvar erc--inhibit-part-channel-on-kill-p nil)
+(define-obsolete-function-alias 'erc-kill-channel #'erc-part-channel-on-kill
+  "30.1")
+(defun erc-part-channel-on-kill ()
+  "Run `erc-part-hook' and send a \"PART\" when killing a channel buffer."
+  (when (and (erc-server-process-alive)
+             (not erc--inhibit-part-channel-on-kill-p))
+    (unless erc--run-part-hook-with-nil-buffer-p
+      (let ((buffer (current-buffer))
+            (erc--inhibit-part-channel-on-kill-p t))
+        (erc-with-server-buffer
+          (run-hook-with-args 'erc-part-hook buffer))))
     (let ((tgt (erc-default-target)))
       (if tgt
          (erc-server-send (format "PART %s :%s" tgt
diff --git a/test/lisp/erc/erc-scenarios-base-kill-on-part.el b/test/lisp/erc/erc-scenarios-base-kill-on-part.el
new file mode 100644
index 00000000000..109cf27b01a
--- /dev/null
+++ b/test/lisp/erc/erc-scenarios-base-kill-on-part.el
@@ -0,0 +1,132 @@
+;;; erc-scenarios-base-kill-on-part.el --- killing buffers on part -*- lexical-binding: t -*-
+
+;; Copyright (C) 2024 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert-x)
+(eval-and-compile
+  (let ((load-path (cons (ert-resource-directory) load-path)))
+    (require 'erc-scenarios-common)))
+
+;; Assert channel buffer is killed when `erc-kill-buffer-on-part' is
+;; enabled and a user issues a /part.
+(ert-deftest erc-scenarios-base-kill-on-part--enabled ()
+  :tags '(:expensive-test)
+  (should-not erc-kill-buffer-on-part)
+
+  (erc-scenarios-common-with-cleanup
+      ((erc-scenarios-common-dialog "base/reuse-buffers/channel")
+       (erc-server-flood-penalty 0.1)
+       (dumb-server (erc-d-run "localhost" t 'foonet))
+       (port (process-contact dumb-server :service))
+       (erc-kill-buffer-on-part t)
+       (calls nil)
+       (erc-part-hook (lambda (b) (push (buffer-name b) calls)))
+       (expect (erc-d-t-make-expecter)))
+
+    (ert-info ("Connect to foonet")
+      (with-current-buffer (erc :server "127.0.0.1"
+                                :port port
+                                :nick "tester"
+                                :password "foonet:changeme"
+                                :full-name "tester")
+        (funcall expect 10 "This server is in debug mode")))
+
+    (with-current-buffer (erc-d-t-wait-for 20 (get-buffer "#chan"))
+      (funcall expect 10 "<alice> bob: Whilst I can shake")
+      (erc-scenarios-common-say "/part"))
+
+    (erc-d-t-wait-for 20 (null (get-buffer "#chan")))
+    (should (equal calls '("#chan")))))
+
+;; When `erc-kill-buffer-on-part' is non-nil, and the parted buffer has
+;; already been killed, don't kill the server buffer.  Bug#70840
+(ert-deftest erc-scenarios-base-kill-on-part--enabled/killed ()
+  :tags '(:expensive-test)
+  (should-not erc-kill-buffer-on-part)
+
+  (erc-scenarios-common-with-cleanup
+      ((erc-scenarios-common-dialog "base/reuse-buffers/channel")
+       (erc-server-flood-penalty 0.1)
+       (dumb-server (erc-d-run "localhost" t 'foonet))
+       (port (process-contact dumb-server :service))
+       (erc-kill-buffer-on-part t)
+       (killingp nil)
+       (calls nil)
+       (erc-part-hook (lambda (b) (push (cons (buffer-name b) killingp)
+                                        calls)))
+       (expect (erc-d-t-make-expecter)))
+
+    (ert-info ("Connect to foonet")
+      (with-current-buffer (erc :server "127.0.0.1"
+                                :port port
+                                :nick "tester"
+                                :password "foonet:changeme"
+                                :full-name "tester")
+        (funcall expect 10 "This server is in debug mode")))
+
+    (with-current-buffer (erc-d-t-wait-for 20 (get-buffer "#chan"))
+      (funcall expect 10 "<alice> bob: Whilst I can shake")
+      ;; There's a race here because the response handler may run
+      ;; before `killingp' is nil, but at least false negatives aren't
+      ;; possible.  Shadowing `erc-server-send-queue' temporarily
+      ;; would fix it, but that's too leaky for this style of test.
+      (setq killingp 'kill)
+      (kill-buffer)
+      (setq killingp nil))
+
+    (erc-d-t-wait-for 20 (null (get-buffer "#chan")))
+    (should (equal calls '(("#chan" . kill))))
+    (erc-d-t-ensure-for 0.1 (get-buffer "foonet"))))
+
+(ert-deftest erc-scenarios-base-kill-on-part--enabled/killed/compat ()
+  :tags '(:expensive-test)
+  (should-not erc-kill-buffer-on-part)
+
+  (erc-scenarios-common-with-cleanup
+      ((erc-scenarios-common-dialog "base/reuse-buffers/channel")
+       (erc-server-flood-penalty 0.1)
+       (dumb-server (erc-d-run "localhost" t 'foonet))
+       (port (process-contact dumb-server :service))
+       (erc-kill-buffer-on-part t)
+       (erc--run-part-hook-with-nil-buffer-p t) ; option under test
+       (killingp nil)
+       (calls nil)
+       (erc-part-hook (lambda (b) (push (cons b killingp) calls)))
+       (expect (erc-d-t-make-expecter)))
+
+    (ert-info ("Connect to foonet")
+      (with-current-buffer (erc :server "127.0.0.1"
+                                :port port
+                                :nick "tester"
+                                :password "foonet:changeme"
+                                :full-name "tester")
+        (funcall expect 10 "This server is in debug mode")))
+
+    (with-current-buffer (erc-d-t-wait-for 20 (get-buffer "#chan"))
+      (funcall expect 10 "<alice> bob: Whilst I can shake")
+      (setq killingp 'kill)
+      (kill-buffer)
+      (setq killingp nil))
+
+    (erc-d-t-wait-for 20 (null (get-buffer "#chan")))
+    (erc-d-t-wait-for 20 (equal calls '((nil . nil))))
+    (should (get-buffer "foonet"))))
+
+;;; erc-scenarios-base-kill-on-part.el ends here
-- 
2.44.0