GNU bug report logs - #60954
30.0.50; ERC >5.5: Stop requiring erc-goodies in erc.el

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 60954 in the body.
You can then email your comments to 60954 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: "J.P." <jp <at> neverwas.me>
To: bug-gnu-emacs <at> gnu.org
Subject: 29.0.60; ERC 5.4.1: loading ERC clobbers customizations to
 erc-mode-hook
Date: Thu, 19 Jan 2023 21:34:37 -0800

[Message part 1 (text/plain, inline)]

Tags: patch

To reproduce:

  1. Put the following in /tmp/test/init.el
  
       (custom-set-variables
        '(erc-mode-hook '(erc-imenu-setup visual-line-mode)))

     which is exactly what you'd get by doing

       M-x customize-group RET erc-hooks RET

     and adding a `visual-line-mode' item to `erc-mode-hook'.

  2. Run

       emacs --no-site-file --init-directory=/tmp/test

  3. If you do a

       C-h v erc-mode-hook RET

     which loads `erc' behind the scenes, the hook's value will be

       (erc-imenu-setup)

     Note the missing `visual-line-mode'.


Superficially, this is a regression stemming from 

  bug#56340: 29.0.50; [PATCH] Don't ask people to order requires.

which became

   commit c2d657e7c4fd9685591f2120007eabf78745919d
   Author: Dick R. Chiang <dick.r.chiang <at> gmail.com>
   Date:   Fri Jul 1 11:06:51 2022 -0400

   Move ERC's core dependencies to separate file

But amid all that rearranging, a subtle and pernicious headache long ago
swept under the rug happened to resurface [1].

I see two basic avenues of attack here. The first is a pretty safe
stopgap and the second a slightly riskier comprehensive approach that
should have been on the books the moment that bug was closed:

  hack: partially revert a tiny hunk from the commit above

  fix:  don't require goodies at all and instead update the module
        mapping data and add all necessary autoloads and forward
        declarations

Implementations of both are attached. (I didn't bother updating the
required Compat version in the second patch, but that'd also need to
accompany the change set.) For now, I propose we take the shorter route
but with an eye toward revisiting the issue soon after the next (ERC)
release. If anyone has an opinion here, now would be the time to speak
up.

Thanks.

P.S. This bug owes its existence to Libera user jrm, who kindly brought
it to our attention earlier today.


[1] Some related background. For ages, `erc-goodies' was `require'd by
    erc.el at the very end of that file, likely in order to sidestep the
    manual module-to-feature mapping needed by `erc-update-modules' for
    outliers whose names don't match their features (arguably justified
    for smaller, miscellaneous modules). A secondary benefit and
    possible motivating factor was that various commands from goodies
    could feature in `erc-mode-map' without being autoloaded. However,
    additional liberties were eventually taken until erc-goodies.el
    became a de facto addendum to erc.el.



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-19 built on localhost
Repository revision: 7b7b2b95138e691f1b155060b91a8998e3905651
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 64464 6515)
 (symbols 48 8616 0)
 (strings 32 23657 2042)
 (string-bytes 1 685680)
 (vectors 16 15232)
 (vector-slots 8 209553 7914)
 (floats 8 24 23)
 (intervals 56 228 0)
 (buffers 976 10))

[0001-Don-t-load-erc-goodies-atop-erc.el.patch (text/x-patch, attachment)]

[0002-5.6-Copy-over-upstream-Compat-macros-to-erc-compat.patch (text/x-patch, attachment)]

[0003-5.6-Don-t-require-erc-goodies-in-erc.el.patch (text/x-patch, attachment)]

Message #8 received at 60954 <at> debbugs.gnu.org (full text, mbox):

From: Eli Zaretskii <eliz <at> gnu.org>
To: "J.P." <jp <at> neverwas.me>
Cc: 60954 <at> debbugs.gnu.org, emacs-erc <at> gnu.org
Subject: Re: bug#60954: 29.0.60;
 ERC 5.4.1: loading ERC clobbers customizations to erc-mode-hook
Date: Fri, 20 Jan 2023 09:16:48 +0200

> Cc: emacs-erc <at> gnu.org
> From: "J.P." <jp <at> neverwas.me>
> Date: Thu, 19 Jan 2023 21:34:37 -0800
> 
> I see two basic avenues of attack here. The first is a pretty safe
> stopgap and the second a slightly riskier comprehensive approach that
> should have been on the books the moment that bug was closed:
> 
>   hack: partially revert a tiny hunk from the commit above
> 
>   fix:  don't require goodies at all and instead update the module
>         mapping data and add all necessary autoloads and forward
>         declarations
> 
> Implementations of both are attached. (I didn't bother updating the
> required Compat version in the second patch, but that'd also need to
> accompany the change set.) For now, I propose we take the shorter route
> but with an eye toward revisiting the issue soon after the next (ERC)
> release. If anyone has an opinion here, now would be the time to speak
> up.

Your proposal is fine with me, thanks.

Message #11 received at 60954 <at> debbugs.gnu.org (full text, mbox):

From: "J.P." <jp <at> neverwas.me>
To: Eli Zaretskii <eliz <at> gnu.org>
Cc: 60954 <at> debbugs.gnu.org, emacs-erc <at> gnu.org
Subject: Re: bug#60954: 29.0.60; ERC 5.4.1: loading ERC clobbers
 customizations to erc-mode-hook
Date: Fri, 20 Jan 2023 06:14:17 -0800

Eli Zaretskii <eliz <at> gnu.org> writes:

>> I see two basic avenues of attack here. The first is a pretty safe
>> stopgap and the second a slightly riskier comprehensive approach that
>> should have been on the books the moment that bug was closed:
>> 
>>   hack: partially revert a tiny hunk from the commit above
>> 
>>   fix:  don't require goodies at all and instead update the module
>>         mapping data and add all necessary autoloads and forward
>>         declarations
>> 
>> Implementations of both are attached. (I didn't bother updating the
>> required Compat version in the second patch, but that'd also need to
>> accompany the change set.) For now, I propose we take the shorter route
>> but with an eye toward revisiting the issue soon after the next (ERC)
>> release. If anyone has an opinion here, now would be the time to speak
>> up.
>
> Your proposal is fine with me, thanks.

Great, thanks. I went ahead and added the lighter change to the release
branch.

Leaving this open until the rest is settled (whenever that may be).

Message #14 received at 60954 <at> debbugs.gnu.org (full text, mbox):

From: "J.P." <jp <at> neverwas.me>
To: 60954 <at> debbugs.gnu.org
Cc: emacs-erc <at> gnu.org
Subject: Re: bug#60954: 29.0.60; ERC 5.4.1: loading ERC clobbers
 customizations to erc-mode-hook
Date: Fri, 20 Jan 2023 06:15:29 -0800

[Message part 1 (text/plain, inline)]

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

> I see two basic avenues of attack here. The first is a pretty safe
> stopgap and the second a slightly riskier comprehensive approach that
> should have been on the books the moment that bug was closed:
>
>   hack: partially revert a tiny hunk from the commit above
>
>   fix:  don't require goodies at all and instead update the module
>         mapping data and add all necessary autoloads and forward
>         declarations

Under "fix," I forgot to mention also couching any top-level `add-hook'
calls in module-setup and teardown functions. Of particular concern are
those that mutate user options. Attached is an addition to the (5.6
portion of the) patch set that does this for the Imenu integration.

[0004-5.6-Convert-ERC-s-Imenu-integration-into-proper-modu.patch (text/x-patch, attachment)]

Message #19 received at 60954 <at> debbugs.gnu.org (full text, mbox):

From: "J.P." <jp <at> neverwas.me>
To: 60954 <at> debbugs.gnu.org
Cc: emacs-erc <at> gnu.org
Subject: Re: bug#60954: 30.0.50; ERC >5.5: Stop requiring erc-goodies in erc.el
Date: Sat, 21 Jan 2023 07:03:11 -0800

[Message part 1 (text/plain, inline)]

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

> diff --git a/lisp/erc/erc-goodies.el b/lisp/erc/erc-goodies.el
> index 05a21019042..0bf4bb9537c 100644
> --- a/lisp/erc/erc-goodies.el
> +++ b/lisp/erc/erc-goodies.el
> @@ -38,9 +38,10 @@ erc-controls-highlight-regexp
> [...]
>  (declare-function erc-buffer-list "erc" (&optional predicate proc))
>  (declare-function erc-error "erc" (&rest args))
> @@ -384,9 +385,11 @@ erc-get-fg-color-face
>  (define-erc-module irccontrols nil
>    "This mode enables the interpretation of IRC control chars."
>    ((add-hook 'erc-insert-modify-hook #'erc-controls-highlight)
> -   (add-hook 'erc-send-modify-hook #'erc-controls-highlight))
> +   (add-hook 'erc-send-modify-hook #'erc-controls-highlight)
> +   (define-key erc-mode-map "\C-c\C-c" #'erc-toggle-interpret-controls))
>    ((remove-hook 'erc-insert-modify-hook #'erc-controls-highlight)
> -   (remove-hook 'erc-send-modify-hook #'erc-controls-highlight)))
> +   (remove-hook 'erc-send-modify-hook #'erc-controls-highlight)
> +   (erc-compat-call define-key erc-mode-map "\C-c\C-c" nil t)))

Actually, messing with `erc-mode-map' like this is sure to break
configs. Snippets like

  (with-eval-after-load 'erc
    (define-key erc-mode-map (kbd "RET") nil)
    (define-key erc-mode-map (kbd "C-c C-c") #'erc-send-current-line))

are fairly common [1]. Of course, mutating `erc-mode-map' is prevalent
throughout ERC [2], but we probably shouldn't perpetuate the practice.
If this were a brand new module, we could just give it its own
minor-mode map. But doing so now might just serve to confuse.

Here are two possibilities that seem relatively benign:

  1. Leave the original binding in `erc-mode-map' but autoload its
     command, `erc-toggle-interpret-controls', and make it smart enough
     to enable `erc-irccontrols-mode' when necessary and maybe print a
     message explaining why.

  2. Mutate the local map when the module's loaded, but only do so
     conditionally, by first checking to see if the key is taken
     (locally or in `erc-mode-map'). Likewise, only remove the binding
     if it's set to `erc-toggle-interpret-controls' locally.

Door #1 is more conservative because it introduces zero churn, AFAICT.
But without any plan for deprecation, it's tantamount to kicking the can
down the road. Door #2 keeps things more compartmentalized and walled
off (ERC does tout itself as being modular, after all), but there's a
nonzero chance of churn in some cases, for example, when

  - someone sets a binding for `erc-mode-map' after the module is loaded
  - some map depends on `erc-mode-map' as a parent

Otherwise, conditionally updating the current local map seems to have
the same functional effect here as using a minor-mode map and
conditionally overriding that in all ERC buffers (since this is a global
module).

The main downside I see for #2 is that it adds yet another facet of
complexity to an already cluttered landscape, seeing as minor-mode maps
are likely to become more common with the introduction of local modules.
Perhaps of less concern is that there's no precedent for it (in ERC).
That said, if we were to go bold and convert all `erc-mode-map'
mutations across all ERC libraries, it would at least offer some sense
of consistency and predictability going forward. (I suspect there may be
other downsides I've not yet considered, though, so if you're privy to
any, please do share.)

Because #1 is pretty easy to visualize, I've gone ahead and provided a
POC of #2 (for the `irccontrols' module only) in the third patch below.
Anyone interested, please have a look.

Thanks.


[1] See (info "(erc) Sample Configuration").

[2] See `erc-button-setup' and the modules toggles for `erc-sound' and
    `erc-ring' and top-level forms in erc-match.el and erc-log.el.

[0000-v1-v2.diff (text/x-patch, attachment)]

[0001-Don-t-load-erc-goodies-atop-erc.el.patch (text/x-patch, attachment)]

[0002-5.6-Copy-over-upstream-Compat-macros-to-erc-compat.patch (text/x-patch, attachment)]

[0003-5.6-Don-t-require-erc-goodies-in-erc.el.patch (text/x-patch, attachment)]

[0004-5.6-Convert-ERC-s-Imenu-integration-into-proper-modu.patch (text/x-patch, attachment)]

Message #22 received at 60954 <at> debbugs.gnu.org (full text, mbox):

From: "J.P." <jp <at> neverwas.me>
To: 60954 <at> debbugs.gnu.org
Cc: emacs-erc <at> gnu.org
Subject: Re: bug#60954: 30.0.50; ERC >5.5: Stop requiring erc-goodies in erc.el
Date: Tue, 31 Jan 2023 07:27:54 -0800

[Message part 1 (text/plain, inline)]

v3. Require erc-goodies in erc-ibuffer. Fix bug in
`erc-update-mode-line-buffer'.

The issue of modules changing the major-mode map remains unaddressed.

[0000-v2-v3.diff (text/x-patch, attachment)]

[0001-5.6-Copy-over-upstream-Compat-macros-to-erc-compat.patch (text/x-patch, attachment)]

[0002-5.6-Don-t-require-erc-goodies-in-erc.el.patch (text/x-patch, attachment)]

[0003-5.6-Convert-ERC-s-Imenu-integration-into-proper-modu.patch (text/x-patch, attachment)]

Message #25 received at 60954 <at> debbugs.gnu.org (full text, mbox):

From: "J.P." <jp <at> neverwas.me>
To: 60954 <at> debbugs.gnu.org
Cc: emacs-erc <at> gnu.org
Subject: Re: bug#60954: 30.0.50; ERC >5.5: Stop requiring erc-goodies in erc.el
Date: Tue, 07 Feb 2023 07:22:42 -0800

[Message part 1 (text/plain, inline)]

v4. Switch to autoloads-based module migration scheme. Add old patch for
extending control-color range. Move remaining top-level `define-key'
occurrences to module definitions.

[0000-v3-v4.diff (text/x-patch, attachment)]

[0001-5.6-Copy-over-upstream-Compat-macros-to-erc-compat.patch (text/x-patch, attachment)]

[0002-5.6-Leverage-loaddefs-for-migrating-ERC-modules.patch (text/x-patch, attachment)]

[0003-5.6-Don-t-require-erc-goodies-in-erc.el.patch (text/x-patch, attachment)]

[0004-5.6-Modify-erc-mode-map-in-module-definitions.patch (text/x-patch, attachment)]

[0005-5.6-Add-missing-colors-to-erc-irccontrols-mode.patch (text/x-patch, attachment)]

[0006-5.6-Convert-ERC-s-Imenu-integration-into-proper-modu.patch (text/x-patch, attachment)]

Message #28 received at 60954 <at> debbugs.gnu.org (full text, mbox):

From: "J.P." <jp <at> neverwas.me>
To: 60954 <at> debbugs.gnu.org
Cc: emacs-erc <at> gnu.org
Subject: Re: bug#60954: 30.0.50; ERC >5.5: Stop requiring erc-goodies in erc.el
Date: Sun, 19 Feb 2023 07:07:19 -0800

[Message part 1 (text/plain, inline)]

v5. Add (tangentially related) patch to allow arbitrary CHANTYPES. Fix
Customize widget bug by maintaining sorted `erc-modules' value. Tag
built-in modules in `erc-modules' :initialize form. Actually respect
autoloads when looking for module mode vars. Fix old bug in
`erc-unfill-notice'.

[0000-v4-v5.diff (text/x-patch, attachment)]

[0001-5.6-Honor-arbitrary-CHANTYPES-in-ERC.patch (text/x-patch, attachment)]

[0002-5.6-Copy-over-upstream-Compat-macros-to-erc-compat.patch (text/x-patch, attachment)]

[0003-5.6-Leverage-loaddefs-for-migrating-ERC-modules.patch (text/x-patch, attachment)]

[0004-5.6-Don-t-require-erc-goodies-in-erc.el.patch (text/x-patch, attachment)]

[0005-5.6-Modify-erc-mode-map-in-module-definitions.patch (text/x-patch, attachment)]

[0006-5.6-Add-missing-colors-to-erc-irccontrols-mode.patch (text/x-patch, attachment)]

[0007-5.6-Convert-ERC-s-Imenu-integration-into-proper-modu.patch (text/x-patch, attachment)]

Message #31 received at 60954 <at> debbugs.gnu.org (full text, mbox):

From: "J.P." <jp <at> neverwas.me>
To: 60954 <at> debbugs.gnu.org
Cc: emacs-erc <at> gnu.org
Subject: Re: bug#60954: 30.0.50; ERC >5.5: Stop requiring erc-goodies in erc.el
Date: Thu, 09 Mar 2023 06:43:26 -0800

[Message part 1 (text/plain, inline)]

v6. Autoload `erc-controls-strip' in erc-loaddefs.el and also `require'
the latter in erc.el when compiling. Remove unneeded forward
declarations. Remove barely used `require's atop file (possibly
controversial). Use dedicated "spoilers" face to avoid appearance of
gaps in text.

[0000-v5-v6.diff (text/x-patch, attachment)]

[0001-5.6-Honor-arbitrary-CHANTYPES-in-ERC.patch (text/x-patch, attachment)]

[0002-5.6-Copy-over-upstream-Compat-macros-to-erc-compat.patch (text/x-patch, attachment)]

[0003-5.6-Leverage-loaddefs-for-migrating-ERC-modules.patch (text/x-patch, attachment)]

[0004-5.6-Don-t-require-erc-goodies-in-erc.el.patch (text/x-patch, attachment)]

[0005-5.6-Modify-erc-mode-map-in-module-definitions.patch (text/x-patch, attachment)]

[0006-5.6-Add-missing-colors-to-erc-irccontrols-mode.patch (text/x-patch, attachment)]

[0007-5.6-Convert-ERC-s-Imenu-integration-into-proper-modu.patch (text/x-patch, attachment)]

Message #34 received at 60954 <at> debbugs.gnu.org (full text, mbox):

From: "J.P." <jp <at> neverwas.me>
To: 60954 <at> debbugs.gnu.org
Cc: emacs-erc <at> gnu.org
Subject: Re: bug#60954: 30.0.50; ERC >5.5: Stop requiring erc-goodies in erc.el
Date: Tue, 14 Mar 2023 06:32:15 -0700

[Message part 1 (text/plain, inline)]

v7. Prevent erroneous Custom "edit" state when initializing
`erc-modules' on account of `standard-value' ordering mismatch.

[0000-v6-v7.diff (text/x-patch, attachment)]

[0001-5.6-Honor-arbitrary-CHANTYPES-in-ERC.patch (text/x-patch, attachment)]

[0002-5.6-Copy-over-upstream-Compat-macros-to-erc-compat.patch (text/x-patch, attachment)]

[0003-5.6-Leverage-loaddefs-for-migrating-ERC-modules.patch (text/x-patch, attachment)]

[0004-5.6-Don-t-require-erc-goodies-in-erc.el.patch (text/x-patch, attachment)]

[0005-5.6-Modify-erc-mode-map-in-module-definitions.patch (text/x-patch, attachment)]

[0006-5.6-Add-missing-colors-to-erc-irccontrols-mode.patch (text/x-patch, attachment)]

[0007-5.6-Convert-ERC-s-Imenu-integration-into-proper-modu.patch (text/x-patch, attachment)]

Message #37 received at 60954 <at> debbugs.gnu.org (full text, mbox):

From: "J.P." <jp <at> neverwas.me>
To: 60954 <at> debbugs.gnu.org
Cc: emacs-erc <at> gnu.org
Subject: Re: bug#60954: 30.0.50; ERC >5.5: Stop requiring erc-goodies in erc.el
Date: Wed, 15 Mar 2023 07:04:09 -0700

[Message part 1 (text/plain, inline)]

v8. Maintain sorted default value for `erc-modules' (instead of mutating
`standard-value'). Add test to verify.

[0000-v7-v8.diff (text/x-patch, attachment)]

[0001-5.6-Honor-arbitrary-CHANTYPES-in-ERC.patch (text/x-patch, attachment)]

[0002-5.6-Copy-over-upstream-Compat-macros-to-erc-compat.patch (text/x-patch, attachment)]

[0003-5.6-Leverage-loaddefs-for-migrating-ERC-modules.patch (text/x-patch, attachment)]

[0004-5.6-Don-t-require-erc-goodies-in-erc.el.patch (text/x-patch, attachment)]

[0005-5.6-Modify-erc-mode-map-in-module-definitions.patch (text/x-patch, attachment)]

[0006-5.6-Add-missing-colors-to-erc-irccontrols-mode.patch (text/x-patch, attachment)]

[0007-5.6-Convert-ERC-s-Imenu-integration-into-proper-modu.patch (text/x-patch, attachment)]

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.