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

From: Jim Porter <[email-masked]>
Date: Wed, 15 Feb 2023 09:31:57 -0800

On 2/15/2023 4:59 AM, Eli Zaretskii wrote:
>> Date: Tue, 14 Feb 2023 16:56:54 -0800
>> Cc: [email-masked]
>> From: Jim Porter <[email-masked]>
>>
>> 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?
> 
> It's okay, but I actually thought about telling that unloading Eshell
> now works...

Oh, I see. How about this?

I could also try to merge the two news entries. However, since the 
previous one that I added about hook names describes an incompatible 
change, my gut feeling is to keep them separate, or just remove the 
previous entry (I'm not sure anyone uses the old names since they didn't 
work in the first place).

From 1456b83734e956bd3fb0415715e148e3dd3f8497 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 this change.
---
 doc/misc/eshell.texi                    |  2 -
 etc/NEWS                                | 11 +++
 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, 190 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..8eff74e00d3 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -131,6 +131,11 @@ this to your configuration:
 
     (keymap-set eshell-mode-map "<home>" #'eshell-bol-ignoring-prompt)
 
+---
+*** You can now properly unload Eshell.
+Calling "(unload-feature 'eshell)" no longer signals an error, and now
+correctly unloads Eshell and all of its modules.
+
 +++
 *** 'eshell-read-aliases-list' is now an interactive command.
 After manually editing 'eshell-aliases-file', you can use this command
@@ -196,6 +201,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