Don't manipulate args in-place for 'eshell-eval-using-options'

This is necessary for preserve the original arguments to forward on to
:external commands.  Previously, when :preserve-args was also set, the
original argument list could be altered, changing the meaning of the
command.

* lisp/eshell/esh-opt.el (eshell-eval-using-options): Copy MACRO-ARGS
when :preserve-args is set, and pass the original value to
'eshell--do-opts'.
(eshell--do-opts): Use the original arguments when calling an external
command.

* lisp/eshell/em-tramp.el (eshell/su, eshell/sudo): Don't copy the
original arguments, since 'eshell-eval-using-options' does this for
us.

* test/lisp/eshell/esh-opt-tests.el (esh-opt-process-args-test):
Split this test into...
(esh-opt-test/process-args)
(esh-opt-test/process-args-parse-leading-options-only)
(esh-opt-test/process-args-external): ... these.
(test-eshell-eval-using-options): Split this test into...
(esh-opt-test/eval-using-options-short)
(esh-opt-test/eval-using-options-long)
(esh-opt-test/eval-using-options-constant)
(esh-opt-test/eval-using-options-user-specified)
(esh-opt-test/eval-using-options-short-single-token)
(esh-opt-test/eval-using-options-terminate-options)
(esh-opt-test/eval-using-options-parse-leading-options-only)
(esh-opt-test/eval-using-options-unrecognized): ... these.
(esh-opt-test/eval-using-options-external): New test.

* test/lisp/eshell/em-tramp-tests.el: New tests.

4 files changed, 209 insertions, 100 deletions

diff --git a/lisp/eshell/em-tramp.el b/lisp/eshell/em-tramp.el
index e9018bdb934..791458822da 100644
--- a/lisp/eshell/em-tramp.el
+++ b/lisp/eshell/em-tramp.el
@@ -57,41 +57,42 @@
 
 (autoload 'eshell-parse-command "esh-cmd")
 
-(defun eshell/su (&rest args)
+(defun eshell/su (&rest arguments)
   "Alias \"su\" to call TRAMP.
 
 Uses the system su through TRAMP's su method."
-  (setq args (eshell-stringify-list (flatten-tree args)))
-  (let ((orig-args (copy-tree args)))
-    (eshell-eval-using-options
-     "su" args
-     '((?h "help" nil nil "show this usage screen")
-       (?l "login" nil login "provide a login environment")
-       (?  nil nil login "provide a login environment")
-       :usage "[- | -l | --login] [USER]
+  (setq arguments (eshell-stringify-list (flatten-tree arguments)))
+  (eshell-eval-using-options
+   "su" arguments
+   '((?h "help" nil nil "show this usage screen")
+     (?l "login" nil login "provide a login environment")
+     (?  nil nil login "provide a login environment")
+     :usage "[- | -l | --login] [USER]
 Become another USER during a login session.")
-     (throw 'eshell-replace-command
-	    (let ((user "root")
-		  (host (or (file-remote-p default-directory 'host)
-			    "localhost"))
-		  (dir (file-local-name (expand-file-name default-directory)))
-		  (prefix (file-remote-p default-directory)))
-	      (dolist (arg args)
-		(if (string-equal arg "-") (setq login t) (setq user arg)))
-	      ;; `eshell-eval-using-options' does not handle "-".
-	      (if (member "-" orig-args) (setq login t))
-	      (if login (setq dir "~/"))
-	      (if (and prefix
-		       (or
-			(not (string-equal
-			      "su" (file-remote-p default-directory 'method)))
-			(not (string-equal
-			      user (file-remote-p default-directory 'user)))))
-		  (eshell-parse-command
-		   "cd" (list (format "%s|su:%s@%s:%s"
-				      (substring prefix 0 -1) user host dir)))
-		(eshell-parse-command
-		 "cd" (list (format "/su:%s@%s:%s" user host dir)))))))))
+   (throw 'eshell-replace-command
+          (let ((user "root")
+                (host (or (file-remote-p default-directory 'host)
+                          "localhost"))
+                (dir (file-local-name (expand-file-name default-directory)))
+                (prefix (file-remote-p default-directory)))
+            (dolist (arg args)
+              (if (string-equal arg "-") (setq login t) (setq user arg)))
+            ;; `eshell-eval-using-options' tries to handle "-" as a
+            ;; short option; double-check whether the original
+            ;; arguments include it.
+            (when (member "-" arguments) (setq login t))
+            (when login (setq dir "~/"))
+            (if (and prefix
+                     (or
+                      (not (string-equal
+                            "su" (file-remote-p default-directory 'method)))
+                      (not (string-equal
+                            user (file-remote-p default-directory 'user)))))
+                (eshell-parse-command
+                 "cd" (list (format "%s|su:%s@%s:%s"
+                                    (substring prefix 0 -1) user host dir)))
+              (eshell-parse-command
+               "cd" (list (format "/su:%s@%s:%s" user host dir))))))))
 
 (put 'eshell/su 'eshell-no-numeric-conversions t)
 
@@ -99,41 +100,35 @@ Become another USER during a login session.")
   "Alias \"sudo\" to call Tramp.
 
 Uses the system sudo through TRAMP's sudo method."
-  (setq args (eshell-stringify-list (flatten-tree args)))
-  (let ((orig-args (copy-tree args)))
-    (eshell-eval-using-options
-     "sudo" args
-     '((?h "help" nil nil "show this usage screen")
-       (?u "user" t user "execute a command as another USER")
-       :show-usage
-       :parse-leading-options-only
-       :usage "[(-u | --user) USER] COMMAND
+  (eshell-eval-using-options
+   "sudo" args
+   '((?h "help" nil nil "show this usage screen")
+     (?u "user" t user "execute a command as another USER")
+     :show-usage
+     :parse-leading-options-only
+     :usage "[(-u | --user) USER] COMMAND
 Execute a COMMAND as the superuser or another USER.")
-     (throw 'eshell-external
-	    (let ((user (or user "root"))
-		  (host (or (file-remote-p default-directory 'host)
-			    "localhost"))
-		  (dir (file-local-name (expand-file-name default-directory)))
-		  (prefix (file-remote-p default-directory)))
-	      ;; `eshell-eval-using-options' reads options of COMMAND.
-	      (while (and (stringp (car orig-args))
-			  (member (car orig-args) '("-u" "--user")))
-		(setq orig-args (cddr orig-args)))
-	      (let ((default-directory
-		      (if (and prefix
-			       (or
-				(not
-				 (string-equal
-				  "sudo"
-				  (file-remote-p default-directory 'method)))
-				(not
-				 (string-equal
-				  user
-				  (file-remote-p default-directory 'user)))))
-			  (format "%s|sudo:%s@%s:%s"
-				  (substring prefix 0 -1) user host dir)
-			(format "/sudo:%s@%s:%s" user host dir))))
-		(eshell-named-command (car orig-args) (cdr orig-args))))))))
+   (throw 'eshell-external
+          (let* ((user (or user "root"))
+                 (host (or (file-remote-p default-directory 'host)
+                           "localhost"))
+                 (dir (file-local-name (expand-file-name default-directory)))
+                 (prefix (file-remote-p default-directory))
+                 (default-directory
+                   (if (and prefix
+                            (or
+                             (not
+                              (string-equal
+                               "sudo"
+                               (file-remote-p default-directory 'method)))
+                             (not
+                              (string-equal
+                               user
+                               (file-remote-p default-directory 'user)))))
+                       (format "%s|sudo:%s@%s:%s"
+                               (substring prefix 0 -1) user host dir)
+                     (format "/sudo:%s@%s:%s" user host dir))))
+            (eshell-named-command (car args) (cdr args))))))
 
 (put 'eshell/sudo 'eshell-no-numeric-conversions t)
 
diff --git a/lisp/eshell/esh-opt.el b/lisp/eshell/esh-opt.el
index c802bee3af5..8c29fff8096 100644
--- a/lisp/eshell/esh-opt.el
+++ b/lisp/eshell/esh-opt.el
@@ -97,10 +97,10 @@ let-bound variable `args'."
   (declare (debug (form form sexp body)))
   `(let* ((temp-args
            ,(if (memq ':preserve-args (cadr options))
-                macro-args
+                (list 'copy-tree macro-args)
               (list 'eshell-stringify-list
                     (list 'flatten-tree macro-args))))
-          (processed-args (eshell--do-opts ,name ,options temp-args))
+          (processed-args (eshell--do-opts ,name ,options temp-args ,macro-args))
           ,@(delete-dups
              (delq nil (mapcar (lambda (opt)
                                  (and (listp opt) (nth 3 opt)
@@ -117,7 +117,7 @@ let-bound variable `args'."
 ;; Documented part of the interface; see eshell-eval-using-options.
 (defvar eshell--args)
 
-(defun eshell--do-opts (name options args)
+(defun eshell--do-opts (name options args orig-args)
   "Helper function for `eshell-eval-using-options'.
 This code doesn't really need to be macro expanded everywhere."
   (require 'esh-ext)
@@ -135,7 +135,7 @@ This code doesn't really need to be macro expanded everywhere."
                (error "%s" usage-msg))))))
     (if ext-command
         (throw 'eshell-external
-               (eshell-external-command ext-command args))
+               (eshell-external-command ext-command orig-args))
       args)))
 
 (defun eshell-show-usage (name options)
diff --git a/test/lisp/eshell/em-tramp-tests.el b/test/lisp/eshell/em-tramp-tests.el
new file mode 100644
index 00000000000..7f054da5143
--- /dev/null
+++ b/test/lisp/eshell/em-tramp-tests.el
@@ -0,0 +1,85 @@
+;;; em-tramp-tests.el --- em-tramp 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/>.
+
+;;; Code:
+
+(require 'ert)
+(require 'em-tramp)
+
+(ert-deftest em-tramp-test/su-default ()
+  "Test Eshell `su' command with no arguments."
+  (should (equal
+           (catch 'eshell-replace-command (eshell/su))
+           `(eshell-trap-errors
+             (eshell-named-command
+              "cd"
+              (list ,(format "/su:root@localhost:%s" default-directory)))))))
+
+(ert-deftest em-tramp-test/su-user ()
+  "Test Eshell `su' command with USER argument."
+  (should (equal
+           (catch 'eshell-replace-command (eshell/su "USER"))
+           `(eshell-trap-errors
+             (eshell-named-command
+              "cd"
+              (list ,(format "/su:USER@localhost:%s" default-directory)))))))
+
+(ert-deftest em-tramp-test/su-login ()
+  "Test Eshell `su' command with -/-l/--login option."
+  (dolist (args '(("--login")
+                  ("-l")
+                  ("-")))
+    (should (equal
+             (catch 'eshell-replace-command (apply #'eshell/su args))
+             `(eshell-trap-errors
+               (eshell-named-command
+                "cd"
+                (list "/su:root@localhost:~/")))))))
+
+(defun mock-eshell-named-command (&rest args)
+  "Dummy function to test Eshell `sudo' command rewriting."
+  (list default-directory args))
+
+(ert-deftest em-tramp-test/sudo-basic ()
+  "Test Eshell `sudo' command with default user."
+  (cl-letf (((symbol-function 'eshell-named-command)
+             #'mock-eshell-named-command))
+    (should (equal
+             (catch 'eshell-external (eshell/sudo "echo" "hi"))
+             `(,(format "/sudo:root@localhost:%s" default-directory)
+               ("echo" ("hi")))))
+    (should (equal
+             (catch 'eshell-external (eshell/sudo "echo" "-u" "hi"))
+             `(,(format "/sudo:root@localhost:%s" default-directory)
+               ("echo" ("-u" "hi")))))))
+
+(ert-deftest em-tramp-test/sudo-user ()
+  "Test Eshell `sudo' command with specified user."
+  (cl-letf (((symbol-function 'eshell-named-command)
+             #'mock-eshell-named-command))
+    (should (equal
+             (catch 'eshell-external (eshell/sudo "-u" "USER" "echo" "hi"))
+             `(,(format "/sudo:USER@localhost:%s" default-directory)
+               ("echo" ("hi")))))
+    (should (equal
+             (catch 'eshell-external (eshell/sudo "-u" "USER" "echo" "-u" "hi"))
+             `(,(format "/sudo:USER@localhost:%s" default-directory)
+               ("echo" ("-u" "hi")))))))
+
+;;; em-tramp-tests.el ends here
diff --git a/test/lisp/eshell/esh-opt-tests.el b/test/lisp/eshell/esh-opt-tests.el
index b76ed8866df..4331c02ff5b 100644
--- a/test/lisp/eshell/esh-opt-tests.el
+++ b/test/lisp/eshell/esh-opt-tests.el
@@ -22,8 +22,8 @@
 (require 'ert)
 (require 'esh-opt)
 
-(ert-deftest esh-opt-process-args-test ()
-  "Unit tests which verify correct behavior of `eshell--process-args'."
+(ert-deftest esh-opt-test/process-args ()
+  "Test behavior of `eshell--process-args'."
   (should
    (equal '(t)
           (eshell--process-args
@@ -35,7 +35,10 @@
           (eshell--process-args
            "sudo" '("-u" "root" "world")
            '((?u "user" t user
-                 "execute a command as another USER")))))
+                 "execute a command as another USER"))))))
+
+(ert-deftest esh-opt-test/process-args-parse-leading-options-only ()
+  "Test behavior of :parse-leading-options-only in `eshell--process-args'."
   (should
    (equal '(nil "emerge" "-uDN" "world")
           (eshell--process-args
@@ -55,9 +58,10 @@
           (eshell--process-args
            "sudo" '("-u" "root" "emerge" "-uDN" "world")
            '((?u "user" t user
-                 "execute a command as another USER")))))
+                 "execute a command as another USER"))))))
 
-  ;; Test :external.
+(ert-deftest esh-opt-test/process-args-external ()
+  "Test behavior of :external in `eshell--process-args'."
   (cl-letf (((symbol-function 'eshell-search-path) #'ignore))
     (should
      (equal '(nil "/some/path")
@@ -85,9 +89,8 @@
         :external "ls"))
      :type 'error)))
 
-(ert-deftest test-eshell-eval-using-options ()
-  "Tests for `eshell-eval-using-options'."
-  ;; Test short options.
+(ert-deftest esh-opt-test/eval-using-options-short ()
+  "Test `eshell-eval-using-options' with short options."
   (eshell-eval-using-options
    "ls" '("-a" "/some/path")
    '((?a "all" nil show-all
@@ -99,17 +102,19 @@
    '((?a "all" nil show-all
          "do not ignore entries starting with ."))
    (should (eq show-all nil))
-   (should (equal args '("/some/path"))))
+   (should (equal args '("/some/path")))))
 
-  ;; Test long options.
+(ert-deftest esh-opt-test/eval-using-options-long ()
+  "Test `eshell-eval-using-options' with long options."
   (eshell-eval-using-options
    "ls" '("--all" "/some/path")
    '((?a "all" nil show-all
          "do not ignore entries starting with ."))
    (should (eq show-all t))
-   (should (equal args '("/some/path"))))
+   (should (equal args '("/some/path")))))
 
-  ;; Test options with constant values.
+(ert-deftest esh-opt-test/eval-using-options-constant ()
+  "Test `eshell-eval-using-options' with options with constant values."
   (eshell-eval-using-options
    "ls" '("/some/path" "-h")
    '((?h "human-readable" 1024 human-readable
@@ -127,9 +132,10 @@
    '((?h "human-readable" 1024 human-readable
          "print sizes in human readable format"))
    (should (eq human-readable nil))
-   (should (equal args '("/some/path"))))
+   (should (equal args '("/some/path")))))
 
-  ;; Test options with user-specified values.
+(ert-deftest esh-opt-test/eval-using-options-user-specified ()
+  "Test `eshell-eval-using-options' with options with user-specified values."
   (eshell-eval-using-options
    "ls" '("-I" "*.txt" "/some/path")
    '((?I "ignore" t ignore-pattern
@@ -153,9 +159,10 @@
    '((?I "ignore" t ignore-pattern
          "do not list implied entries matching pattern"))
    (should (equal ignore-pattern "*.txt"))
-   (should (equal args '("/some/path"))))
+   (should (equal args '("/some/path")))))
 
-  ;; Test multiple short options in a single token.
+(ert-deftest esh-opt-test/eval-using-options-short-single-token ()
+  "Test `eshell-eval-using-options' with multiple short options in one token."
   (eshell-eval-using-options
    "ls" '("-al" "/some/path")
    '((?a "all" nil show-all
@@ -173,9 +180,10 @@
          "do not list implied entries matching pattern"))
    (should (eq t show-all))
    (should (equal ignore-pattern "*.txt"))
-   (should (equal args '("/some/path"))))
+   (should (equal args '("/some/path")))))
 
-  ;; Test that "--" terminates options.
+(ert-deftest esh-opt-test/eval-using-options-terminate-options ()
+  "Test that \"--\" terminates options in `eshell-eval-using-options'."
   (eshell-eval-using-options
    "ls" '("--" "-a")
    '((?a "all" nil show-all
@@ -187,9 +195,10 @@
    '((?a "all" nil show-all
          "do not ignore entries starting with ."))
    (should (eq show-all nil))
-   (should (equal args '("--all"))))
+   (should (equal args '("--all")))))
 
-  ;; Test :parse-leading-options-only.
+(ert-deftest esh-opt-test/eval-using-options-parse-leading-options-only ()
+  "Test :parse-leading-options-only in `eshell-eval-using-options'."
   (eshell-eval-using-options
    "sudo" '("-u" "root" "whoami")
    '((?u "user" t user "execute a command as another USER")
@@ -212,27 +221,47 @@
    '((?u "user" t user "execute a command as another USER")
      :parse-leading-options-only)
    (should (eq user nil))
-   (should (equal args '("emerge" "-uDN" "world"))))
+   (should (equal args '("emerge" "-uDN" "world")))))
 
-  ;; Test unrecognized options.
+(ert-deftest esh-opt-test/eval-using-options-unrecognized ()
+  "Test `eshell-eval-using-options' with unrecognized options."
   (should-error
    (eshell-eval-using-options
     "ls" '("-u" "/some/path")
-    '((?a "all" nil show-all
-          "do not ignore entries starting with ."))
-    (ignore show-all)))
+    '((?a "all" nil _show-all
+          "do not ignore entries starting with ."))))
   (should-error
    (eshell-eval-using-options
     "ls" '("-au" "/some/path")
-    '((?a "all" nil show-all
-          "do not ignore entries starting with ."))
-    (ignore show-all)))
+    '((?a "all" nil _show-all
+          "do not ignore entries starting with ."))))
   (should-error
    (eshell-eval-using-options
     "ls" '("--unrecognized" "/some/path")
-    '((?a "all" nil show-all
-          "do not ignore entries starting with ."))
-    (ignore show-all))))
+    '((?a "all" nil _show-all
+          "do not ignore entries starting with .")))))
+
+(ert-deftest esh-opt-test/eval-using-options-external ()
+  "Test :external in `eshell-eval-using-options'."
+  (cl-letf (((symbol-function 'eshell-search-path) #'identity)
+            ((symbol-function 'eshell-external-command) #'list))
+    (should
+     (equal (catch 'eshell-external
+              (eshell-eval-using-options
+               "ls" '("/some/path" "-u")
+               '((?a "all" nil _show-all
+                     "do not ignore entries starting with .")
+                 :external "ls")))
+            '("ls" ("/some/path" "-u"))))
+    (should
+     (equal (catch 'eshell-external
+              (eshell-eval-using-options
+               "ls" '("/some/path2" "-u")
+               '((?a "all" nil _show-all
+                     "do not ignore entries starting with .")
+                 :preserve-args
+                 :external "ls")))
+            '("ls" ("/some/path2" "-u"))))))
 
 (provide 'esh-opt-tests)