Subject: Re: bug#61501: 30.0.50; [PATCH] Support unloading Eshell

From: Jim Porter <[email-masked]>
Date: Tue, 14 Feb 2023 16:56:54 -0800

On 2/14/2023 6:34 AM, Eli Zaretskii wrote:
> Thanks, good to hear.  Please add a NEWS entry to call out this
> change.

Thanks. How does this look?

From 743ea9facc76151a50a928e6265154b112604f27 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Sun, 12 Feb 2023 23:25:59 -0800
Subject: [PATCH 2/2] Allow unloading Eshell

* lisp/eshell/em-extpipe.el (eshell-extpipe):
* lisp/eshell/esh-opt.el (eshell-opt): New groups.  Eshell uses these
to identify modules to unload.

* lisp/eshell/em-hist.el (eshell-hist-unload-hook):
* lisp/eshell/em-ls.el (eshell-ls-unload-hook):
* lisp/eshell/em-smart.el (eshell-smart-unload-hook):
* lisp/eshell/eshell.el (eshell-unload-hook): Make obsolete and move
to...

* lisp/eshell/em-smart.el (em-smart-unload-function):
* lisp/eshell/em-hist.el (em-hist-unload-function):
* lisp/eshell/em-ls.el (em-ls-unload-function):
* lisp/eshell/eshell.el (eshell-unload-function): ... these.

* lisp/eshell/esh-mode.el (eshell-mode-unload-hook):
* lisp/eshell/esh-module.el (eshell-module-unload-hook): Make
obsolete.

* lisp/eshell/em-ls (eshell-ls-enable-in-dired,
eshell-ls-disable-in-dired): New functions...
(eshell-ls-use-in-dired): ... use them.

* lisp/eshell/esh-module.el (eshell-module--feature-name,
eshell-unload-modules): New functions.
(eshell-unload-extension-modules): Use 'eshell-unload-modules'.

* lisp/eshell/eshell.el (eshell-unload-all-modules): Remove.

* test/lisp/eshell/eshell-tests-unload.el: New file.

* doc/misc/eshell.texi (Bugs and ideas): Remove item about unloading
Eshell not working.

* etc/NEWS: Announce the obsoletion of the old hooks.
---
 doc/misc/eshell.texi                    |  2 -
 etc/NEWS                                |  6 ++
 lisp/eshell/em-extpipe.el               | 15 ++++
 lisp/eshell/em-hist.el                  |  4 +
 lisp/eshell/em-ls.el                    | 31 ++++----
 lisp/eshell/em-smart.el                 |  4 +
 lisp/eshell/esh-mode.el                 |  1 +
 lisp/eshell/esh-module.el               | 32 ++++++--
 lisp/eshell/esh-opt.el                  |  5 ++
 lisp/eshell/eshell.el                   | 33 +++------
 test/lisp/eshell/eshell-tests-unload.el | 99 +++++++++++++++++++++++++
 11 files changed, 185 insertions(+), 47 deletions(-)
 create mode 100644 test/lisp/eshell/eshell-tests-unload.el

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index e51e2cf799b..1c33c04f647 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -2189,8 +2189,6 @@ Bugs and ideas
 cause it to track the bottom of the output; but backspace no longer
 scrolls back.
 
-@item It's not possible to fully @code{unload-feature} Eshell
-
 @item Menu support was removed, but never put back
 
 @item If an interactive process is currently running, @kbd{M-!} doesn't work
diff --git a/etc/NEWS b/etc/NEWS
index 2d63593ff17..a51e9d9006c 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -196,6 +196,12 @@ their customization options.
 This user option has been obsoleted in Emacs 27, use
 'remote-file-name-inhibit-cache' instead.
 
+---
+** User options 'eshell-NAME-unload-hook' are now obsolete.
+These hooks were named incorrectly, and so they never actually ran
+when unloading the correspending feature.  Instead, you should use
+hooks named after the feature name, like 'esh-mode-unload-hook'.
+
 
 * Lisp Changes in Emacs 30.1
 
diff --git a/lisp/eshell/em-extpipe.el b/lisp/eshell/em-extpipe.el
index 9078c44ed9f..5c9a0a85934 100644
--- a/lisp/eshell/em-extpipe.el
+++ b/lisp/eshell/em-extpipe.el
@@ -36,6 +36,21 @@
 
 (eval-when-compile (require 'files-x))
 
+;;;###autoload
+(progn
+(defgroup eshell-extpipe nil
+  "Native shell pipelines.
+
+This module lets you construct pipelines that use your operating
+system's shell instead of Eshell's own pipelining support.  This
+is especially relevant when executing commands on a remote
+machine using Eshell's Tramp integration: using the remote
+shell's pipelining avoids copying the data which will flow
+through the pipeline to local Emacs buffers and then right back
+again."
+  :tag "External pipelines"
+  :group 'eshell-module))
+
 ;;; Functions:
 
 (defun eshell-extpipe-initialize () ;Called from `eshell-mode' via intern-soft!
diff --git a/lisp/eshell/em-hist.el b/lisp/eshell/em-hist.el
index 4796df1604e..2c199ec160f 100644
--- a/lisp/eshell/em-hist.el
+++ b/lisp/eshell/em-hist.el
@@ -80,6 +80,7 @@ eshell-hist-unload-hook
      (remove-hook 'kill-emacs-hook 'eshell-save-some-history)))
   "A hook that gets run when `eshell-hist' is unloaded."
   :type 'hook)
+(make-obsolete-variable 'eshell-hist-unload-hook nil "30.1")
 
 (defcustom eshell-history-file-name
   (expand-file-name "history" eshell-directory-name)
@@ -1037,6 +1038,9 @@ eshell-isearch-return
   (isearch-done)
   (eshell-send-input))
 
+(defun em-hist-unload-function ()
+  (remove-hook 'kill-emacs-hook 'eshell-save-some-history))
+
 (provide 'em-hist)
 
 ;; Local Variables:
diff --git a/lisp/eshell/em-ls.el b/lisp/eshell/em-ls.el
index 7e2a7578ef9..56c5f262789 100644
--- a/lisp/eshell/em-ls.el
+++ b/lisp/eshell/em-ls.el
@@ -62,24 +62,27 @@ eshell-ls-dired-initial-args
 This is useful for enabling human-readable format (-h), for example."
   :type '(repeat :tag "Arguments" string))
 
+(defun eshell-ls-enable-in-dired ()
+  "Use `eshell-ls' to read directories in Dired."
+  (require 'dired)
+  (advice-add 'insert-directory :around #'eshell-ls--insert-directory)
+  (advice-add 'dired :around #'eshell-ls--dired))
+
+(defun eshell-ls-disable-in-dired ()
+  "Stop using `eshell-ls' to read directories in Dired."
+  (advice-remove 'insert-directory #'eshell-ls--insert-directory)
+  (advice-remove 'dired #'eshell-ls--dired))
+
 (defcustom eshell-ls-use-in-dired nil
   "If non-nil, use `eshell-ls' to read directories in Dired.
 Changing this without using customize has no effect."
   :set (lambda (symbol value)
-	 (cond (value
-                (require 'dired)
-                (advice-add 'insert-directory :around
-                            #'eshell-ls--insert-directory)
-                (advice-add 'dired :around #'eshell-ls--dired))
-               (t
-                (advice-remove 'insert-directory
-                               #'eshell-ls--insert-directory)
-                (advice-remove 'dired #'eshell-ls--dired)))
+         (if value
+             (eshell-ls-enable-in-dired)
+           (eshell-ls-disable-in-dired))
          (set symbol value))
   :type 'boolean
   :require 'em-ls)
-(add-hook 'eshell-ls-unload-hook #'eshell-ls-unload-function)
-
 
 (defcustom eshell-ls-default-blocksize 1024
   "The default blocksize to use when display file sizes with -s."
@@ -954,10 +957,8 @@ eshell-ls-decorated-name
 				 (car file)))))
   (car file))
 
-(defun eshell-ls-unload-function ()
-  (advice-remove 'insert-directory #'eshell-ls--insert-directory)
-  (advice-remove 'dired #'eshell-ls--dired)
-  nil)
+(defun em-ls-unload-function ()
+  (eshell-ls-disable-in-dired))
 
 (provide 'em-ls)
 
diff --git a/lisp/eshell/em-smart.el b/lisp/eshell/em-smart.el
index 154ff760212..d8b7fadc2c2 100644
--- a/lisp/eshell/em-smart.el
+++ b/lisp/eshell/em-smart.el
@@ -99,6 +99,7 @@ eshell-smart-unload-hook
   "A hook that gets run when `eshell-smart' is unloaded."
   :type 'hook
   :group 'eshell-smart)
+(make-obsolete-variable 'eshell-smart-unload-hook nil "30.1")
 
 (defcustom eshell-review-quick-commands nil
   "If t, always review commands.
@@ -321,6 +322,9 @@ eshell-smart-display-move
     (if clear
 	(remove-hook 'pre-command-hook 'eshell-smart-display-move t))))
 
+(defun em-smart-unload-hook ()
+  (remove-hook 'window-configuration-change-hook #'eshell-refresh-windows))
+
 (provide 'em-smart)
 
 ;; Local Variables:
diff --git a/lisp/eshell/esh-mode.el b/lisp/eshell/esh-mode.el
index e0af3579edf..b3cde472713 100644
--- a/lisp/eshell/esh-mode.el
+++ b/lisp/eshell/esh-mode.el
@@ -79,6 +79,7 @@ eshell-mode
 (defcustom eshell-mode-unload-hook nil
   "A hook that gets run when `eshell-mode' is unloaded."
   :type 'hook)
+(make-obsolete-variable 'eshell-mode-unload-hook nil "30.1")
 
 (defcustom eshell-mode-hook nil
   "A hook that gets run when `eshell-mode' is entered."
diff --git a/lisp/eshell/esh-module.el b/lisp/eshell/esh-module.el
index 651e793ad98..7fc74d38796 100644
--- a/lisp/eshell/esh-module.el
+++ b/lisp/eshell/esh-module.el
@@ -47,6 +47,7 @@ eshell-module-unload-hook
   "A hook run when `eshell-module' is unloaded."
   :type 'hook
   :group 'eshell-module)
+(make-obsolete-variable 'eshell-module-unload-hook nil "30.1")
 
 (defcustom eshell-modules-list
   '(eshell-alias
@@ -85,20 +86,37 @@ eshell-modules-list
 
 ;;; Code:
 
+(defsubst eshell-module--feature-name (module &optional kind)
+  "Get the feature name for the specified Eshell MODULE."
+  (let ((module-name (symbol-name module))
+        (prefix (cond ((eq kind 'core) "esh-")
+                      ((memq kind '(extension nil)) "em-")
+                      (t (error "unknown module kind %s" kind)))))
+    (if (string-match "^eshell-\\(.*\\)" module-name)
+	(concat prefix (match-string 1 module-name))
+      (error "Invalid Eshell module name: %s" module))))
+
 (defsubst eshell-using-module (module)
   "Return non-nil if a certain Eshell MODULE is in use.
 The MODULE should be a symbol corresponding to that module's
 customization group.  Example: `eshell-cmpl' for that module."
   (memq module eshell-modules-list))
 
+(defun eshell-unload-modules (modules &optional kind)
+  "Try to unload the specified Eshell MODULES."
+  (dolist (module modules)
+    (let ((module-feature (intern (eshell-module--feature-name module kind))))
+      (when (featurep module-feature)
+	(message "Unloading %s..." (symbol-name module))
+        (condition-case-unless-debug _
+            (progn
+              (unload-feature module-feature)
+              (message "Unloading %s...done" (symbol-name module)))
+          (error (message "Unloading %s...failed" (symbol-name module))))))))
+
 (defun eshell-unload-extension-modules ()
-  "Unload any memory resident extension modules."
-  (dolist (module (eshell-subgroups 'eshell-module))
-    (if (featurep module)
-	(ignore-errors
-	  (message "Unloading %s..." (symbol-name module))
-	  (unload-feature module)
-	  (message "Unloading %s...done" (symbol-name module))))))
+  "Try to unload all currently-loaded Eshell extension modules."
+  (eshell-unload-modules (eshell-subgroups 'eshell-module)))
 
 (provide 'esh-module)
 ;;; esh-module.el ends here
diff --git a/lisp/eshell/esh-opt.el b/lisp/eshell/esh-opt.el
index 9253f9a4a7d..09c19767a19 100644
--- a/lisp/eshell/esh-opt.el
+++ b/lisp/eshell/esh-opt.el
@@ -29,6 +29,11 @@
 ;; defined in esh-util.
 (require 'esh-util)
 
+(defgroup eshell-opt nil
+  "Functions for argument parsing in Eshell commands."
+  :tag "Option parsing"
+  :group 'eshell)
+
 (defmacro eshell-eval-using-options (name macro-args options &rest body-forms)
   "Process NAME's MACRO-ARGS using a set of command line OPTIONS.
 After doing so, stores settings in local symbols as declared by OPTIONS;
diff --git a/lisp/eshell/eshell.el b/lisp/eshell/eshell.el
index 0bfc0413cbf..7d2c0335db2 100644
--- a/lisp/eshell/eshell.el
+++ b/lisp/eshell/eshell.el
@@ -199,10 +199,11 @@ eshell-load-hook
   :type 'hook
   :group 'eshell)
 
-(defcustom eshell-unload-hook '(eshell-unload-all-modules)
+(defcustom eshell-unload-hook nil
   "A hook run when Eshell is unloaded from memory."
   :type 'hook
   :group 'eshell)
+(make-obsolete-variable 'eshell-unload-hook nil "30.1")
 
 (defcustom eshell-buffer-name "*eshell*"
   "The basename used for Eshell buffers.
@@ -370,28 +371,14 @@ eshell-command-result
 	      (set status-var eshell-last-command-status))
 	  (cadr result))))))
 
-;;; Code:
-
-(defun eshell-unload-all-modules ()
-  "Unload all modules that were loaded by Eshell, if possible.
-If the user has require'd in any of the modules, or customized a
-variable with a :require tag (such as `eshell-prefer-to-shell'), it
-will be impossible to unload Eshell completely without restarting
-Emacs."
-  ;; if the user set `eshell-prefer-to-shell' to t, but never loaded
-  ;; Eshell, then `eshell-subgroups' will be unbound
-  (when (fboundp 'eshell-subgroups)
-    (dolist (module (eshell-subgroups 'eshell))
-      ;; this really only unloads as many modules as possible,
-      ;; since other `require' references (such as by customizing
-      ;; `eshell-prefer-to-shell' to a non-nil value) might make it
-      ;; impossible to unload Eshell completely
-      (if (featurep module)
-	  (ignore-errors
-	    (message "Unloading %s..." (symbol-name module))
-	    (unload-feature module)
-	    (message "Unloading %s...done" (symbol-name module)))))
-    (message "Unloading eshell...done")))
+(defun eshell-unload-function ()
+  (eshell-unload-extension-modules)
+  ;; Wait to unload core modules until after `eshell' has finished
+  ;; unloading.  `eshell' depends on several of them, so they can't be
+  ;; unloaded immediately.
+  (run-at-time 0 nil #'eshell-unload-modules
+               (reverse (eshell-subgroups 'eshell)) 'core)
+  nil)
 
 (run-hooks 'eshell-load-hook)
 
diff --git a/test/lisp/eshell/eshell-tests-unload.el b/test/lisp/eshell/eshell-tests-unload.el
new file mode 100644
index 00000000000..cdd58efef18
--- /dev/null
+++ b/test/lisp/eshell/eshell-tests-unload.el
@@ -0,0 +1,99 @@
+;;; eshell-tests-unload.el --- test unloading Eshell  -*- 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:
+
+;; Tests for unloading Eshell.
+
+;;; Code:
+
+(require 'ert)
+(require 'ert-x)
+
+;; In order to test unloading Eshell, don't require any of its files
+;; at the top level.  This means we need to explicitly declare some of
+;; the variables and functions we'll use.
+(defvar eshell-directory-name)
+(defvar eshell-history-file-name)
+(defvar eshell-last-dir-ring-file-name)
+(defvar eshell-modules-list)
+
+(declare-function eshell-module--feature-name "esh-module"
+                  (module &optional kind))
+(declare-function eshell-subgroups "esh-util" (groupsym))
+
+(defvar max-unload-time 5
+  "The maximum amount of time to wait to unload Eshell modules, in seconds.
+See `unload-eshell'.")
+
+(defun load-eshell ()
+  "Load Eshell by calling the `eshell' function and immediately closing it."
+  (save-current-buffer
+    (ert-with-temp-directory eshell-directory-name
+      (let* (;; We want no history file, so prevent Eshell from falling
+             ;; back on $HISTFILE.
+             (process-environment (cons "HISTFILE" process-environment))
+             (eshell-history-file-name nil)
+             (eshell-last-dir-ring-file-name nil)
+             (eshell-buffer (eshell t)))
+        (let (kill-buffer-query-functions)
+          (kill-buffer eshell-buffer))))))
+
+(defun unload-eshell ()
+  "Unload Eshell, waiting until the core modules are unloaded as well."
+  (let ((debug-on-error t)
+        (inhibit-message t))
+    (unload-feature 'eshell)
+    ;; We unload core modules are unloaded from a timer, since they
+    ;; need to wait until after `eshell' itself is unloaded.  Wait for
+    ;; this to finish.
+    (let ((start (current-time)))
+      (while (featurep 'esh-arg)
+        (when (> (float-time (time-since start))
+                 max-unload-time)
+          (error "timed out waiting to unload Eshell modules"))
+        (sit-for 0.1)))))
+
+;;; Tests:
+
+(ert-deftest eshell-test-unload/default ()
+  "Test unloading Eshell with the default list of extension modules."
+  (load-eshell)
+  (unload-eshell))
+
+(ert-deftest eshell-test-unload/no-modules ()
+  "Test unloading Eshell with no extension modules."
+  (require 'esh-module)
+  (let (eshell-modules-list)
+    (load-eshell))
+  (dolist (module (eshell-subgroups 'eshell-module))
+    (should-not (featurep (intern (eshell-module--feature-name module)))))
+  (unload-eshell))
+
+(ert-deftest eshell-test-unload/all-modules ()
+  "Test unloading Eshell with every extension module."
+  (require 'esh-module)
+  (let ((eshell-modules-list (eshell-subgroups 'eshell-module)))
+    (load-eshell))
+  (dolist (module (eshell-subgroups 'eshell-module))
+    (should (featurep (intern (eshell-module--feature-name module)))))
+  (unload-eshell))
+
+(provide 'eshell-tests-unload)
+;;; eshell-tests-unload.el ends here
-- 
2.25.1