From a176dc9717a801cb5ad1d0621c75ddf6a742222f Mon Sep 17 00:00:00 2001 From: Jim Porter Date: Mon, 24 Jan 2022 21:03:42 -0800 Subject: [PATCH 1/2] 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. --- lisp/eshell/em-tramp.el | 125 ++++++++++++++--------------- lisp/eshell/esh-opt.el | 8 +- test/lisp/eshell/em-tramp-tests.el | 85 ++++++++++++++++++++ test/lisp/eshell/esh-opt-tests.el | 91 ++++++++++++++------- 4 files changed, 209 insertions(+), 100 deletions(-) create mode 100644 test/lisp/eshell/em-tramp-tests.el diff --git a/lisp/eshell/em-tramp.el b/lisp/eshell/em-tramp.el index e9018bdb93..791458822d 100644 --- a/lisp/eshell/em-tramp.el +++ b/lisp/eshell/em-tramp.el @@ -57,41 +57,42 @@ eshell-tramp-initialize (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 @@ eshell/sudo "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 c802bee3af..8c29fff809 100644 --- a/lisp/eshell/esh-opt.el +++ b/lisp/eshell/esh-opt.el @@ -97,10 +97,10 @@ eshell-eval-using-options (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 @@ eshell-eval-using-options ;; 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 @@ eshell--do-opts (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 0000000000..7f054da514 --- /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 . + +;;; 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 b76ed8866d..4331c02ff5 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 @@ esh-opt-process-args-test (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 @@ esh-opt-process-args-test (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 @@ esh-opt-process-args-test :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 @@ test-eshell-eval-using-options '((?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 @@ test-eshell-eval-using-options '((?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 @@ test-eshell-eval-using-options '((?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 @@ test-eshell-eval-using-options "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 @@ test-eshell-eval-using-options '((?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 @@ test-eshell-eval-using-options '((?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) -- 2.25.1