master d5435a0d822 25/37: Refactor marker initialization in erc-open

[F. Jason Park]

branch: master
commit d5435a0d822e95bfb18f3cdf4fac83611ec17972
Author: F. Jason Park <jp@neverwas.me>
Commit: F. Jason Park <jp@neverwas.me>

    Refactor marker initialization in erc-open
    
    * lisp/erc/erc.el (erc--initialize-markers): New helper to ensure
    prompt and its associated markers are set up correctly.
    (erc-open): When determining whether a session is a logical
    continuation, leverage the work already performed by the
    `erc-networks' library to that effect.  Its verdicts are based on
    network context and thus reliable even when a user dials anew from an
    entry-point, which is not a simple reconnection because the user
    expects a clean slate for everything except an existing buffer's
    messages, meaning `erc--server-reconnecting' will be nil and
    local-module state variables need resetting.  Also remove the check
    for `erc-reuse-buffers' and instead trust that `erc-get-buffer-create'
    always does the right thing.  Replace all code involving marker and
    prompt setup by deferring to a new helper, `erc--initialize markers'.
    * test/lisp/erc/erc-scenarios-base-local-module-modes.el: New file.
    * test/lisp/erc/erc-scenarios-base-local-modules.el
    (erc-scenarios-base-local-modules--mode-persistence): Move test to
    separate file to help with parallel "-j" runs.
    * test/lisp/erc/erc-tests.el (erc-tests--send-prep): Replace
    redundant prompt-setup code.
    (erc--initialize-markers): New test.  (Bug#60936.)
---
 lisp/erc/erc.el                                    |  70 +++----
 .../erc/erc-scenarios-base-local-module-modes.el   | 211 +++++++++++++++++++++
 test/lisp/erc/erc-scenarios-base-local-modules.el  |  99 ----------
 test/lisp/erc/erc-tests.el                         |  79 +++++++-
 4 files changed, 322 insertions(+), 137 deletions(-)

diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 6e14f4780e4..e8bd4ace1a6 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -2051,6 +2051,35 @@ nil."
         (cons (nreverse (car out)) (nreverse (cdr out))))
     (list new-modes)))
 
+;; This function doubles as a convenient helper for use in unit tests.
+;; Prior to 5.6, its contents lived in `erc-open'.
+
+(defun erc--initialize-markers (old-point continued-session)
+  "Ensure prompt and its bounding markers have been initialized."
+  ;; FIXME erase assertions after code review and additional testing.
+  (setq erc-insert-marker (make-marker)
+        erc-input-marker (make-marker))
+  (if continued-session
+      (progn
+        ;; Trust existing markers.
+        (set-marker erc-insert-marker
+                    (alist-get 'erc-insert-marker continued-session))
+        (set-marker erc-input-marker
+                    (alist-get 'erc-input-marker continued-session))
+        (goto-char erc-insert-marker)
+        (cl-assert (= (field-end) erc-input-marker))
+        (goto-char old-point)
+        (erc--unhide-prompt))
+    (cl-assert (not (get-text-property (point) 'erc-prompt)))
+    ;; In the original version from `erc-open', the snippet that
+    ;; handled these newline insertions appeared twice close in
+    ;; proximity, which was probably unintended.  Nevertheless, we
+    ;; preserve the double newlines here for historical reasons.
+    (insert "\n\n")
+    (set-marker erc-insert-marker (point))
+    (erc-display-prompt)
+    (cl-assert (= (point) (point-max)))))
+
 (defun erc-open (&optional server port nick full-name
                            connect passwd tgt-list channel process
                            client-certificate user id)
@@ -2084,10 +2113,13 @@ Returns the buffer for the given server or channel."
          (old-recon-count erc-server-reconnect-count)
          (old-point nil)
          (delayed-modules nil)
-         (continued-session (and erc--server-reconnecting
-                                 (with-suppressed-warnings
-                                     ((obsolete erc-reuse-buffers))
-                                   erc-reuse-buffers))))
+         (continued-session (or erc--server-reconnecting
+                                erc--target-priors
+                                (and-let* (((not target))
+                                           (m (buffer-local-value
+                                               'erc-input-marker buffer))
+                                           ((marker-position m)))
+                                  (buffer-local-variables buffer)))))
     (when connect (run-hook-with-args 'erc-before-connect server port nick))
     (set-buffer buffer)
     (setq old-point (point))
@@ -2105,21 +2137,6 @@ Returns the buffer for the given server or channel."
             (buffer-local-value 'erc-server-announced-name old-buffer)))
     ;; connection parameters
     (setq erc-server-process process)
-    (setq erc-insert-marker (make-marker))
-    (setq erc-input-marker (make-marker))
-    ;; go to the end of the buffer and open a new line
-    ;; (the buffer may have existed)
-    (goto-char (point-max))
-    (forward-line 0)
-    (when (or continued-session (get-text-property (point) 'erc-prompt))
-      (setq continued-session t)
-      (set-marker erc-input-marker
-                  (or (next-single-property-change (point) 'erc-prompt)
-                      (point-max))))
-    (unless continued-session
-      (goto-char (point-max))
-      (insert "\n"))
-    (set-marker erc-insert-marker (point))
     ;; stack of default recipients
     (setq erc-default-recipients tgt-list)
     (when target
@@ -2166,20 +2183,7 @@ Returns the buffer for the given server or channel."
             (get-buffer-create (concat "*ERC-DEBUG: " server "*"))))
 
     (erc-determine-parameters server port nick full-name user passwd)
-
-    ;; FIXME consolidate this prompt-setup logic with the pass above.
-
-    ;; set up prompt
-    (unless continued-session
-      (goto-char (point-max))
-      (insert "\n"))
-    (if continued-session
-        (progn (goto-char old-point)
-               (erc--unhide-prompt))
-      (set-marker erc-insert-marker (point))
-      (erc-display-prompt)
-      (goto-char (point-max)))
-
+    (erc--initialize-markers old-point continued-session)
     (save-excursion (run-mode-hooks)
                     (dolist (mod (car delayed-modules)) (funcall mod +1))
                     (dolist (var (cdr delayed-modules)) (set var nil)))
diff --git a/test/lisp/erc/erc-scenarios-base-local-module-modes.el 
b/test/lisp/erc/erc-scenarios-base-local-module-modes.el
new file mode 100644
index 00000000000..7b91e28dc83
--- /dev/null
+++ b/test/lisp/erc/erc-scenarios-base-local-module-modes.el
@@ -0,0 +1,211 @@
+;;; erc-scenarios-base-local-module-modes.el --- More local-mod ERC tests -*- 
lexical-binding: t -*-
+
+;; Copyright (C) 2023 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/>.
+
+;;; Commentary:
+
+;; A local module doubles as a minor mode whose mode variable and
+;; associated local data can withstand service disruptions.
+;; Unfortunately, the current implementation is too unwieldy to be
+;; made public because it doesn't perform any of the boiler plate
+;; needed to save and restore buffer-local and "network-local" copies
+;; of user options.  Ultimately, a user-friendly framework must fill
+;; this void if third-party local modules are ever to become
+;; practical.
+;;
+;; The following tests all use `sasl' because, as of ERC 5.5, it's the
+;; only local module.
+
+;;; Code:
+
+(require 'ert-x)
+(eval-and-compile
+  (let ((load-path (cons (ert-resource-directory) load-path)))
+    (require 'erc-scenarios-common)))
+
+(require 'erc-sasl)
+
+;; After quitting a session for which `sasl' is enabled, you
+;; disconnect and toggle `erc-sasl-mode' off.  You then reconnect
+;; using an alternate nickname.  You again disconnect and reconnect,
+;; this time immediately, and the mode stays disabled.  Finally, you
+;; once again disconnect, toggle the mode back on, and reconnect.  You
+;; are authenticated successfully, just like in the initial session.
+;;
+;; This is meant to show that a user's local mode settings persist
+;; between sessions.  It also happens to show (in round four, below)
+;; that a server renicking a user on 001 after a 903 is handled just
+;; like a user-initiated renick, although this is not the main thrust.
+
+(ert-deftest erc-scenarios-base-local-module-modes--reconnect ()
+  :tags '(:expensive-test)
+  (erc-scenarios-common-with-cleanup
+      ((erc-scenarios-common-dialog "base/local-modules")
+       (erc-server-flood-penalty 0.1)
+       (dumb-server (erc-d-run "localhost" t 'first 'second 'third 'fourth))
+       (port (process-contact dumb-server :service))
+       (erc-modules (cons 'sasl erc-modules))
+       (expect (erc-d-t-make-expecter))
+       (server-buffer-name (format "127.0.0.1:%d" port)))
+
+    (ert-info ("Round one, initial authentication succeeds as expected")
+      (with-current-buffer (erc :server "127.0.0.1"
+                                :port port
+                                :nick "tester"
+                                :user "tester"
+                                :password "changeme"
+                                :full-name "tester")
+        (should (string= (buffer-name) server-buffer-name))
+        (funcall expect 10 "You are now logged in as tester"))
+
+      (with-current-buffer (erc-d-t-wait-for 10 (get-buffer "foonet"))
+        (funcall expect 10 "This server is in debug mode")
+        (erc-cmd-JOIN "#chan")
+
+        (with-current-buffer (erc-d-t-wait-for 10 (get-buffer "#chan"))
+          (funcall expect 20 "She is Lavinia, therefore must"))
+
+        (erc-cmd-QUIT "")
+        (funcall expect 10 "finished")))
+
+    (ert-info ("Round two, nick rejected, alternate granted")
+      (with-current-buffer "foonet"
+
+        (ert-info ("Toggle mode off, reconnect")
+          (erc-sasl-mode -1)
+          (erc-cmd-RECONNECT))
+
+        (funcall expect 10 "User modes for tester`")
+        (should-not (cdr (erc-scenarios-common-buflist "foonet")))
+        (should (equal (buffer-name) "foonet"))
+        (should-not (cdr (erc-scenarios-common-buflist "#chan")))
+
+        (with-current-buffer "#chan"
+          (funcall expect 10 "Some enigma, some riddle"))
+
+        (erc-cmd-QUIT "")
+        (funcall expect 10 "finished")))
+
+    (ert-info ("Round three, send alternate nick initially")
+      (with-current-buffer "foonet"
+
+        (ert-info ("Keep mode off, reconnect")
+          (should-not erc-sasl-mode)
+          (should (local-variable-p 'erc-sasl-mode))
+          (erc-cmd-RECONNECT))
+
+        (funcall expect 10 "User modes for tester`")
+        (should-not (cdr (erc-scenarios-common-buflist "foonet")))
+        (should (equal (buffer-name) "foonet"))
+        (should-not (cdr (erc-scenarios-common-buflist "#chan")))
+
+        (with-current-buffer "#chan"
+          (funcall expect 10 "Let our reciprocal vows be remembered."))
+
+        (erc-cmd-QUIT "")
+        (funcall expect 10 "finished")))
+
+    (ert-info ("Round four, authenticated successfully again")
+      (with-current-buffer "foonet"
+
+        (ert-info ("Toggle mode on, reconnect")
+          (should-not erc-sasl-mode)
+          (should (local-variable-p 'erc-sasl-mode))
+          (erc-sasl-mode +1)
+          (erc-cmd-RECONNECT))
+
+        (funcall expect 10 "User modes for tester")
+        (should-not (cdr (erc-scenarios-common-buflist "foonet")))
+        (should (equal (buffer-name) "foonet"))
+        (should-not (cdr (erc-scenarios-common-buflist "#chan")))
+
+        (with-current-buffer "#chan"
+          (funcall expect 10 "Well met; good morrow, Titus and Hortensius."))
+
+        (erc-cmd-QUIT "")))))
+
+;; In contrast to the mode-persistence test above, this one
+;; demonstrates that a user reinvoking an entry point declares their
+;; intention to reset local-module state for the server buffer.
+;; Whether a local-module's state variable is also reset in target
+;; buffers up to the module.  That is, by default, they're left alone.
+
+(ert-deftest erc-scenarios-base-local-module-modes--entrypoint ()
+  :tags '(:expensive-test)
+  (erc-scenarios-common-with-cleanup
+      ((erc-scenarios-common-dialog "base/local-modules")
+       (erc-server-flood-penalty 0.1)
+       (dumb-server (erc-d-run "localhost" t 'first 'first))
+       (port (process-contact dumb-server :service))
+       (erc-modules (cons 'sasl erc-modules))
+       (expect (erc-d-t-make-expecter))
+       (server-buffer-name (format "127.0.0.1:%d" port)))
+
+    (ert-info ("Round one, initial authentication succeeds as expected")
+      (with-current-buffer (erc :server "127.0.0.1"
+                                :port port
+                                :nick "tester"
+                                :user "tester"
+                                :password "changeme"
+                                :full-name "tester")
+        (should (string= (buffer-name) server-buffer-name))
+        (funcall expect 10 "You are now logged in as tester"))
+
+      (with-current-buffer (erc-d-t-wait-for 10 (get-buffer "foonet"))
+        (funcall expect 10 "This server is in debug mode")
+        (erc-cmd-JOIN "#chan")
+
+        (ert-info ("Toggle local-module off in target buffer")
+          (with-current-buffer (erc-d-t-wait-for 10 (get-buffer "#chan"))
+            (funcall expect 20 "She is Lavinia, therefore must")
+            (erc-sasl-mode -1)))
+
+        (erc-cmd-QUIT "")
+        (funcall expect 10 "finished")
+
+        (ert-info ("Toggle mode off")
+          (erc-sasl-mode -1)
+          (should (local-variable-p 'erc-sasl-mode)))))
+
+    (ert-info ("Reconnecting via entry point discards `erc-sasl-mode' value.")
+      ;; If you were to /RECONNECT here, no PASS changeme would be
+      ;; sent instead of CAP SASL, resulting in a failure.
+      (with-current-buffer (erc :server "127.0.0.1"
+                                :port port
+                                :nick "tester"
+                                :user "tester"
+                                :password "changeme"
+                                :full-name "tester")
+        (should (string= (buffer-name) server-buffer-name))
+        (funcall expect 10 "You are now logged in as tester")
+
+        (erc-d-t-wait-for 10 (equal (buffer-name) "foonet"))
+        (funcall expect 10 "User modes for tester")
+        (should erc-sasl-mode)) ; obviously
+
+      ;; No other foonet buffer exists, e.g., foonet<2>
+      (should-not (cdr (erc-scenarios-common-buflist "foonet")))
+
+      (ert-info ("Target buffer retains local-module state")
+        (with-current-buffer (erc-d-t-wait-for 10 (get-buffer "#chan"))
+          (funcall expect 20 "She is Lavinia, therefore must")
+          (should-not erc-sasl-mode)
+          (should (local-variable-p 'erc-sasl-mode))
+          (erc-cmd-QUIT ""))))))
+
+;;; erc-scenarios-base-local-module-modes.el ends here
diff --git a/test/lisp/erc/erc-scenarios-base-local-modules.el 
b/test/lisp/erc/erc-scenarios-base-local-modules.el
index 1318207a3bf..d6dbd87c8cc 100644
--- a/test/lisp/erc/erc-scenarios-base-local-modules.el
+++ b/test/lisp/erc/erc-scenarios-base-local-modules.el
@@ -82,105 +82,6 @@
         (erc-cmd-QUIT "")
         (funcall expect 10 "finished")))))
 
-;; After quitting a session for which `sasl' is enabled, you
-;; disconnect and toggle `erc-sasl-mode' off.  You then reconnect
-;; using an alternate nickname.  You again disconnect and reconnect,
-;; this time immediately, and the mode stays disabled.  Finally, you
-;; once again disconnect, toggle the mode back on, and reconnect.  You
-;; are authenticated successfully, just like in the initial session.
-;;
-;; This is meant to show that a user's local mode settings persist
-;; between sessions.  It also happens to show (in round four, below)
-;; that a server renicking a user on 001 after a 903 is handled just
-;; like a user-initiated renick, although this is not the main thrust.
-
-(ert-deftest erc-scenarios-base-local-modules--mode-persistence ()
-  :tags '(:expensive-test)
-  (erc-scenarios-common-with-cleanup
-      ((erc-scenarios-common-dialog "base/local-modules")
-       (erc-server-flood-penalty 0.1)
-       (dumb-server (erc-d-run "localhost" t 'first 'second 'third 'fourth))
-       (port (process-contact dumb-server :service))
-       (erc-modules (cons 'sasl erc-modules))
-       (expect (erc-d-t-make-expecter))
-       (server-buffer-name (format "127.0.0.1:%d" port)))
-
-    (ert-info ("Round one, initial authentication succeeds as expected")
-      (with-current-buffer (erc :server "127.0.0.1"
-                                :port port
-                                :nick "tester"
-                                :user "tester"
-                                :password "changeme"
-                                :full-name "tester")
-        (should (string= (buffer-name) server-buffer-name))
-        (funcall expect 10 "You are now logged in as tester"))
-
-      (with-current-buffer (erc-d-t-wait-for 10 (get-buffer "foonet"))
-        (funcall expect 10 "This server is in debug mode")
-        (erc-cmd-JOIN "#chan")
-
-        (with-current-buffer (erc-d-t-wait-for 10 (get-buffer "#chan"))
-          (funcall expect 20 "She is Lavinia, therefore must"))
-
-        (erc-cmd-QUIT "")
-        (funcall expect 10 "finished")))
-
-    (ert-info ("Round two, nick rejected, alternate granted")
-      (with-current-buffer "foonet"
-
-        (ert-info ("Toggle mode off, reconnect")
-          (erc-sasl-mode -1)
-          (erc-cmd-RECONNECT))
-
-        (funcall expect 10 "User modes for tester`")
-        (should-not (cdr (erc-scenarios-common-buflist "foonet")))
-        (should (equal (buffer-name) "foonet"))
-        (should-not (cdr (erc-scenarios-common-buflist "#chan")))
-
-        (with-current-buffer "#chan"
-          (funcall expect 10 "Some enigma, some riddle"))
-
-        (erc-cmd-QUIT "")
-        (funcall expect 10 "finished")))
-
-    (ert-info ("Round three, send alternate nick initially")
-      (with-current-buffer "foonet"
-
-        (ert-info ("Keep mode off, reconnect")
-          (should-not erc-sasl-mode)
-          (should (local-variable-p 'erc-sasl-mode))
-          (erc-cmd-RECONNECT))
-
-        (funcall expect 10 "User modes for tester`")
-        (should-not (cdr (erc-scenarios-common-buflist "foonet")))
-        (should (equal (buffer-name) "foonet"))
-        (should-not (cdr (erc-scenarios-common-buflist "#chan")))
-
-        (with-current-buffer "#chan"
-          (funcall expect 10 "Let our reciprocal vows be remembered."))
-
-        (erc-cmd-QUIT "")
-        (funcall expect 10 "finished")))
-
-    (ert-info ("Round four, authenticated successfully again")
-      (with-current-buffer "foonet"
-
-        (ert-info ("Toggle mode on, reconnect")
-          (should-not erc-sasl-mode)
-          (should (local-variable-p 'erc-sasl-mode))
-          (erc-sasl-mode +1)
-          (erc-cmd-RECONNECT))
-
-        (funcall expect 10 "User modes for tester")
-        (should-not (cdr (erc-scenarios-common-buflist "foonet")))
-        (should (equal (buffer-name) "foonet"))
-        (should-not (cdr (erc-scenarios-common-buflist "#chan")))
-
-        (with-current-buffer "#chan"
-          (funcall expect 10 "Well met; good morrow, Titus and Hortensius."))
-
-        (erc-cmd-QUIT "")))))
-
 ;; For local modules, the twin toggle commands `erc-FOO-enable' and
 ;; `erc-FOO-disable' affect all buffers of a connection, whereas
 ;; `erc-FOO-mode' continues to operate only on the current buffer.
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index b2f24aa718e..6e66de53edd 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -117,11 +117,7 @@
   ;; Caller should probably shadow `erc-insert-modify-hook' or
   ;; populate user tables for erc-button.
   (erc-mode)
-  (insert "\n\n")
-  (setq erc-input-marker (make-marker)
-        erc-insert-marker (make-marker))
-  (set-marker erc-insert-marker (point-max))
-  (erc-display-prompt)
+  (erc--initialize-markers (point) nil)
   (should (= (point) erc-input-marker)))
 
 (defun erc-tests--set-fake-server-process (&rest args)
@@ -257,6 +253,79 @@
       (kill-buffer "bob")
       (kill-buffer "ServNet"))))
 
+(ert-deftest erc--initialize-markers ()
+  (let ((proc (start-process "true" (current-buffer) "true"))
+        erc-modules
+        erc-connect-pre-hook
+        erc-insert-modify-hook
+        erc-kill-channel-hook erc-kill-server-hook erc-kill-buffer-hook)
+    (set-process-query-on-exit-flag proc nil)
+    (erc-mode)
+    (setq erc-server-process proc
+          erc-networks--id (erc-networks--id-create 'foonet))
+    (erc-open "localhost" 6667 "tester" "Tester" nil
+              "fake" nil "#chan" proc nil "user" nil)
+    (with-current-buffer (should (get-buffer "#chan"))
+      (should (= ?\n (char-after 1)))
+      (should (= ?E (char-after erc-insert-marker)))
+      (should (= 3 (marker-position erc-insert-marker)))
+      (should (= 8 (marker-position erc-input-marker)))
+      (should (= 8 (point-max)))
+      (should (= 8 (point)))
+      ;; These prompt properties are a continual source of confusion.
+      ;; Including the literal defaults here can hopefully serve as a
+      ;; quick reference for anyone operating in that area.
+      (should (equal (buffer-string)
+                     #("\n\nERC> "
+                       2 6 ( font-lock-face erc-prompt-face
+                             rear-nonsticky t
+                             erc-prompt t
+                             field erc-prompt
+                             front-sticky t
+                             read-only t)
+                       6 7 ( rear-nonsticky t
+                             erc-prompt t
+                             field erc-prompt
+                             front-sticky t
+                             read-only t))))
+
+      ;; Simulate some activity by inserting some text before and
+      ;; after the prompt (multiline).
+      (erc-display-error-notice nil "Welcome")
+      (goto-char (point-max))
+      (insert "Hello\nWorld")
+      (goto-char 3)
+      (should (looking-at-p (regexp-quote "*** Welcome"))))
+
+    (ert-info ("Reconnect")
+      (erc-open "localhost" 6667 "tester" "Tester" nil
+                "fake" nil "#chan" proc nil "user" nil)
+      (should-not (get-buffer "#chan<2>")))
+
+    (ert-info ("Existing prompt respected")
+      (with-current-buffer (should (get-buffer "#chan"))
+        (should (= ?\n (char-after 1)))
+        (should (= ?E (char-after erc-insert-marker)))
+        (should (= 15 (marker-position erc-insert-marker)))
+        (should (= 20 (marker-position erc-input-marker)))
+        (should (= 3 (point))) ; point restored
+        (should (equal (buffer-string)
+                       #("\n\n*** Welcome\nERC> Hello\nWorld"
+                         2 13 (font-lock-face erc-error-face)
+                         14 18 ( font-lock-face erc-prompt-face
+                                 rear-nonsticky t
+                                 erc-prompt t
+                                 field erc-prompt
+                                 front-sticky t
+                                 read-only t)
+                         18 19 ( rear-nonsticky t
+                                 erc-prompt t
+                                 field erc-prompt
+                                 front-sticky t
+                                 read-only t))))
+        (when noninteractive
+          (kill-buffer))))))
+
 (ert-deftest erc--switch-to-buffer ()
   (defvar erc-modified-channels-alist) ; lisp/erc/erc-track.el