bug#60784: 29.0.60; ERC 5.5: Don't interfere with non-module minor modes in erc-open

bug#60784: 29.0.60; ERC 5.5: Don't interfere with non-module minor modes in erc-open

[J.P.]

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

Tags: patch

ERC currently takes ownership of any minor mode it encounters whose name
happens to be prefixed with "erc-" as long as its mode variable is set
locally in an ERC buffer. That is, it propagates the enabled/disabled
state to new `erc-mode' buffers, possibly by calling the namesake
minor-mode function. This makes sense for modes defined by "local"
modules, whether built-in or third-party. But other, non-module minor
modes that just happen to use the same top-level library namespace
should be left alone.


In GNU Emacs 29.0.60 (build 1, x86_64-pc-linux-gnu, GTK+ Version
 3.24.35, cairo version 1.17.6) of 2023-01-13 built on localhost
Repository revision: c6bbf9cc270dedb8adcafdd0c7ff902611176993
Repository branch: emacs-29
Windowing system distributor 'The X.Org Foundation', version 11.0.12014000
System Description: Fedora Linux 36 (Workstation Edition)

Configured using:
 'configure --enable-check-lisp-object-type --enable-checking=yes,glyphs
 'CFLAGS=-O0 -g3'
 PKG_CONFIG_PATH=:/usr/lib64/pkgconfig:/usr/share/pkgconfig'

Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GPM GSETTINGS HARFBUZZ JPEG
JSON LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 M17N_FLT MODULES NOTIFY
INOTIFY PDUMPER PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS WEBP X11 XDBE XIM XINPUT2 XPM GTK3 ZLIB

Important settings:
  value of $LANG: en_US.UTF-8
  value of $XMODIFIERS: @im=ibus
  locale-coding-system: utf-8-unix

Major mode: Lisp Interaction

Minor modes in effect:
  tooltip-mode: t
  global-eldoc-mode: t
  eldoc-mode: t
  show-paren-mode: t
  electric-indent-mode: t
  mouse-wheel-mode: t
  tool-bar-mode: t
  menu-bar-mode: t
  file-name-shadow-mode: t
  global-font-lock-mode: t
  font-lock-mode: t
  blink-cursor-mode: t
  line-number-mode: t
  indent-tabs-mode: t
  transient-mark-mode: t
  auto-composition-mode: t
  auto-encryption-mode: t
  auto-compression-mode: t

Load-path shadows:
None found.

Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec epa derived epg rfc6068 epg-config
gnus-util text-property-search mm-decode mm-bodies mm-encode mail-parse
rfc2231 mailabbrev gmm-utils mailheader sendmail rfc2047 rfc2045
ietf-drums mm-util mail-prsvr mail-utils erc iso8601 time-date
auth-source cl-seq eieio eieio-core cl-macs password-cache json subr-x
map thingatpt pp format-spec cl-loaddefs cl-lib erc-backend erc-goodies
erc-networks byte-opt gv bytecomp byte-compile erc-common erc-compat
erc-loaddefs rmc iso-transl tooltip cconv eldoc paren electric uniquify
ediff-hook vc-hooks lisp-float-type elisp-mode mwheel term/x-win x-win
term/common-win x-dnd tool-bar dnd fontset image regexp-opt fringe
tabulated-list replace newcomment text-mode lisp-mode prog-mode register
page tab-bar menu-bar rfn-eshadow isearch easymenu timer select
scroll-bar mouse jit-lock font-lock syntax font-core term/tty-colors
frame minibuffer nadvice seq simple cl-generic indonesian philippine
cham georgian utf-8-lang misc-lang vietnamese tibetan thai tai-viet lao
korean japanese eucjp-ms cp51932 hebrew greek romanian slovak czech
european ethiopic indian cyrillic chinese composite emoji-zwj charscript
charprop case-table epa-hook jka-cmpr-hook help abbrev obarray oclosure
cl-preloaded button loaddefs theme-loaddefs faces cus-face macroexp
files window text-properties overlay sha1 md5 base64 format env
code-pages mule custom widget keymap hashtable-print-readable backquote
threads dbusbind inotify lcms2 dynamic-setting system-font-setting
font-render-setting cairo move-toolbar gtk x-toolkit xinput2 x multi-tty
make-network-process emacs)

Memory information:
((conses 16 64566 5968)
 (symbols 48 8614 0)
 (strings 32 23664 2035)
 (string-bytes 1 685847)
 (vectors 16 15236)
 (vector-slots 8 209710 8268)
 (floats 8 24 33)
 (intervals 56 236 0)
 (buffers 976 11))


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Avoid-plist-get-as-generalized-var-in-erc-compat.patch --]
[-- Type: text/x-patch, Size: 1166 bytes --]

From 0ed497e97bef0cd16c6b6cc9f79c605db6dae31c Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Tue, 10 Jan 2023 11:59:57 -0800
Subject: [PATCH 1/2] ; Avoid plist-get as generalized var in erc-compat

* lisp/erc/erc-compat.el (erc-compat--29-auth-source-pass-search): The
gv expander for `plist-get' was added in Emacs 28.  But ERC still
supports 27, as of this function's introduction, in Emacs 29.
---
 lisp/erc/erc-compat.el | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lisp/erc/erc-compat.el b/lisp/erc/erc-compat.el
index 73ce612a33..5601ede27a 100644
--- a/lisp/erc/erc-compat.el
+++ b/lisp/erc/erc-compat.el
@@ -260,8 +260,8 @@ erc-compat--29-auth-source-pass-search
           (dolist (e rv out)
             (when-let* ((s (plist-get e :secret))
                         (v (auth-source--obfuscate s)))
-              (setf (plist-get e :secret)
-                    (apply-partially #'auth-source--deobfuscate v)))
+              (setq e (plist-put e :secret (apply-partially
+                                            #'auth-source--deobfuscate v))))
             (push e out)))
       rv)))
 
-- 
2.38.1


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Don-t-preserve-non-module-minor-modes-in-erc-open.patch --]
[-- Type: text/x-patch, Size: 3491 bytes --]

From 2a712dc6d22b81ac778063a6c2f6b217f13a5169 Mon Sep 17 00:00:00 2001
From: "F. Jason Park" <jp@neverwas.me>
Date: Fri, 13 Jan 2023 06:03:15 -0800
Subject: [PATCH 2/2] Don't preserve non-module minor modes in erc-open

* lisp/erc/erc.el (erc--merge-local-modes): Be more conservative when
persisting local minor-mode state across ERC sessions.  User and
third-party modes that were not defined via `define-erc-modules'
should be left alone.
* test/lisp/erc/erc-tests.el (erc--merge-local-modes): Add mocks for
"-enable" toggles and a test case covering some foreign ERC mode.
---
 lisp/erc/erc.el            |  5 ++++-
 test/lisp/erc/erc-tests.el | 34 ++++++++++++++++++++++------------
 2 files changed, 26 insertions(+), 13 deletions(-)

diff --git a/lisp/erc/erc.el b/lisp/erc/erc.el
index ba7db15cf8..ab84849e20 100644
--- a/lisp/erc/erc.el
+++ b/lisp/erc/erc.el
@@ -1950,7 +1950,10 @@ erc--merge-local-modes
       (let ((out (list (reverse new-modes))))
         (pcase-dolist (`(,k . ,v) old-vars)
           (when (and (string-prefix-p "erc-" (symbol-name k))
-                     (string-suffix-p "-mode" (symbol-name k)))
+                     (string-suffix-p "-mode" (symbol-name k))
+                     (fboundp (intern-soft
+                               (concat (substring (symbol-name k) 0 -5)
+                                       "-enable"))))
             (if v
                 (cl-pushnew k (car out))
               (setf (car out) (delq k (car out)))
diff --git a/test/lisp/erc/erc-tests.el b/test/lisp/erc/erc-tests.el
index 85506c3d27..4501db6102 100644
--- a/test/lisp/erc/erc-tests.el
+++ b/test/lisp/erc/erc-tests.el
@@ -1251,18 +1251,28 @@ erc--update-modules
         (setq calls nil)))))
 
 (ert-deftest erc--merge-local-modes ()
-
-  (ert-info ("No existing modes")
-    (let ((old '((a) (b . t)))
-          (new '(erc-c-mode erc-d-mode)))
-      (should (equal (erc--merge-local-modes new old)
-                     '((erc-c-mode erc-d-mode))))))
-
-  (ert-info ("Active existing added, inactive existing removed, deduped")
-    (let ((old '((a) (erc-b-mode) (c . t) (erc-d-mode . t) (erc-e-mode . t)))
-          (new '(erc-b-mode erc-d-mode)))
-      (should (equal (erc--merge-local-modes new old)
-                     '((erc-d-mode erc-e-mode) . (erc-b-mode)))))))
+  (cl-letf (((symbol-function 'erc-b-enable) #'ignore)
+            ((symbol-function 'erc-c-enable) #'ignore)
+            ((symbol-function 'erc-d-enable) #'ignore)
+            ((symbol-function 'erc-e-enable) #'ignore))
+
+    (ert-info ("No existing modes")
+      (let ((old '((a) (b . t)))
+            (new '(erc-c-mode erc-d-mode)))
+        (should (equal (erc--merge-local-modes new old)
+                       '((erc-c-mode erc-d-mode))))))
+
+    (ert-info ("Active existing added, inactive existing removed, deduped")
+      (let ((old '((a) (erc-b-mode) (c . t) (erc-d-mode . t) (erc-e-mode . t)))
+            (new '(erc-b-mode erc-d-mode)))
+        (should (equal (erc--merge-local-modes new old)
+                       '((erc-d-mode erc-e-mode) . (erc-b-mode))))))
+
+    (ert-info ("Non-module erc-prefixed mode ignored")
+      (let ((old '((erc-b-mode) (erc-f-mode . t) (erc-d-mode . t)))
+            (new '(erc-b-mode)))
+        (should (equal (erc--merge-local-modes new old)
+                       '((erc-d-mode) . (erc-b-mode))))))))
 
 (ert-deftest define-erc-module--global ()
   (let ((global-module '(define-erc-module mname malias
-- 
2.38.1

bug#60784: 29.0.60; ERC 5.5: Don't interfere with non-module minor modes in erc-open

[J.P.]

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

> ERC currently takes ownership of any minor mode it encounters whose name
> happens to be prefixed with "erc-" as long as its mode variable is set
> locally in an ERC buffer. That is, it propagates the enabled/disabled
> state to new `erc-mode' buffers, possibly by calling the namesake
> minor-mode function. This makes sense for modes defined by "local"
> modules, whether built-in or third-party. But other, non-module minor
> modes that just happen to use the same top-level library namespace
> should be left alone.

I've addressed this in a slightly more reliable way that doesn't depend
on heuristics. Instead of banking on the unlikelihood of a non-module
minor mode also defining a function suffixed with "-enable," I've opted
for tagging a module's minor-mode symbol with the property `erc-module',
to be paired with its canonical symbol as a value.

It happens that while addressing this, I stumbled upon another, closely
related (and arguably more important) bug stemming from the "local
modules" change set (bug#57955), which was installed last November to
support the SASL module. Basically, a local module's activation state
(i.e., that of its minor-mode variable) wouldn't survive reconnections,
as vociferously claimed. My oversight here owes its inception, in part,
to the SASL module's lack of interest in target buffers, which its test
cases came to reflect. And while the (vital) matter of reinstating a
local module's associated data falls beyond the scope of this change, it
ranks highly among the improvements I'd like to see in ERC's next major
release.

Thanks (and closing).