Multiple definitions of explicit-shell-file-name

[Basil L. Contovounesios]

I noticed that explicit-shell-file-name has been defined as a user
option in three different places since at least as far back as 1997:

- lisp/shell.el
- lisp/term.el
- lisp/obsolete/terminal.el

Each file also assigns it a different custom :group.  Can we consolidate
these duplicates in a single place such as lisp/simple.el, as follows?

>From 3cc187a1bbe0058b5bd535d693371194c156d7d1 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <address@hidden>
Date: Wed, 15 Apr 2020 18:26:38 +0100
Subject: [PATCH] Define explicit-shell-file-name in simple.el

* doc/emacs/cmdargs.texi (General Variables): Mention
shell-file-name in relation to SHELL.
* doc/emacs/misc.texi (Interactive Shell): Move index entry for
SHELL environment variable from here, where it is not mentioned...
(Single Shell): ...to here, where it is discussed along with
shell-file-name.

* lisp/shell.el (shell): Simplify.
(explicit-shell-file-name):
* lisp/term.el (explicit-shell-file-name):
* lisp/obsolete/terminal.el (explicit-shell-file-name): Consolidate
duplicate defcustoms from here...
* lisp/simple.el (explicit-shell-file-name): ...to here.

* lisp/dired.el (dired-insert-directory):
* lisp/emulation/viper-ex.el (ex-show-vars):
* lisp/progmodes/sh-script.el:
* lisp/w32-fns.el (explicit-shell-file-name):
(w32-check-shell-configuration): Remove defvar declarations and
boundp guards now that explicit-shell-file-name is bound by default.
---
 doc/emacs/cmdargs.texi      |  3 ++-
 doc/emacs/misc.texi         |  2 +-
 lisp/dired.el               |  7 +++----
 lisp/emulation/viper-ex.el  |  6 +-----
 lisp/obsolete/terminal.el   |  6 ------
 lisp/progmodes/sh-script.el |  2 --
 lisp/shell.el               | 22 +++++++---------------
 lisp/simple.el              |  8 ++++++++
 lisp/term.el                |  5 -----
 lisp/w32-fns.el             |  5 +----
 10 files changed, 23 insertions(+), 43 deletions(-)

diff --git a/doc/emacs/cmdargs.texi b/doc/emacs/cmdargs.texi
index 9303b0b8dd..936facbf28 100644
--- a/doc/emacs/cmdargs.texi
+++ b/doc/emacs/cmdargs.texi
@@ -647,7 +647,8 @@ General Variables
 @item SHELL
 @vindex SHELL@r{, environment variable}
 The name of an interpreter used to parse and execute programs run from
-inside Emacs.
+inside Emacs.  This is used to initialize the variable
+@code{shell-file-name} (@pxref{Single Shell}).
 @item SMTPSERVER
 @vindex SMTPSERVER@r{, environment variable}
 The name of the outgoing mail server.  This is used to initialize the
diff --git a/doc/emacs/misc.texi b/doc/emacs/misc.texi
index d097f4ee7d..8e1b61815d 100644
--- a/doc/emacs/misc.texi
+++ b/doc/emacs/misc.texi
@@ -806,6 +806,7 @@ Single Shell
 @file{*Shell Command Output*} buffer.
 
 @vindex shell-file-name
+@cindex @env{SHELL} environment variable
   The above commands use the shell specified by the variable
 @code{shell-file-name}.  Its default value is determined by the
 @env{SHELL} environment variable when Emacs is started.  If the file
@@ -875,7 +876,6 @@ Interactive Shell
 @vindex explicit-shell-file-name
 @cindex environment variables for subshells
 @cindex @env{ESHELL} environment variable
-@cindex @env{SHELL} environment variable
   To specify the shell file name used by @kbd{M-x shell}, customize
 the variable @code{explicit-shell-file-name}.  If this is @code{nil}
 (the default), Emacs uses the environment variable @env{ESHELL} if it
diff --git a/lisp/dired.el b/lisp/dired.el
index 9583d5d809..ac43814679 100644
--- a/lisp/dired.el
+++ b/lisp/dired.el
@@ -1447,10 +1447,9 @@ dired-insert-directory
              (let* ((default-directory (car dir-wildcard))
                     (script (format "ls %s %s" switches (cdr dir-wildcard)))
                     (remotep (file-remote-p dir))
-                    (sh (or (and remotep "/bin/sh")
-                            (and (bound-and-true-p explicit-shell-file-name)
-                                 (executable-find explicit-shell-file-name))
-                            (executable-find "sh")))
+                    (sh (if remotep
+                            "/bin/sh"
+                          (executable-find (or explicit-shell-file-name 
"sh"))))
                     (switch (if remotep "-c" shell-command-switch)))
                (unless
                    (zerop
diff --git a/lisp/emulation/viper-ex.el b/lisp/emulation/viper-ex.el
index 929b7a0bed..b2eb5a94e0 100644
--- a/lisp/emulation/viper-ex.el
+++ b/lisp/emulation/viper-ex.el
@@ -37,7 +37,6 @@ viper-local-search-start-marker
 (defvar viper-expert-level)
 (defvar viper-custom-file-name)
 (defvar viper-case-fold-search)
-(defvar explicit-shell-file-name)
 (defvar compile-command)
 (require 'viper-keym)
 ;; end pacifier
@@ -2281,10 +2280,7 @@ ex-show-vars
                   (- (window-width) fill-column)))
     (princ (format "wrapmargin (global) \t= %S\n"
                   (- (window-width) (default-value 'fill-column))))
-    (princ (format "shell \t\t\t= %S\n" (if (boundp 'explicit-shell-file-name)
-                                           explicit-shell-file-name
-                                         'none)))
-    ))
+    (princ (format "shell \t\t\t= %S\n" explicit-shell-file-name))))
 
 (defun ex-print ()
   (viper-default-ex-addresses)
diff --git a/lisp/obsolete/terminal.el b/lisp/obsolete/terminal.el
index 6ee53af648..4dba7a5e89 100644
--- a/lisp/obsolete/terminal.el
+++ b/lisp/obsolete/terminal.el
@@ -1056,12 +1056,6 @@ te-stty-string
 ;; This used to have `new' in it, but that loses outside BSD
 ;; and it's apparently not needed in BSD.
 
-(defcustom explicit-shell-file-name nil
-  "If non-nil, is file name to use for explicitly requested inferior shell."
-  :type '(choice (const :tag "None" nil)
-                file)
-  :group 'terminal)
-
 ;;;###autoload
 (defun terminal-emulator (buffer program args &optional width height)
   "Under a display-terminal emulator in BUFFER, run PROGRAM on arguments ARGS.
diff --git a/lisp/progmodes/sh-script.el b/lisp/progmodes/sh-script.el
index 044d7820ee..e13687bb5c 100644
--- a/lisp/progmodes/sh-script.el
+++ b/lisp/progmodes/sh-script.el
@@ -1395,8 +1395,6 @@ sh-make-vars-local
 (defvar-local sh-shell-process nil
   "The inferior shell process for interaction.")
 
-(defvar explicit-shell-file-name)
-
 (defun sh-shell-process (force)
   "Get a shell process for interaction.
 If FORCE is non-nil and no process found, create one."
diff --git a/lisp/shell.el b/lisp/shell.el
index 1e2679f723..6344935db6 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -267,13 +267,6 @@ shell-dirtrack-verbose
   :type 'boolean
   :group 'shell-directories)
 
-(defcustom explicit-shell-file-name nil
-  "If non-nil, is file name to use for explicitly requested inferior shell.
-When nil, such interactive shell sessions fallback to using either
-the shell specified in $ESHELL or in `shell-file-name'."
-  :type '(choice (const :tag "None" nil) file)
-  :group 'shell)
-
 ;; Note: There are no explicit references to the variable `explicit-csh-args'.
 ;; It is used implicitly by M-x shell when the shell is `csh'.
 (defcustom explicit-csh-args
@@ -742,16 +735,15 @@ shell
 
   (with-connection-local-variables
    ;; On remote hosts, the local `shell-file-name' might be useless.
-   (when (file-remote-p default-directory)
-     (if (and (called-interactively-p 'any)
+   (when (and (file-remote-p default-directory)
+              (called-interactively-p 'any)
               (null explicit-shell-file-name)
               (null (getenv "ESHELL")))
-         (set (make-local-variable 'explicit-shell-file-name)
-              (file-local-name
-              (expand-file-name
-                (read-file-name
-                 "Remote shell path: " default-directory shell-file-name
-                 t shell-file-name))))))
+     (setq-local explicit-shell-file-name
+                 (file-local-name
+                  (expand-file-name
+                   (read-file-name "Remote shell path: " default-directory
+                                   shell-file-name t shell-file-name)))))
 
    ;; Rain or shine, BUFFER must be current by now.
    (unless (comint-check-proc buffer)
diff --git a/lisp/simple.el b/lisp/simple.el
index abf5ee2625..efea5bfaea 100644
--- a/lisp/simple.el
+++ b/lisp/simple.el
@@ -3357,6 +3357,14 @@ undo-outer-limit-truncate
     (setq buffer-undo-list nil)
     t))
 
+(defcustom explicit-shell-file-name nil
+  "If non-nil, the file name to use for explicitly requested inferior shells.
+When nil, such interactive shell sessions fall back to using
+either the environment variable \"ESHELL\" if it exists, or the
+value of the user option `shell-file-name'."
+  :type '(choice (const :tag "Default" nil) file)
+  :group 'shell)
+
 (defvar shell-command-history nil
   "History list for some commands that read shell commands.
 
diff --git a/lisp/term.el b/lisp/term.el
index b990c83cfc..148f83cec3 100644
--- a/lisp/term.el
+++ b/lisp/term.el
@@ -394,11 +394,6 @@ term-pager-old-filter
 (defvar-local term-line-mode-buffer-read-only nil
   "The `buffer-read-only' state to set in `term-line-mode'.")
 
-(defcustom explicit-shell-file-name nil
-  "If non-nil, is file name to use for explicitly requested inferior shell."
-  :type '(choice (const nil) file)
-  :group 'term)
-
 (defvar term-prompt-regexp "^"
   "Regexp to recognize prompts in the inferior process.
 Defaults to \"^\", the null string at BOL.
diff --git a/lisp/w32-fns.el b/lisp/w32-fns.el
index c252c0b18f..7cca7e8476 100644
--- a/lisp/w32-fns.el
+++ b/lisp/w32-fns.el
@@ -27,8 +27,6 @@
 ;;; Code:
 (require 'w32-vars)
 
-(defvar explicit-shell-file-name)
-
 ;;;; Function keys
 
 (declare-function w32-get-locale-info "w32proc.c" (lcid &optional longform))
@@ -86,8 +84,7 @@ w32-check-shell-configuration
        (insert (format "Warning! shell-file-name uses %s.
 You probably want to change it so that it uses cmdproxy.exe instead.\n\n"
                        shell-file-name)))
-    (if (and (boundp 'explicit-shell-file-name)
-            (w32-system-shell-p explicit-shell-file-name))
+    (if (w32-system-shell-p explicit-shell-file-name)
        (insert (format "Warning! explicit-shell-file-name uses %s.
 You probably want to change it so that it uses cmdproxy.exe instead.\n\n"
                        explicit-shell-file-name)))
-- 
2.25.1

Some followup questions in case this is a welcome change:

0. Does the user option's :version need to be bumped?

1. Does this change need to be called out in etc/NEWS?

2. I stuck with the 'shell' custom group as it's in the name of the user
   option, is used by all other shell-related user options in simple.el,
   and is I think where users were most likely to find the user option
   until now.  Is that okay?

Thanks,

-- 
Basil