bug#67220: 30.0.50; ERC 5.6: Prefer parameter-driven MODE processing in ERC

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

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

This commit

  e69bd59ec59784b2f646e93355d4d63f41426cfc
  
  Honor arbitrary CHANTYPES in ERC
  
  * lisp/erc/erc.el (erc-channel-p): Favor "CHANTYPES" ISUPPORT item
  before falling back to well known prefixes.
  * test/lisp/erc/erc-tests.el (erc-channel-p): Add test.  Arbitrarily
  bundled with bug#60954.

introduced "smarter" handling of CHANTYPES but overlooked a subtlety
regarding how ERC interprets empty vs. missing ISUPPORT values. As
implied in a comment for the function `erc--parse-isupport-value',

  ;; https://tools.ietf.org/html/draft-brocklesby-irc-isupport-03#section-2
  ;;
  ;; > The server SHOULD send "X", not "X="; this is the normalized form.
  ;;
  ;; Note: for now, assume the server will only send non-empty values,

ERC punts on this. Indeed, it's always treated "X=" as having a value
and thus deserving of ("X" . "") in `erc-server-parameters', whereas
it's always seen "X" as more of a flag/switch with no associated value,
hence ("X").

It turns out a not entirely frivolous use case for abiding by that RFC
draft and *not* distinguishing between the two forms has arisen.
Basically, a server may choose to support no channels whatsoever for a
subset of clients, limiting them to direct messages only. To accommodate
this, ERC will need to interpret both "CHANTYPES" and "CHANTYPES=" as
expressing such a policy instead of sticking with its current behavior
of only doing so for the "=" form and treating "CHANTYPES" as equivalent
to ${default/fallback} (and thus also to "-CHANTYPES", which is clearly
wrong).

I think it's worth correcting this in ERC 5.6. Proposed changes
attached. (The first patch is unrelated.)

Thanks.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-5.6-Ignore-the-TGT-LIST-parameter-in-erc-open.patch --]
[-- Type: text/x-patch, Size: 3354 bytes --]

From 20aed6e1b7ab514669a3980f6b5d96d655e1b851 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sun, 11 Feb 2024 20:42:18 -0800
Subject: [PATCH 1/3] [5.6] Ignore the TGT-LIST parameter in erc-open

* lisp/erc/erc.el (erc-open): Set `erc-default-recipients' to a list
containing only the supplied target.  Any other value may cause ERC to
malfunction.  Also redo doc string.
---
 lisp/erc/erc.el | 39 ++++++++++++++++-----------------------
 1 file changed, 16 insertions(+), 23 deletions(-)

diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 08dfa4b8f1b..45869f43c91 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -2479,29 +2479,22 @@ erc--initialize-markers
     (cl-assert (= (point) (point-max)))))
 
 (defun erc-open (&optional server port nick full-name
-                           connect passwd tgt-list channel process
+                           connect passwd _tgt-list channel process
                            client-certificate user id)
-  "Connect to SERVER on PORT as NICK with USER and FULL-NAME.
-
-If CONNECT is non-nil, connect to the server.  Otherwise assume
-already connected and just create a separate buffer for the new
-target given by CHANNEL, meaning these parameters are mutually
-exclusive.  Note that CHANNEL may also be a query; its name has
-been retained for historical reasons.
-
-Use PASSWD as user password on the server.  If TGT-LIST is
-non-nil, use it to initialize `erc-default-recipients'.
-
-CLIENT-CERTIFICATE, if non-nil, should either be a list where the
-first element is the file name of the private key corresponding
-to a client certificate and the second element is the file name
-of the client certificate itself to use when connecting over TLS,
-or t, which means that `auth-source' will be queried for the
-private key and the certificate.
-
-When non-nil, ID should be a symbol for identifying the connection.
-
-Returns the buffer for the given server or channel."
+  "Return a new or reinitialized server or target buffer.
+If CONNECT is non-nil, connect to SERVER and return its new or
+reassociated buffer.  Otherwise, assume PROCESS is non-nil and belongs
+to an active session, and return a new or refurbished target buffer for
+CHANNEL, which may also be a query target (the parameter name remains
+for historical reasons).  Pass SERVER, PORT, NICK, USER, FULL-NAME, and
+PASSWD to `erc-determine-parameters' for preserving as session-local
+variables.  Do something similar for CLIENT-CERTIFICATE and ID, which
+should be as described by `erc-tls'.
+
+Note that ERC ignores TGT-LIST and initializes `erc-default-recipients'
+with CHANNEL as its only member.  Note also that this function has the
+side effect of setting the current buffer to the one it returns.  Use
+`with-current-buffer' or `save-excursion' to nullify this effect."
   (let* ((target (and channel (erc--target-from-string channel)))
          (buffer (erc-get-buffer-create server port nil target id))
          (old-buffer (current-buffer))
@@ -2538,7 +2531,7 @@ erc-open
     ;; connection parameters
     (setq erc-server-process process)
     ;; stack of default recipients
-    (setq erc-default-recipients tgt-list)
+    (when channel (setq erc-default-recipients (list channel)))
     (when target
       (setq erc--target target
             erc-network (erc-network)))
-- 
2.43.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-5.6-Normalize-ISUPPORT-params-with-empty-values-in-E.patch --]
[-- Type: text/x-patch, Size: 7296 bytes --]

From fbfc31ab2f1675136ecbe4c030606f031d8bec90 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sun, 11 Feb 2024 17:15:14 -0800
Subject: [PATCH 2/3] [5.6] Normalize ISUPPORT params with empty values in ERC

* lisp/erc/erc-backend.el (erc-server-parameters)
(erc--isupport-params): Mention parsing and storage behavior regarding
nonstandard "FOO=" tokens.
(erc--parse-isupport-value): Move comment closer to code.
(erc--get-isupport-entry): Treat the empty string as truly null, as
prescribed by the Brocklesby draft cited in the top-level comment.
* test/lisp/erc/erc-tests.el (erc--get-isupport-entry): Add case for
the empty string appearing as a value for an `erc-server-parameters'
item.
(erc-server-005): Assert compat-related behavior of retaining the
empty string as a valid value from a raw "FOO=" token.
---
 lisp/erc/erc-backend.el    | 20 ++++++++++++--------
 test/lisp/erc/erc-tests.el | 26 ++++++++++++++++++--------
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index e379066b08e..2c6da90890b 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -254,6 +254,10 @@ erc-server-parameters
 or
   (PARAMETER) if no value is provided.
 
+where PARAMETER is a string and VALUE is a string or nil.  For
+compatibility, a raw parameter of the form \"FOO=\" becomes
+(\"FOO\" . \"\") even though it's equivalent to \"FOO\" => (\"FOO\").
+
 Some examples of possible parameters sent by servers:
 CHANMODES=b,k,l,imnpst - list of supported channel modes
 CHANNELLEN=50 - maximum length of channel names
@@ -273,7 +277,8 @@ erc-server-parameters
 (defvar-local erc--isupport-params nil
   "Hash map of \"ISUPPORT\" params.
 Keys are symbols.  Values are lists of zero or more strings with hex
-escapes removed.")
+escapes removed.  ERC normalizes incoming parameters of the form
+\"FOO=\" to (FOO).")
 
 ;;; Server and connection state
 
@@ -2150,10 +2155,6 @@ erc--parse-isupport-value
   ;;
   ;; > The server SHOULD send "X", not "X="; this is the normalized form.
   ;;
-  ;; Note: for now, assume the server will only send non-empty values,
-  ;; possibly with printable ASCII escapes.  Though in practice, the
-  ;; only two escapes we're likely to see are backslash and space,
-  ;; meaning the pattern is too liberal.
   (let (case-fold-search)
     (mapcar
      (lambda (v)
@@ -2164,7 +2165,9 @@ erc--parse-isupport-value
                      (string-match "[\\]x[0-9A-F][0-9A-F]" v start))
            (setq m (substring v (+ 2 (match-beginning 0)) (match-end 0))
                  c (string-to-number m 16))
-           (if (<= ?\  c ?~)
+           ;; In practice, this range is too liberal.  The only
+           ;; escapes we're likely to see are ?\\, ?=, and ?\s.
+           (if (<= ?\s c ?~)
                (setq v (concat (substring v 0 (match-beginning 0))
                                (string c)
                                (substring v (match-end 0)))
@@ -2189,8 +2192,9 @@ erc--get-isupport-entry
                                           (or erc-server-parameters
                                               (erc-with-server-buffer
                                                 erc-server-parameters)))))
-                       (if (cdr v)
-                           (erc--parse-isupport-value (cdr v))
+                       (if-let ((vv (cdr v))
+                                ((not (string-empty-p vv))))
+                           (erc--parse-isupport-value vv)
                          '--empty--)))))
       (pcase value
         ('--empty-- (unless single (list key)))
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 7d189d37929..827bd9435e1 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1054,7 +1054,8 @@ erc--parse-isupport-value
 
 (ert-deftest erc--get-isupport-entry ()
   (let ((erc--isupport-params (make-hash-table))
-        (erc-server-parameters '(("FOO" . "1") ("BAR") ("BAZ" . "A,B,C")))
+        (erc-server-parameters '(("FOO" . "1") ("BAR") ("BAZ" . "A,B,C")
+                                 ("SPAM" . "")))
         (items (lambda ()
                  (cl-loop for k being the hash-keys of erc--isupport-params
                           using (hash-values v) collect (cons k v)))))
@@ -1075,7 +1076,9 @@ erc--get-isupport-entry
     (should (equal (erc--get-isupport-entry 'FOO) '(FOO "1")))
 
     (should (equal (funcall items)
-                   '((BAR . --empty--) (BAZ "A" "B" "C") (FOO "1"))))))
+                   '((BAR . --empty--) (BAZ "A" "B" "C") (FOO "1"))))
+    (should (equal (erc--get-isupport-entry 'SPAM) '(SPAM)))
+    (should-not (erc--get-isupport-entry 'SPAM 'single))))
 
 (ert-deftest erc-server-005 ()
   (let* ((hooked 0)
@@ -1093,34 +1096,41 @@ erc-server-005
                (lambda (_ _ _ line) (push line calls))))
 
       (ert-info ("Baseline")
-        (setq args '("tester" "BOT=B" "EXCEPTS" "PREFIX=(ov)@+" "are supp...")
+        (setq args '("tester" "BOT=B" "CHANTYPES=" "EXCEPTS" "PREFIX=(ov)@+"
+                     "are supp...")
               parsed (make-erc-response :command-args args :command "005"))
 
         (setq verify
               (lambda ()
                 (should (equal erc-server-parameters
                                '(("PREFIX" . "(ov)@+") ("EXCEPTS")
+                                 ;; Should be ("CHANTYPES") but
+                                 ;; retained for compatibility.
+                                 ("CHANTYPES" . "")
                                  ("BOT" . "B"))))
                 (should (zerop (hash-table-count erc--isupport-params)))
                 (should (equal "(ov)@+" (erc--get-isupport-entry 'PREFIX t)))
                 (should (equal '(EXCEPTS) (erc--get-isupport-entry 'EXCEPTS)))
                 (should (equal "B" (erc--get-isupport-entry 'BOT t)))
-                (should (string= (pop calls)
-                                 "BOT=B EXCEPTS PREFIX=(ov)@+ are supp..."))
+                (should (string=
+                         (pop calls)
+                         "BOT=B CHANTYPES= EXCEPTS PREFIX=(ov)@+ are supp..."))
                 (should (equal args (erc-response.command-args parsed)))))
 
         (erc-call-hooks nil parsed))
 
       (ert-info ("Negated, updated")
-        (setq args '("tester" "-EXCEPTS" "-FAKE" "PREFIX=(ohv)@%+" "are su...")
+        (setq args '("tester" "-EXCEPTS" "-CHANTYPES" "-FAKE" "PREFIX=(ohv)@%+"
+                     "are su...")
               parsed (make-erc-response :command-args args :command "005"))
 
         (setq verify
               (lambda ()
                 (should (equal erc-server-parameters
                                '(("PREFIX" . "(ohv)@%+") ("BOT" . "B"))))
-                (should (string= (pop calls)
-                                 "-EXCEPTS -FAKE PREFIX=(ohv)@%+ are su..."))
+                (should (string-prefix-p
+                         "-EXCEPTS -CHANTYPES -FAKE PREFIX=(ohv)@%+ "
+                         (pop calls)))
                 (should (equal "(ohv)@%+" (erc--get-isupport-entry 'PREFIX t)))
                 (should (equal "B" (erc--get-isupport-entry 'BOT t)))
                 (should-not (erc--get-isupport-entry 'EXCEPTS))
-- 
2.43.0


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-5.6-Use-modern-fallback-for-channel-name-detection-i.patch --]
[-- Type: text/x-patch, Size: 5954 bytes --]

From d252960c60af97e669f1c120722f36f5166550c2 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Sun, 11 Feb 2024 20:01:54 -0800
Subject: [PATCH 3/3] [5.6] Use modern fallback for channel name detection in
 ERC

* lisp/erc/erc-backend.el (erc-query-buffer-p): Remove forward declaration.
* lisp/erc/erc.el (erc-query-buffer-p): Defer to `erc-channel-p'.
(erc-channel-p): Refactor and use `erc--fallback-channel-prefixes' for
the default CHANTYPES value.  Honor an empty CHANTYPES set as valid
for dealing with servers that only support direct messages.
(erc--fallback-channel-prefixes): New variable to hold fallback
CHANTYPES value recommended by authorities on the matter.
* test/lisp/erc/erc-tests.el (erc-channel-p): Revise test.
---
 lisp/erc/erc-backend.el    |  1 -
 lisp/erc/erc.el            | 32 +++++++++++++-------------
 test/lisp/erc/erc-tests.el | 46 ++++++++++++++++++++++++--------------
 3 files changed, 44 insertions(+), 35 deletions(-)

diff --git a/lisp/erc/erc-backend.el b/lisp/erc/erc-backend.el
index 2c6da90890b..fbbda6fdbec 100644
--- a/lisp/erc/erc-backend.el
+++ b/lisp/erc/erc-backend.el
@@ -158,7 +158,6 @@ erc-verbose-server-ping
 (declare-function erc-parse-user "erc" (string))
 (declare-function erc-process-away "erc" (proc away-p))
 (declare-function erc-process-ctcp-query "erc" (proc parsed nick login host))
-(declare-function erc-query-buffer-p "erc" (&optional buffer))
 (declare-function erc-remove-channel-member "erc" (channel nick))
 (declare-function erc-remove-channel-users "erc" nil)
 (declare-function erc-remove-user "erc" (nick))
diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index 45869f43c91..154b55c4b62 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1656,11 +1656,7 @@ erc-open-server-buffer-p
 (defun erc-query-buffer-p (&optional buffer)
   "Return non-nil if BUFFER is an ERC query buffer.
 If BUFFER is nil, the current buffer is used."
-  (with-current-buffer (or buffer (current-buffer))
-    (let ((target (erc-target)))
-      (and (eq major-mode 'erc-mode)
-           target
-           (not (memq (aref target 0) '(?# ?& ?+ ?!)))))))
+  (not (erc-channel-p (or buffer (current-buffer)))))
 
 (defun erc-ison-p (nick)
   "Return non-nil if NICK is online."
@@ -1875,18 +1871,20 @@ erc-reuse-frames
   :group 'erc-buffers
   :type 'boolean)
 
-(defun erc-channel-p (channel)
-  "Return non-nil if CHANNEL seems to be an IRC channel name."
-  (cond ((stringp channel)
-         (memq (aref channel 0)
-               (if-let ((types (erc--get-isupport-entry 'CHANTYPES 'single)))
-                   (append types nil)
-                 '(?# ?& ?+ ?!))))
-        ((and-let* (((bufferp channel))
-                    ((buffer-live-p channel))
-                    (target (buffer-local-value 'erc--target channel)))
-           (erc-channel-p (erc--target-string target))))
-        (t nil)))
+(defvar erc--fallback-channel-prefixes "#&"
+  "Prefix chars for distinguishing channel targets when CHANTYPES is unknown.")
+
+(defun erc-channel-p (target)
+  "Return non-nil if TARGET is a valid channel name or a channel buffer."
+  (cond ((stringp target)
+         (and-let*
+             (((not (string-empty-p target)))
+              (value (let ((entry (erc--get-isupport-entry 'CHANTYPES)))
+                       (if entry (cadr entry) erc--fallback-channel-prefixes)))
+              ((erc--strpos (aref target 0) value)))))
+        ((and-let* (((buffer-live-p target))
+                    (target (buffer-local-value 'erc--target target))
+                    ((erc--target-channel-p target)))))))
 
 ;; For the sake of compatibility, a historical quirk concerning this
 ;; option, when nil, has been preserved: all buffers are suffixed with
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 827bd9435e1..1c16c3633c8 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1167,25 +1167,37 @@ erc-downcase
       (should (equal (erc-downcase "\\O/") "|o/" )))))
 
 (ert-deftest erc-channel-p ()
-  (let ((erc--isupport-params (make-hash-table))
-        erc-server-parameters)
-
-    (should (erc-channel-p "#chan"))
-    (should (erc-channel-p "##chan"))
-    (should (erc-channel-p "&chan"))
-    (should (erc-channel-p "+chan"))
-    (should (erc-channel-p "!chan"))
-    (should-not (erc-channel-p "@chan"))
-
-    (push '("CHANTYPES" . "#&@+!") erc-server-parameters)
+  (erc-tests-common-make-server-buf)
 
-    (should (erc-channel-p "!chan"))
-    (should (erc-channel-p "#chan"))
+  (should (erc-channel-p "#chan"))
+  (should (erc-channel-p "##chan"))
+  (should (erc-channel-p "&chan"))
+  (should-not (erc-channel-p "+chan"))
+  (should-not (erc-channel-p "!chan"))
+  (should-not (erc-channel-p "@chan"))
+
+  ;; Server sends "CHANTYPES=#&+!"
+  (should-not erc-server-parameters)
+  (setq erc-server-parameters '(("CHANTYPES" . "#&+!")))
+  (should (erc-channel-p "#chan"))
+  (should (erc-channel-p "&chan"))
+  (should (erc-channel-p "+chan"))
+  (should (erc-channel-p "!chan"))
+
+  (with-current-buffer (erc--open-target "#chan")
+    (should (erc-channel-p (current-buffer))))
+  (with-current-buffer (erc--open-target "+chan")
+    (should (erc-channel-p (current-buffer))))
+  (should (erc-channel-p (get-buffer "#chan")))
+  (should (erc-channel-p (get-buffer "+chan")))
+
+  ;; Server sends "CHANTYPES=" because it's query only.
+  (puthash 'CHANTYPES '("CHANTYPES") erc--isupport-params)
+  (should-not (erc-channel-p "#spam"))
+  (should-not (erc-channel-p "&spam"))
+  (should-not (erc-channel-p (save-excursion (erc--open-target "#spam"))))
 
-    (with-current-buffer (get-buffer-create "#chan")
-      (setq erc--target (erc--target-from-string "#chan")))
-    (should (erc-channel-p (get-buffer "#chan"))))
-  (kill-buffer "#chan"))
+  (erc-tests-common-kill-buffers))
 
 (ert-deftest erc--valid-local-channel-p ()
   (ert-info ("Local channels not supported")
-- 
2.43.0