[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
master dea24a0f7d 1/2: Don't manipulate args in-place for 'eshell-eval-u
|
From: |
Lars Ingebrigtsen |
|
Subject: |
master dea24a0f7d 1/2: Don't manipulate args in-place for 'eshell-eval-using-options' |
|
Date: |
Tue, 25 Jan 2022 07:31:05 -0500 (EST) |
branch: master
commit dea24a0f7d4ae42fae912dd724a770678054989a
Author: Jim Porter <jporterbugs@gmail.com>
Commit: Lars Ingebrigtsen <larsi@gnus.org>
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(-)
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 @@
(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 c802bee3af..8c29fff809 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 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 <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 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 @@
(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)