master cee1cbfd54 6/7: Improve handling of $PATH in Eshell for remote directories

[Jim Porter]

branch: master
commit cee1cbfd54375cdece23d4741ced6b0c7091f6d9
Author: Jim Porter <jporterbugs@gmail.com>
Commit: Jim Porter <jporterbugs@gmail.com>

    Improve handling of $PATH in Eshell for remote directories
    
    * lisp/eshell/esh-util.el (eshell-path-env, eshell-parse-colon-path):
    Make obsolete.
    (eshell-path-env-list): New variable.
    (eshell-connection-default-profile): New connection-local profile.
    (eshell-get-path): Reimplement using 'eshell-path-env-list'; add
    LITERAL-P argument.
    (eshell-set-path): New function.
    
    * lisp/eshell/esh-var.el (eshell-variable-aliases-list): Add entry for
    $PATH.
    (eshell-var-initialize): Add 'eshell-path-env-list' to
    'eshell-subcommand-bindings'.
    
    * lisp/eshell/esh-ext.el (eshell-search-path): Use 'file-name-concat'
    instead of 'concat'.
    (eshell/addpath): Use 'eshell-get-path' and 'eshell-set-path'.
    
    * lisp/net/tramp-integration.el: Only apply Eshell hooks when
    'eshell-path-env-list' is unbound.
    
    * test/lisp/eshell/esh-var-tests.el
    (esh-var-test/path-var/local-directory)
    (esh-var-test/path-var/remote-directory, esh-var-test/path-var/set)
    (esh-var-test/path-var/set-locally)
    (esh-var-test/path-var-preserve-across-hosts): New tests.
    
    * test/lisp/eshell/esh-ext-tests.el: New file.
    
    * test/lisp/eshell/eshell-tests-helpers.el
    (with-temp-eshell): Set 'eshell-last-dir-ring-file-name' to nil.
    (eshell-tests-remote-accessible-p, eshell-last-input)
    (eshell-last-output): New functions.
    (eshell-match-output, eshell-match-output--explainer): Use
    'eshell-last-input' and 'eshell-last-output'.
    
    * doc/misc/eshell.texi (Variables): Document $PATH.
    
    * etc/NEWS: Announce this change (bug#57556).
---
 doc/misc/eshell.texi                     | 10 +++++
 etc/NEWS                                 |  5 +++
 lisp/eshell/esh-ext.el                   | 23 +++++-----
 lisp/eshell/esh-util.el                  | 55 ++++++++++++++++++++---
 lisp/eshell/esh-var.el                   | 12 ++++-
 lisp/net/tramp-integration.el            | 21 ++++-----
 test/lisp/eshell/esh-ext-tests.el        | 76 ++++++++++++++++++++++++++++++++
 test/lisp/eshell/esh-var-tests.el        | 60 +++++++++++++++++++++++++
 test/lisp/eshell/eshell-tests-helpers.el | 32 +++++++++++---
 9 files changed, 257 insertions(+), 37 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index 21c1671a21..d518eafd72 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -942,6 +942,16 @@ When using @code{$-}, you can also access older 
directories in the
 directory ring via subscripting, e.g.@: @samp{$-[1]} refers to the
 working directory @emph{before} the previous one.
 
+@vindex $PATH
+@item $PATH
+This specifies the directories to search for executable programs.  Its
+value is a string, separated by @code{":"} for Unix and GNU systems,
+and @code{";"} for MS systems.  This variable is connection-aware, so
+whenever you change the current directory to a different host
+(@pxref{Remote Files, , , emacs, The GNU Emacs Manual}),
+the value will automatically update to reflect the search path on that
+host.
+
 @vindex $_
 @item $_
 This refers to the last argument of the last command.  With a
diff --git a/etc/NEWS b/etc/NEWS
index d64614783b..e63c7742bc 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -356,6 +356,11 @@ previous 'C-x ='.
 
 ** Eshell
 
+*** Eshell's PATH is now derived from 'exec-path'.
+For consistency with remote connections, Eshell now uses 'exec-path'
+to determine the execution path on the local system, instead of using
+the PATH environment variable directly.
+
 ---
 *** 'source' and '.' no longer accept the '--help' option.
 This is for compatibility with the shell versions of these commands,
diff --git a/lisp/eshell/esh-ext.el b/lisp/eshell/esh-ext.el
index 98902fc6f2..d513d750d9 100644
--- a/lisp/eshell/esh-ext.el
+++ b/lisp/eshell/esh-ext.el
@@ -77,7 +77,7 @@ but Eshell will be able to understand
     (let ((list (eshell-get-path))
          suffixes n1 n2 file)
       (while list
-       (setq n1 (concat (car list) name))
+       (setq n1 (file-name-concat (car list) name))
        (setq suffixes eshell-binary-suffixes)
        (while suffixes
          (setq n2 (concat n1 (car suffixes)))
@@ -239,17 +239,16 @@ causing the user to wonder if anything's really going 
on..."
      (?h "help" nil nil  "display this usage message")
      :usage "[-b] PATH
 Adds the given PATH to $PATH.")
-   (if args
-       (progn
-        (setq eshell-path-env (getenv "PATH")
-              args (mapconcat #'identity args path-separator)
-              eshell-path-env
-              (if prepend
-                  (concat args path-separator eshell-path-env)
-                (concat eshell-path-env path-separator args)))
-        (setenv "PATH" eshell-path-env))
-     (dolist (dir (parse-colon-path (getenv "PATH")))
-       (eshell-printn dir)))))
+   (let ((path (eshell-get-path t)))
+     (if args
+         (progn
+           (setq path (if prepend
+                          (append args path)
+                        (append path args)))
+           (eshell-set-path path)
+           (string-join path (path-separator)))
+       (dolist (dir path)
+         (eshell-printn dir))))))
 
 (put 'eshell/addpath 'eshell-no-numeric-conversions t)
 (put 'eshell/addpath 'eshell-filename-arguments t)
diff --git a/lisp/eshell/esh-util.el b/lisp/eshell/esh-util.el
index 9258ca5e40..9b464a0a13 100644
--- a/lisp/eshell/esh-util.el
+++ b/lisp/eshell/esh-util.el
@@ -249,17 +249,60 @@ trailing newlines removed.  Otherwise, this behaves as 
follows:
 It might be different from \(getenv \"PATH\"), when
 `default-directory' points to a remote host.")
 
-(defun eshell-get-path ()
+(make-obsolete-variable 'eshell-path-env 'eshell-get-path "29.1")
+
+(defvar-local eshell-path-env-list nil)
+
+(connection-local-set-profile-variables
+ 'eshell-connection-default-profile
+ '((eshell-path-env-list . nil)))
+
+(connection-local-set-profiles
+ '(:application eshell)
+ 'eshell-connection-default-profile)
+
+(defun eshell-get-path (&optional literal-p)
   "Return $PATH as a list.
-Add the current directory on MS-Windows."
-  (eshell-parse-colon-path
-   (if (eshell-under-windows-p)
-       (concat "." path-separator eshell-path-env)
-     eshell-path-env)))
+If LITERAL-P is nil, return each directory of the path as a full,
+possibly-remote file name; on MS-Windows, add the current
+directory as the first directory in the path as well.
+
+If LITERAL-P is non-nil, return the local part of each directory,
+as the $PATH was actually specified."
+  (with-connection-local-application-variables 'eshell
+    (let ((remote (file-remote-p default-directory))
+          (path
+           (or eshell-path-env-list
+               ;; If not already cached, get the path from
+               ;; `exec-path', removing the last element, which is
+               ;; `exec-directory'.
+               (setq-connection-local eshell-path-env-list
+                                      (butlast (exec-path))))))
+      (when (and (not literal-p)
+                 (not remote)
+                 (eshell-under-windows-p))
+        (push "." path))
+      (if (and remote (not literal-p))
+          (mapcar (lambda (x) (file-name-concat remote x)) path)
+        path))))
+
+(defun eshell-set-path (path)
+  "Set the Eshell $PATH to PATH.
+PATH can be either a list of directories or a string of
+directories separated by `path-separator'."
+  (with-connection-local-application-variables 'eshell
+    (setq-connection-local
+     eshell-path-env-list
+     (if (listp path)
+        path
+       ;; Don't use `parse-colon-path' here, since we don't want
+       ;; the additonal translations it does on each element.
+       (split-string path (path-separator))))))
 
 (defun eshell-parse-colon-path (path-env)
   "Split string with `parse-colon-path'.
 Prepend remote identification of `default-directory', if any."
+  (declare (obsolete nil "29.1"))
   (let ((remote (file-remote-p default-directory)))
     (if remote
        (mapcar
diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index caf143e1a1..57ea42f493 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -156,7 +156,14 @@ if they are quoted with a backslash."
     ("LINES" ,(lambda () (window-body-height nil 'remap)) t t)
     ("INSIDE_EMACS" eshell-inside-emacs t)
 
-    ;; for eshell-cmd.el
+    ;; for esh-ext.el
+    ("PATH" (,(lambda () (string-join (eshell-get-path t) (path-separator)))
+             . ,(lambda (_ value)
+                  (eshell-set-path value)
+                  value))
+     t t)
+
+    ;; for esh-cmd.el
     ("_" ,(lambda (indices quoted)
            (if (not indices)
                (car (last eshell-last-arguments))
@@ -249,7 +256,8 @@ copied (a.k.a. \"exported\") to the environment of created 
subprocesses."
   (setq-local eshell-subcommand-bindings
               (append
                '((process-environment (eshell-copy-environment))
-                 (eshell-variable-aliases-list eshell-variable-aliases-list))
+                 (eshell-variable-aliases-list eshell-variable-aliases-list)
+                 (eshell-path-env-list eshell-path-env-list))
                eshell-subcommand-bindings))
 
   (setq-local eshell-special-chars-inside-quoting
diff --git a/lisp/net/tramp-integration.el b/lisp/net/tramp-integration.el
index 35c0636b1c..4be019edd9 100644
--- a/lisp/net/tramp-integration.el
+++ b/lisp/net/tramp-integration.el
@@ -136,16 +136,17 @@ been set up by `rfn-eshadow-setup-minibuffer'."
           (getenv "PATH"))))
 
 (with-eval-after-load 'esh-util
-  (add-hook 'eshell-mode-hook
-           #'tramp-eshell-directory-change)
-  (add-hook 'eshell-directory-change-hook
-           #'tramp-eshell-directory-change)
-  (add-hook 'tramp-integration-unload-hook
-           (lambda ()
-             (remove-hook 'eshell-mode-hook
-                          #'tramp-eshell-directory-change)
-             (remove-hook 'eshell-directory-change-hook
-                          #'tramp-eshell-directory-change))))
+  (unless (boundp 'eshell-path-env-list)
+    (add-hook 'eshell-mode-hook
+             #'tramp-eshell-directory-change)
+    (add-hook 'eshell-directory-change-hook
+             #'tramp-eshell-directory-change)
+    (add-hook 'tramp-integration-unload-hook
+             (lambda ()
+               (remove-hook 'eshell-mode-hook
+                            #'tramp-eshell-directory-change)
+               (remove-hook 'eshell-directory-change-hook
+                            #'tramp-eshell-directory-change)))))
 
 ;;; Integration of recentf.el:
 
diff --git a/test/lisp/eshell/esh-ext-tests.el 
b/test/lisp/eshell/esh-ext-tests.el
new file mode 100644
index 0000000000..54191e9409
--- /dev/null
+++ b/test/lisp/eshell/esh-ext-tests.el
@@ -0,0 +1,76 @@
+;;; esh-ext-tests.el --- esh-ext test suite  -*- lexical-binding:t -*-
+
+;; Copyright (C) 2022 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 Eshell's external command handling.
+
+;;; Code:
+
+(require 'ert)
+(require 'esh-mode)
+(require 'esh-ext)
+(require 'eshell)
+
+(require 'eshell-tests-helpers
+         (expand-file-name "eshell-tests-helpers"
+                           (file-name-directory (or load-file-name
+                                                    default-directory))))
+
+;;; Tests:
+
+(ert-deftest esh-ext-test/addpath/end ()
+  "Test that \"addpath\" adds paths to the end of $PATH."
+  (with-temp-eshell
+   (let ((eshell-path-env-list '("/some/path" "/other/path"))
+         (expected-path (string-join '("/some/path" "/other/path" "/new/path"
+                                       "/new/path2")
+                                     (path-separator))))
+     (eshell-match-command-output "addpath /new/path /new/path2"
+                                  (concat expected-path "\n"))
+     (eshell-match-command-output "echo $PATH"
+                                  (concat expected-path "\n")))))
+
+(ert-deftest esh-ext-test/addpath/begin ()
+  "Test that \"addpath -b\" adds paths to the beginning of $PATH."
+  (with-temp-eshell
+   (let ((eshell-path-env-list '("/some/path" "/other/path"))
+         (expected-path (string-join '("/new/path" "/new/path2" "/some/path"
+                                       "/other/path")
+                                     (path-separator))))
+     (eshell-match-command-output "addpath -b /new/path /new/path2"
+                                  (concat expected-path "\n"))
+     (eshell-match-command-output "echo $PATH"
+                                  (concat expected-path "\n")))))
+
+(ert-deftest esh-ext-test/addpath/set-locally ()
+  "Test adding to the path temporarily in a subcommand."
+  (let* ((eshell-path-env-list '("/some/path" "/other/path"))
+         (original-path (string-join eshell-path-env-list (path-separator)))
+         (local-path (string-join (append eshell-path-env-list '("/new/path"))
+                                  (path-separator))))
+    (with-temp-eshell
+     (eshell-match-command-output
+      "{ addpath /new/path; env }"
+      (format "PATH=%s\n" (regexp-quote local-path)))
+     ;; After the last command, the previous $PATH value should be restored.
+     (eshell-match-command-output "echo $PATH"
+                                  (concat original-path "\n")))))
+
+;; esh-ext-tests.el ends here
diff --git a/test/lisp/eshell/esh-var-tests.el 
b/test/lisp/eshell/esh-var-tests.el
index a7ac52ed24..d9b2585a32 100644
--- a/test/lisp/eshell/esh-var-tests.el
+++ b/test/lisp/eshell/esh-var-tests.el
@@ -23,6 +23,7 @@
 
 ;;; Code:
 
+(require 'tramp)
 (require 'ert)
 (require 'esh-mode)
 (require 'esh-var)
@@ -610,6 +611,65 @@ it, since the setter is nil."
    (eshell-match-command-output "echo $INSIDE_EMACS[, 1]"
                                 "eshell")))
 
+(ert-deftest esh-var-test/path-var/local-directory ()
+  "Test using $PATH in a local directory."
+  (let ((expected-path (string-join (eshell-get-path t) (path-separator))))
+    (with-temp-eshell
+     (eshell-match-command-output "echo $PATH" (regexp-quote expected-path)))))
+
+(ert-deftest esh-var-test/path-var/remote-directory ()
+  "Test using $PATH in a remote directory."
+  (skip-unless (eshell-tests-remote-accessible-p))
+  (let* ((default-directory ert-remote-temporary-file-directory)
+         (expected-path (string-join (eshell-get-path t) (path-separator))))
+    (with-temp-eshell
+     (eshell-match-command-output "echo $PATH" (regexp-quote expected-path)))))
+
+(ert-deftest esh-var-test/path-var/set ()
+  "Test setting $PATH."
+  (let* ((path-to-set-list '("/some/path" "/other/path"))
+         (path-to-set (string-join path-to-set-list (path-separator))))
+    (with-temp-eshell
+     (eshell-match-command-output (concat "set PATH " path-to-set)
+                                  (concat path-to-set "\n"))
+     (eshell-match-command-output "echo $PATH" (concat path-to-set "\n"))
+     (should (equal (eshell-get-path t) path-to-set-list)))))
+
+(ert-deftest esh-var-test/path-var/set-locally ()
+  "Test setting $PATH temporarily for a single command."
+  (let* ((path-to-set-list '("/some/path" "/other/path"))
+         (path-to-set (string-join path-to-set-list (path-separator))))
+    (with-temp-eshell
+     (eshell-match-command-output (concat "set PATH " path-to-set)
+                                  (concat path-to-set "\n"))
+     (eshell-match-command-output "PATH=/local/path env"
+                                  "PATH=/local/path\n")
+     ;; After the last command, the previous $PATH value should be restored.
+     (eshell-match-command-output "echo $PATH" (concat path-to-set "\n"))
+     (should (equal (eshell-get-path t) path-to-set-list)))))
+
+(ert-deftest esh-var-test/path-var/preserve-across-hosts ()
+  "Test that $PATH can be set independently on multiple hosts."
+  (let ((local-directory default-directory)
+        local-path remote-path)
+    (with-temp-eshell
+     ;; Set the $PATH on localhost.
+     (eshell-insert-command "set PATH /local/path")
+     (setq local-path (eshell-last-output))
+     ;; `cd' to a remote host and set the $PATH there too.
+     (eshell-insert-command
+      (format "cd %s" ert-remote-temporary-file-directory))
+     (eshell-insert-command "set PATH /remote/path")
+     (setq remote-path (eshell-last-output))
+     ;; Return to localhost and check that $PATH is the value we set
+     ;; originally.
+     (eshell-insert-command (format "cd %s" local-directory))
+     (eshell-match-command-output "echo $PATH" (regexp-quote local-path))
+     ;; ... and do the same for the remote host.
+     (eshell-insert-command
+      (format "cd %s" ert-remote-temporary-file-directory))
+     (eshell-match-command-output "echo $PATH" (regexp-quote remote-path)))))
+
 (ert-deftest esh-var-test/last-status-var-lisp-command ()
   "Test using the \"last exit status\" ($?) variable with a Lisp command"
   (with-temp-eshell
diff --git a/test/lisp/eshell/eshell-tests-helpers.el 
b/test/lisp/eshell/eshell-tests-helpers.el
index e713e162ad..1d9674070c 100644
--- a/test/lisp/eshell/eshell-tests-helpers.el
+++ b/test/lisp/eshell/eshell-tests-helpers.el
@@ -31,11 +31,22 @@
 (require 'eshell)
 
 (defvar eshell-history-file-name nil)
+(defvar eshell-last-dir-ring-file-name nil)
 
 (defvar eshell-test--max-subprocess-time 5
   "The maximum amount of time to wait for a subprocess to finish, in seconds.
 See `eshell-wait-for-subprocess'.")
 
+(defun eshell-tests-remote-accessible-p ()
+  "Return if a test involving remote files can proceed.
+If using this function, be sure to load `tramp' near the
+beginning of the test file."
+  (ignore-errors
+    (and
+     (file-remote-p ert-remote-temporary-file-directory)
+     (file-directory-p ert-remote-temporary-file-directory)
+     (file-writable-p ert-remote-temporary-file-directory))))
+
 (defmacro with-temp-eshell (&rest body)
   "Evaluate BODY in a temporary Eshell buffer."
   `(save-current-buffer
@@ -44,6 +55,7 @@ See `eshell-wait-for-subprocess'.")
               ;; 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)))
          (unwind-protect
              (with-current-buffer eshell-buffer
@@ -83,19 +95,25 @@ After inserting, call FUNC.  If FUNC is nil, instead call
   (insert-and-inherit command)
   (funcall (or func 'eshell-send-input)))
 
+(defun eshell-last-input ()
+  "Return the input of the last Eshell command."
+  (buffer-substring-no-properties
+   eshell-last-input-start eshell-last-input-end))
+
+(defun eshell-last-output ()
+  "Return the output of the last Eshell command."
+  (buffer-substring-no-properties
+   (eshell-beginning-of-output) (eshell-end-of-output)))
+
 (defun eshell-match-output (regexp)
   "Test whether the output of the last command matches REGEXP."
-  (string-match-p
-    regexp (buffer-substring-no-properties
-            (eshell-beginning-of-output) (eshell-end-of-output))))
+  (string-match-p regexp (eshell-last-output)))
 
 (defun eshell-match-output--explainer (regexp)
   "Explain the result of `eshell-match-output'."
   `(mismatched-output
-    (command ,(buffer-substring-no-properties
-               eshell-last-input-start eshell-last-input-end))
-    (output ,(buffer-substring-no-properties
-              (eshell-beginning-of-output) (eshell-end-of-output)))
+    (command ,(eshell-last-input))
+    (output ,(eshell-last-output))
     (regexp ,regexp)))
 
 (put 'eshell-match-output 'ert-explainer #'eshell-match-output--explainer)