Multiple definitions of explicit-shell-file-name

Multiple definitions of explicit-shell-file-name

[Basil L. Contovounesios]

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

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?


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-Define-explicit-shell-file-name-in-simple.el.patch --]
[-- Type: text/x-diff, Size: 9781 bytes --]

From 3cc187a1bbe0058b5bd535d693371194c156d7d1 Mon Sep 17 00:00:00 2001
From: "Basil L. Contovounesios" <contovob@tcd.ie>
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))
 \f
+(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


[-- Attachment #3: Type: text/plain, Size: 437 bytes --]


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

Re: Multiple definitions of explicit-shell-file-name

[Basil L. Contovounesios]

"Basil L. Contovounesios" <contovob@tcd.ie> writes:

> 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?

3. Should this go to master or emacs-27?  The changes are simple but not
   pressing (we've had the current situation for decades), so my guess
   is master.

-- 
Basil

Re: Multiple definitions of explicit-shell-file-name

[Eli Zaretskii]

> From: "Basil L. Contovounesios" <contovob@tcd.ie>
> Date: Thu, 16 Apr 2020 01:06:05 +0100
> 
> 3. Should this go to master or emacs-27?  The changes are simple but not
>    pressing (we've had the current situation for decades), so my guess
>    is master.

Not to emacs-27 in any case.  But let's first see if there are any
comments on your patch.  (And please in the future post patches to the
issue tracker, via report-emacs-bug, TIA.)

Thanks.

Re: Multiple definitions of explicit-shell-file-name

[Stefan Monnier]

> 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?

I don't think it belongs in simple.el.  It should be in shell.el.


        Stefan

Re: Multiple definitions of explicit-shell-file-name

[Basil L. Contovounesios]

Stefan Monnier <monnier@iro.umontreal.ca> writes:

>> 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?
>
> I don't think it belongs in simple.el.  It should be in shell.el.

I agree, but explicit-shell-file-name is used in contexts unrelated to
shell.el, such as ansi-term and dired-insert-directory.

Rather than define explicit-shell-file-name in a central place like
simple.el, should these external users all check that the variable is
bound first?

If so, I'll update the patch and send it to the issue tracker for
further feedback.

Thanks,

-- 
Basil

Re: Multiple definitions of explicit-shell-file-name

[Stefan Monnier]

> I agree, but explicit-shell-file-name is used in contexts unrelated to
> shell.el, such as ansi-term and dired-insert-directory.

I don't consider ansi-term to be "unrelated" to shell.el, so I think
it's perfectly OK for it to (require 'shell).  It might even encourage
more sharing of code between the two.

> Rather than define explicit-shell-file-name in a central place like
> simple.el, should these external users all check that the variable is
> bound first?

For `dired-insert-directory`, I'm not sure whether it's better to use
bound-and-true-p like we do now or to use something else entirely.
IOW, I'm not completely sure why it uses `explicit-shell-file-name`
instead of `shell-file-name`.

But, it's clearly OK to leave the current dired code as is.


        Stefan