master da30a4908e 2/8: Don't set erc-networks--id until network is known

[Amin Bandali]

branch: master
commit da30a4908ec1482c6d86150a197655fb99f8d68a
Author: F. Jason Park <jp@neverwas.me>
Commit: Amin Bandali <bandali@gnu.org>

    Don't set erc-networks--id until network is known
    
    * lisp/erc/erc-networks.el (erc-networks--id-given): Accept a null
    argument.
    (erc-networks--id-on-connect): Remove unused function.
    (erc-networks--id-equal-p): Add method for comparing initialized and
    unset IDs.
    (erc-networks--update-server-identity): Ensure `erc-networks--id' is
    set before continuing search.
    (erc-networks--init-identity): Don't assume `erc-networks--id' is
    non-nil.  Add branch condition to reload ID on non-nil case.
    (erc-networks-on-MOTD-end): Let init-ID function handle renaming of
    server buffer.
    
    * lisp/erc/erc.el (erc-open): For continued sessions, try copying over
    the last network ID.
    (erc--auth-source-determine-params-default): Don't expect a network ID
    to have been initialized.
    (erc-set-current-nick): When connected, reload network ID.  Leave
    comment warning that it may be unneeded.
    
    * lisp/erc/erc-backend.el (erc-server-NICK, erc-server-433): Unless
    already connected, schedule ID reload when server rejects or mandates
    a nick change.
    
    * test/lisp/erc/erc-scenarios-base-association-nick.el
    (erc-scenarios-base-association-nick-bumped,
    erc-scenarios-base-association-nick-bumped-mandated-renick): Update to
    reflect more liberal association behavior when renamed by IRCd.
---
 lisp/erc/erc-backend.el                            |  6 +-
 lisp/erc/erc-networks.el                           | 53 ++++++--------
 lisp/erc/erc.el                                    | 21 ++++--
 .../erc/erc-scenarios-base-association-nick.el     | 84 ++++++++++++----------
 4 files changed, 90 insertions(+), 74 deletions(-)

diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index 15fd6ac50f..f899b866f0 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -1619,7 +1619,7 @@ add things to `%s' instead."
         (cl-pushnew (erc-server-buffer) bufs)
         (erc-set-current-nick nn)
         ;; Rename session, possibly rename server buf and all targets
-        (when (erc-network)
+        (when erc-server-connected
           (erc-networks--id-reload erc-networks--id proc parsed))
         (erc-update-mode-line)
         (setq erc-nick-change-attempt-count 0)
@@ -1629,6 +1629,8 @@ add things to `%s' instead."
          'NICK-you ?n nick ?N nn)
         (run-hook-with-args 'erc-nick-changed-functions nn nick))
        (t
+        (when erc-server-connected
+          (erc-networks--id-reload erc-networks--id proc parsed))
         (erc-handle-user-status-change 'nick (list nick login host) (list nn))
         (erc-display-message parsed 'notice bufs 'NICK ?n nick
                              ?u login ?h host ?N nn))))))
@@ -2255,6 +2257,8 @@ See `erc-display-server-message'." nil
 
 (define-erc-response-handler (433)
   "Login-time \"nick in use\"." nil
+  (when erc-server-connected
+    (erc-networks--id-reload erc-networks--id proc parsed))
   (erc-nickname-in-use (cadr (erc-response.command-args parsed))
                        "already in use"))
 
diff --git a/lisp/erc/erc-networks.el b/lisp/erc/erc-networks.el
index b3e5fcf1a3..19a7ab8643 100644
--- a/lisp/erc/erc-networks.el
+++ b/lisp/erc/erc-networks.el
@@ -826,12 +826,11 @@ respectively.  The separator is given by 
`erc-networks--id-sep'."
 
 ;; For now, please use this instead of `erc-networks--id-fixed-p'.
 (cl-defgeneric erc-networks--id-given (net-id)
-  "Return the preassigned identifier for a network presence, if any.
-This may have originated from an `:id' arg to entry-point commands
-`erc-tls' or `erc'.")
+  "Return the preassigned identifier for a network context, if any.
+When non-nil, assume NET-ID originated from an `:id' argument to
+entry-point commands `erc-tls' or `erc'.")
 
-(cl-defmethod erc-networks--id-given ((_ erc-networks--id))
-  nil)
+(cl-defmethod erc-networks--id-given (_) nil) ; _ may be nil
 
 (cl-defmethod erc-networks--id-given ((nid erc-networks--id-fixed))
   (erc-networks--id-symbol nid))
@@ -866,22 +865,15 @@ This may have originated from an `:id' arg to entry-point 
commands
   ((_ symbol) &context (erc-obsolete-var erc-reuse-buffers null))
   (erc-networks--id-fixed-create (intern (buffer-name))))
 
-(cl-defgeneric erc-networks--id-on-connect (net-id)
-  "Update NET-ID `erc-networks--id' after connection params known.
-This is typically during or just after MOTD.")
-
-(cl-defmethod erc-networks--id-on-connect ((_ erc-networks--id))
-  nil)
-
-(cl-defmethod erc-networks--id-on-connect ((id erc-networks--id-qualifying))
-  (erc-networks--id-qualifying-update id (erc-networks--id-qualifying-create)))
-
 (cl-defgeneric erc-networks--id-equal-p (self other)
-  "Return non-nil when two network identities exhibit underlying equality.
-SELF and OTHER are `erc-networks--id' struct instances.  This
-should normally be used only for ID recovery or merging, after
-which no two identities should be `equal' (timestamps aside) that
-aren't also `eq'.")
+  "Return non-nil when two network IDs exhibit underlying equality.
+Expect SELF and OTHER to be `erc-networks--id' struct instances
+and that this will only be called for ID recovery or merging,
+after which no two identities should be `equal' (timestamps
+aside) that aren't also `eq'.")
+
+(cl-defmethod erc-networks--id-equal-p ((_ null) (_ erc-networks--id)) nil)
+(cl-defmethod erc-networks--id-equal-p ((_ erc-networks--id) (_ null)) nil)
 
 (cl-defmethod erc-networks--id-equal-p ((self erc-networks--id)
                                         (other erc-networks--id))
@@ -1382,7 +1374,8 @@ considered as well because server buffers are often 
killed."
   (let* ((identity erc-networks--id)
          (buffer (current-buffer))
          (f (lambda ()
-              (unless (or (eq (current-buffer) buffer)
+              (unless (or (not erc-networks--id)
+                          (eq (current-buffer) buffer)
                           (eq erc-networks--id identity))
                 (if (erc-networks--id-equal-p identity erc-networks--id)
                     (throw 'buffer erc-networks--id)
@@ -1397,16 +1390,17 @@ considered as well because server buffers are often 
killed."
 ;; server buffer, whereas `erc-networks--rename-server-buffer' can run
 ;; mid-session, after an identity's core components have changed.
 
-(defun erc-networks--init-identity (_proc _parsed)
+(defun erc-networks--init-identity (proc parsed)
   "Update identity with real network name."
   ;; Initialize identity for real now that we know the network
   (cl-assert erc-network)
-  (unless (erc-networks--id-symbol erc-networks--id) ; unless just reconnected
-    (erc-networks--id-on-connect erc-networks--id))
-  ;; Find duplicate identities or other conflicting ones and act
-  ;; accordingly.
-  (erc-networks--update-server-identity)
-  ;;
+  (if erc-networks--id
+      (erc-networks--id-reload erc-networks--id proc parsed)
+    (setq erc-networks--id (erc-networks--id-create nil))
+    ;; Find duplicate identities or other conflicting ones and act
+    ;; accordingly.
+    (erc-networks--update-server-identity)
+    (erc-networks--rename-server-buffer proc parsed))
   nil)
 
 (defun erc-networks--rename-server-buffer (new-proc &optional _parsed)
@@ -1474,8 +1468,7 @@ This must run before `erc-server-connected' is set."
   ;; For now, retain compatibility with erc-server-NNN-functions.
   (or (erc-networks--ensure-announced proc parsed)
       (erc-networks--set-name proc parsed)
-      (erc-networks--init-identity proc parsed)
-      (erc-networks--rename-server-buffer proc parsed)))
+      (erc-networks--init-identity proc parsed)))
 
 (define-erc-module networks nil
   "Provide data about IRC networks."
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 2312246e3e..1052c8c4c0 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -2017,10 +2017,12 @@ Returns the buffer for the given server or channel."
     (setq erc-default-nicks (if (consp erc-nick) erc-nick (list erc-nick)))
     ;; client certificate (only useful if connecting over TLS)
     (setq erc-session-client-certificate client-certificate)
-    (setq erc-networks--id (if connect
-                               (erc-networks--id-create id)
-                             (buffer-local-value 'erc-networks--id
-                                                 old-buffer)))
+    (setq erc-networks--id
+          (if connect
+              (or (and continued-session
+                       (buffer-local-value 'erc-networks--id old-buffer))
+                  (and id (erc-networks--id-create id)))
+            (buffer-local-value 'erc-networks--id old-buffer)))
     ;; debug output buffer
     (setq erc-dbuf
           (when erc-log-p
@@ -3179,7 +3181,8 @@ node `(erc) Connecting'."
                  function))
 
 (defun erc--auth-source-determine-params-defaults ()
-  (let* ((net (and-let* ((esid (erc-networks--id-symbol erc-networks--id))
+  (let* ((net (and-let* ((erc-networks--id)
+                         (esid (erc-networks--id-symbol erc-networks--id))
                          ((symbol-name esid)))))
          (localp (and erc--target (erc--target-channel-local-p erc--target)))
          (hosts (if localp
@@ -5904,7 +5907,13 @@ strings over to the next call."
   (with-current-buffer (if (buffer-live-p (erc-server-buffer))
                            (erc-server-buffer)
                          (current-buffer))
-    (setq erc-server-current-nick nick)))
+    (unless (equal erc-server-current-nick nick)
+      (setq erc-server-current-nick nick)
+      ;; This seems sensible but may well be superfluous.  Should
+      ;; really prove that it's actually needed via test scenario.
+      (when erc-server-connected
+        (erc-networks--id-reload erc-networks--id)))
+    nick))
 
 (defun erc-current-nick ()
   "Return the current nickname."
diff --git a/test/lisp/erc/erc-scenarios-base-association-nick.el 
b/test/lisp/erc/erc-scenarios-base-association-nick.el
index 3e848be4df..b46c996bc0 100644
--- a/test/lisp/erc/erc-scenarios-base-association-nick.el
+++ b/test/lisp/erc/erc-scenarios-base-association-nick.el
@@ -25,13 +25,24 @@
 
 (eval-when-compile (require 'erc-join))
 
-;; You register a new nick, disconnect, and log back in, but your nick
-;; is not granted, so ERC obtains a backtick'd version.  You open a
-;; query buffer for NickServ, and ERC names it using the net-ID (which
-;; includes the backtick'd nick) as a suffix.  The original
-;; (disconnected) NickServ buffer gets renamed with *its* net-ID as
-;; well.  You then identify to NickServ, and the dead session is no
-;; longer considered distinct.
+;; You register a new nick in a dedicated query buffer, disconnect,
+;; and log back in, but your nick is not granted (maybe you just
+;; turned off SASL).  In any case, ERC obtains a backtick'd version.
+;; You open a query buffer for NickServ, and ERC gives you the
+;; existing one.  And after you identify, all buffers retain their
+;; names, although your net ID has changed internally.
+;;
+;; If ERC would've instead failed (or intentionally refused) to make
+;; the association, you would've ended up with a new NickServ buffer
+;; named after the new net ID as a suffix (based on the backtick'd
+;; nick), for example, NickServ@foonet/tester`.  And the original
+;; (disconnected) NickServ buffer would've gotten suffixed with *its*
+;; net-ID as well, e.g., NickServ@foonet/tester.  And after
+;; identifying, you would've seen ERC merge the two as well as their
+;; server buffers.  While this alternate behavior may arguably be a
+;; more honest reflection of reality, it's also quite inconvenient.
+;; For a clearer example, see the original version of this file
+;; introduced by "Add user-oriented test scenarios for ERC".
 
 (ert-deftest erc-scenarios-base-association-nick-bumped ()
   :tags '(:expensive-test)
@@ -67,30 +78,29 @@
           (funcall expect 5 "ERC finished"))))
 
     (with-current-buffer "foonet"
-      (erc-cmd-RECONNECT))
+      (erc-cmd-RECONNECT)
+      (funcall expect 10 "User modes for tester`"))
 
-    (erc-d-t-wait-for 10 "Nick request rejection prevents reassociation (good)"
-      (get-buffer "foonet/tester`"))
+    (ert-info ("Server buffer reassociated with new nick")
+      (should-not (get-buffer "foonet/tester`")))
 
     (ert-info ("Ask NickServ to change nick")
-      (with-current-buffer "foonet/tester`"
-        (funcall expect 3 "already in use")
+      (with-current-buffer "foonet"
         (funcall expect 3 "debug mode")
         (erc-cmd-QUERY "NickServ"))
 
-      (erc-d-t-wait-for 1 "Dead NickServ query buffer renamed, now qualified"
-        (get-buffer "NickServ@foonet/tester"))
+      (ert-info ( "NickServ buffer reassociated")
+        (should-not (get-buffer "NickServ@foonet/tester`"))
+        (should-not (get-buffer "NickServ@foonet/tester")))
 
-      (with-current-buffer "NickServ@foonet/tester`" ; new one
+      (with-current-buffer "NickServ" ; new one
         (erc-scenarios-common-say "IDENTIFY tester changeme")
-        (funcall expect 5 "You're now logged in as tester")
-        (ert-info ("Original buffer found, reused")
-          (erc-d-t-wait-for 2 (equal (buffer-name) "NickServ")))))
+        (funcall expect 5 "You're now logged in as tester")))
 
-    (ert-info ("Ours is the only NickServ buffer that remains")
+    (ert-info ("Still just one NickServ buffer")
       (should-not (cdr (erc-scenarios-common-buflist "NickServ"))))
 
-    (ert-info ("Visible network ID truncated to one component")
+    (ert-info ("As well as one server buffer")
       (should (not (get-buffer "foonet/tester`")))
       (should (not (get-buffer "foonet/tester")))
       (should (get-buffer "foonet")))))
@@ -135,29 +145,29 @@
     ;; Since we use reconnect, a new buffer won't be created
     ;; TODO add variant with clean `erc' invocation
     (with-current-buffer "foonet"
-      (erc-cmd-RECONNECT))
+      (erc-cmd-RECONNECT)
+      (funcall expect 10 "User modes for dummy"))
 
-    (ert-info ("Server-initiated renick")
-      (with-current-buffer (erc-d-t-wait-for 10 (get-buffer "foonet/dummy"))
-        (should-not (get-buffer "foonet/tester"))
-        (funcall expect 15 "debug mode"))
+    (ert-info ("Server-initiated renick associated correctly")
+      (with-current-buffer "foonet"
+        (funcall expect 15 "debug mode")
+        (should-not (get-buffer "foonet/dummy"))
+        (should-not (get-buffer "foonet/tester")))
 
-      (erc-d-t-wait-for 1 "Old query renamed, now qualified"
-        (get-buffer "bob@foonet/tester"))
+      (ert-info ("Old query reassociated")
+        (should (get-buffer "bob"))
+        (should-not (get-buffer "bob@foonet/tester"))
+        (should-not (get-buffer "bob@foonet/dummy")))
 
-      (with-current-buffer (erc-d-t-wait-for 5 (get-buffer "bob@foonet/dummy"))
+      (with-current-buffer "foonet"
         (erc-cmd-NICK "tester")
-        (ert-info ("Buffers combined")
-          (erc-d-t-wait-for 2 (equal (buffer-name) "bob")))))
+        (funcall expect 5 "You're now logged in as tester")))
 
-    (with-current-buffer "foonet"
-      (funcall expect 5 "You're now logged in as tester"))
-
-    (ert-info ("Ours is the only bob buffer that remains")
+    (ert-info ("Ours is still the only bob buffer that remains")
       (should-not (cdr (erc-scenarios-common-buflist "bob"))))
 
-    (ert-info ("Visible network ID truncated to one component")
-      (should (not (get-buffer "foonet/dummy")))
-      (should (get-buffer "foonet")))))
+    (ert-info ("Visible network ID still truncated to one component")
+      (should (not (get-buffer "foonet/tester")))
+      (should (not (get-buffer "foonet/dummy"))))))
 
 ;;; erc-scenarios-base-association-nick.el ends here