Don't kill server buffer with erc-kill-buffer-on-part

* etc/ERC-NEWS: Mention new flag `erc-killing-buffer-on-part-p' and
the renaming of `erc-kill-channel'.
* lisp/erc/erc-backend.el (erc-server-PART): Only kill a buffer on
behalf of `erc-kill-buffer-on-part' when the buffer hasn't already
been killed, and bind `erc-killing-buffer-on-part-p' to t when doing
so.
* lisp/erc/erc-log.el (erc-conditional-save-buffer): Don't save logs
when the buffer parameter is nil because that causes the server buffer
to be saved out.  It's possible that user code relying on this
longstanding bug will be affected, however, by default, the server
buffer will also be saved out independently at designated junctures.
* lisp/erc/erc.el (erc-part-hook): Redo doc string.
(erc-killing-buffer-on-part-p): New variable, a flag to prevent
redundant execution of `erc-kill-channel-hook' members concerned with
parted channels.
(erc-kill-buffer-on-part): Tweak doc string.
(erc-kill-channel-hook): Use new name for `erc-kill-channel',
`erc-part-channel-on-kill'.
(erc-kill-channel, erc-part-channel-on-kill): Rename former to latter,
and inhibit execution when `erc-killing-buffer-on-part-p' is non-nil.
* test/lisp/erc/erc-scenarios-base-kill-on-part.el: New file.
(Bug#70840)

etc/ERC-NEWS

@@ -648,6 +648,14 @@ 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.
 
+*** Function 'erc-kill-channel' renamed to 'erc-part-channel-on-kill'.
+This function, which normally emits a 'PART' when ERC kills a channel
+buffer, has been renamed for clarity.  Moreover, this and all other
+members of 'erc-kill-channel-hook' can now take comfort in knowing
+that the killing of buffers done on behalf of the option
+'erc-kill-buffer-on-part' has been made more detectable by the flag
+'erc-killing-buffer-on-part-p'.
+
 *** 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

lisp/erc/erc-backend.el

@@ -1883,6 +1883,9 @@ add things to `%s' instead."
          (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' and
+      ;; `erc-remove-channel-users' do almost nothing, and the message
+      ;; is displayed in the server buffer.
       (erc-remove-channel-member buffer nick)
       (erc-display-message parsed 'notice buffer
                            'PART ?n nick ?u login
@@ -1896,8 +1899,10 @@ add things to `%s' instead."
           (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-killing-buffer-on-part-p)
+          (let ((erc-killing-buffer-on-part-p t))
+            (kill-buffer buffer)))))))
 
 (define-erc-response-handler (PING)
   "Handle ping messages." nil

lisp/erc/erc-log.el

@@ -289,8 +289,8 @@ Return nil if BUFFER is a server buffer."
 				  (erc-save-buffer-in-logs)))
 
 (defun erc-conditional-save-buffer (buffer)
-  "Save Query BUFFER if `erc-save-queries-on-quit' is t."
-  (when erc-save-buffer-on-part
+  "Save channel BUFFER if it and `erc-save-buffer-on-part' are non-nil."
+  (when (and buffer erc-save-buffer-on-part)
     (erc-save-buffer-in-logs buffer)))
 
 (defun erc-conditional-save-queries (process)

lisp/erc/erc.el

@@ -470,13 +470,22 @@ See also `erc-server-QUIT-functions' and `erc-disconnected-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 a single argument: the channel buffer
+being parted.  For historical reasons, the buffer argument may be nil if
+it's been killed or otherwise can't be found.  This typically happens
+when the \"PART\" response being handled comes by way of a channel
+buffer being killed, which, by default, tells `erc-part-channel-on-kill'
+to emit a \"PART\".  Users needing to operate on a parted channel buffer
+before it's killed in this manner should use `erc-kill-channel-hook' and
+condition their code on `erc-killing-buffer-on-part-p' being nil."
   :group 'erc-hooks
   :type 'hook)
 
+(defvar erc-killing-buffer-on-part-p nil
+  "Non-nil when killing a target buffer while handling a \"PART\" response.
+Useful for preventing the redundant execution of code designed to run
+once when parting a channel.")
+
 (defcustom erc-kick-hook nil
   "Hook run when processing a KICK message directed at our nick.
 
@@ -1114,8 +1123,7 @@ directory in the list."
 
 (defcustom erc-kill-buffer-on-part nil
   "Kill the channel buffer on PART.
-This variable should probably stay nil, as ERC can reuse buffers if
-you rejoin them later."
+Nil by default because ERC can reuse buffers later re-joined."
   :group 'erc-quit-and-part
   :type 'boolean)
 
@@ -9598,7 +9606,7 @@ See also `format-spec'."
   :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 +9667,12 @@ This function should be on `erc-kill-server-hook'."
     (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)
+(define-obsolete-function-alias 'erc-kill-channel #'erc-part-channel-on-kill
+  "30.1")
+(defun erc-part-channel-on-kill ()
+  "Send a \"PART\" when killing a channel buffer."
+  (when (and (not erc-killing-buffer-on-part-p)
+             (erc-server-process-alive))
     (let ((tgt (erc-default-target)))
       (if tgt
          (erc-server-send (format "PART %s :%s" tgt

test/lisp/erc/erc-scenarios-base-kill-on-part.el

@@ -0,0 +1,95 @@
+;;; 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.  Also assert that code in
+;; `erc-kill-channel-hook' can detect when `erc-response-PART' is
+;; killing a buffer on behalf of that option.
+(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)))
+       (erc-kill-channel-hook
+        (cons (lambda () (push erc-killing-buffer-on-part-p calls))
+              erc-kill-channel-hook))
+       (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 '(t "#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)
+       (calls nil)
+       (erc-part-hook (lambda (b) (push 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")
+      (kill-buffer))
+
+    (erc-d-t-wait-for 20 (null (get-buffer "#chan")))
+    (erc-d-t-wait-for 10 (equal calls '(nil)))
+    (erc-d-t-ensure-for 0.1 (get-buffer "foonet"))))
+
+;;; erc-scenarios-base-kill-on-part.el ends here