* bug#53517: 29.0.50; [PATCH] `eshell-eval-using-options' :preserve-args breaks :external handling in some cases
@ 2022-01-25 1:11 Jim Porter
2022-01-25 5:19 ` Jim Porter
0 siblings, 1 reply; 4+ messages in thread
From: Jim Porter @ 2022-01-25 1:11 UTC (permalink / raw)
To: 53517
[-- Attachment #1: Type: text/plain, Size: 1960 bytes --]
To see this in action:
$ touch foo
$ emacs -Q
M-x eshell
cp foo bar -s
ls -l bar
`bar' should be a symlink pointing to `foo', but it's just a regular file.
This happens because, when :preserve-args is true,
`eshell-eval-using-options' doesn't flatten and stringify the args to be
parsed. That results in the various helper functions modifying the list
of raw args in-place. When :external is set, if `eshell--process-option'
finds an option it doesn't recognize, it throws, telling
`eshell--do-opts' to run the external command instead. That requires
having the original args, un-tampered with, in order to forward on to
the external command.
Attached is a patch that fixes this by copying the raw args list for the
:preserve-args case before manipulating it. I considered only adding
this when :external is also set, but I think it's useful for callers to
be able to inspect the raw args afterwards. This is used by `eshell/su'
to check for the presence of a "-" argument, since
`eshell-eval-using-options' doesn't handle that. (It might be nice to
fix that too, but I think it's still useful to keep the original raw
args around unchanged. Improving "-" support could be done later.)
As part of this, I also added several unit tests for `eshell/su' and
`eshell/sudo' to make sure I didn't break anything there; these should
pass both before and after the rest of the patch. I also added tests to
make sure the original arguments are preserved for calling the external
command; these should fail before the patch, and pass after.
There's just one remaining issue that I'm not sure how to fix: since I
updated the `eshell-eval-using-options' macro, any file that uses it
needs to be recompiled to get the new version. What's the right way to
tell the build system about this? I just ran `touch lisp/eshell/*.el'
locally, but I'm sure there's a better way so that people can just run
`make' after pulling this patch.
[-- Attachment #2: 0001-Don-t-manipulate-args-in-place-for-eshell-eval-using.patch --]
[-- Type: text/plain, Size: 20391 bytes --]
From ef27596fc63357d37248b15575c64af555f75a97 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 24 Jan 2022 15:46:01 -0800
Subject: [PATCH] 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 | 89 +++++++++++++-------
4 files changed, 208 insertions(+), 99 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 <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..ec3f74ef40 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
@@ -175,7 +182,8 @@ test-eshell-eval-using-options
(should (equal ignore-pattern "*.txt"))
(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
^ permalink raw reply related [flat|nested] 4+ messages in thread
* bug#53517: 29.0.50; [PATCH] `eshell-eval-using-options' :preserve-args breaks :external handling in some cases
2022-01-25 1:11 bug#53517: 29.0.50; [PATCH] `eshell-eval-using-options' :preserve-args breaks :external handling in some cases Jim Porter
@ 2022-01-25 5:19 ` Jim Porter
2022-01-25 6:29 ` Jim Porter
2022-01-25 12:30 ` Lars Ingebrigtsen
0 siblings, 2 replies; 4+ messages in thread
From: Jim Porter @ 2022-01-25 5:19 UTC (permalink / raw)
To: 53517
[-- Attachment #1: Type: text/plain, Size: 779 bytes --]
On 1/24/2022 5:11 PM, Jim Porter wrote:
> (It might be nice to fix that too, but I think it's still useful to
> keep the original raw args around unchanged. Improving "-" support
> could be done later.)
Actually, I'll do that now. Doing so fixes an issue I found in the
implementation of `eshell/cat'. "cat -" should read from stdin (and the
Eshell implementation should use the external /bin/cat to do this), but
it was getting parsed as "cat", so it erroneously thought there were
*no* input files. Previously, `eshell--process-args' saw the "-" and
started to look for short options, found none, and then just shrugged
and ignored that token entirely.
I also fixed an issue with the first patch where I had some misplaced
parens in test/lisp/eshell/esh-opt-tests.el.
[-- Attachment #2: 0001-Don-t-manipulate-args-in-place-for-eshell-eval-using.patch --]
[-- Type: text/plain, Size: 20529 bytes --]
From a176dc9717a801cb5ad1d0621c75ddf6a742222f Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
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 <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 @@ 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
[-- Attachment #3: 0002-Treat-as-a-positional-arg-in-eshell-eval-using-optio.patch --]
[-- Type: text/plain, Size: 4354 bytes --]
From 21d1b727413e8649436431806ec6a2731c9fd28b Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 24 Jan 2022 21:08:50 -0800
Subject: [PATCH 2/2] Treat "-" as a positional arg in
'eshell-eval-using-options'
* lisp/eshell/esh-opt.el (eshell--process-args): Treat "-" as a
positional arg.
* lisp/eshell/em-tramp.el (eshell/su): Simplify checking for "-".
* test/lisp/eshell/esh-opt-tests.el
(esh-opt-test/eval-using-options-stdin): New test.
---
lisp/eshell/em-tramp.el | 9 ++-------
lisp/eshell/esh-opt.el | 9 ++++++---
test/lisp/eshell/esh-opt-tests.el | 21 +++++++++++++++++++++
3 files changed, 29 insertions(+), 10 deletions(-)
diff --git a/lisp/eshell/em-tramp.el b/lisp/eshell/em-tramp.el
index 791458822d..2afd4fe066 100644
--- a/lisp/eshell/em-tramp.el
+++ b/lisp/eshell/em-tramp.el
@@ -57,13 +57,12 @@ eshell-tramp-initialize
(autoload 'eshell-parse-command "esh-cmd")
-(defun eshell/su (&rest arguments)
+(defun eshell/su (&rest args)
"Alias \"su\" to call TRAMP.
Uses the system su through TRAMP's su method."
- (setq arguments (eshell-stringify-list (flatten-tree arguments)))
(eshell-eval-using-options
- "su" arguments
+ "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")
@@ -77,10 +76,6 @@ eshell/su
(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
diff --git a/lisp/eshell/esh-opt.el b/lisp/eshell/esh-opt.el
index 8c29fff809..0961e214f4 100644
--- a/lisp/eshell/esh-opt.el
+++ b/lisp/eshell/esh-opt.el
@@ -283,6 +283,9 @@ eshell--process-args
(memq :parse-leading-options-only options))))
(setq arg (nth ai eshell--args))
(if (not (and (stringp arg)
+ ;; A string of length 1 can't be an option; (if
+ ;; it's "-", that generally means stdin).
+ (> (length arg) 1)
(string-match "^-\\(-\\)?\\(.*\\)" arg)))
;; Positional argument found, skip
(setq ai (1+ ai)
@@ -295,9 +298,9 @@ eshell--process-args
(if (> (length switch) 0)
(eshell--process-option name switch 1 ai options opt-vals)
(setq ai (length eshell--args)))
- (while (> (length switch) 0)
- (setq switch (eshell--process-option name switch 0
- ai options opt-vals)))))))
+ (while (> (length switch) 0)
+ (setq switch (eshell--process-option name switch 0
+ ai options opt-vals)))))))
(nconc (mapcar #'cdr opt-vals) eshell--args)))
(provide 'esh-opt)
diff --git a/test/lisp/eshell/esh-opt-tests.el b/test/lisp/eshell/esh-opt-tests.el
index 4331c02ff5..5b30de414a 100644
--- a/test/lisp/eshell/esh-opt-tests.el
+++ b/test/lisp/eshell/esh-opt-tests.el
@@ -182,6 +182,27 @@ esh-opt-test/eval-using-options-short-single-token
(should (equal ignore-pattern "*.txt"))
(should (equal args '("/some/path")))))
+(ert-deftest esh-opt-test/eval-using-options-stdin ()
+ "Test that \"-\" is a positional arg in `eshell-eval-using-options'."
+ (eshell-eval-using-options
+ "cat" '("-")
+ '((?A "show-all" nil show-all
+ "show all characters"))
+ (should (eq show-all nil))
+ (should (equal args '("-"))))
+ (eshell-eval-using-options
+ "cat" '("-A" "-")
+ '((?A "show-all" nil show-all
+ "show all characters"))
+ (should (eq show-all t))
+ (should (equal args '("-"))))
+ (eshell-eval-using-options
+ "cat" '("-" "-A")
+ '((?A "show-all" nil show-all
+ "show all characters"))
+ (should (eq show-all t))
+ (should (equal args '("-")))))
+
(ert-deftest esh-opt-test/eval-using-options-terminate-options ()
"Test that \"--\" terminates options in `eshell-eval-using-options'."
(eshell-eval-using-options
--
2.25.1
^ permalink raw reply related [flat|nested] 4+ messages in thread
* bug#53517: 29.0.50; [PATCH] `eshell-eval-using-options' :preserve-args breaks :external handling in some cases
2022-01-25 5:19 ` Jim Porter
@ 2022-01-25 6:29 ` Jim Porter
2022-01-25 12:30 ` Lars Ingebrigtsen
1 sibling, 0 replies; 4+ messages in thread
From: Jim Porter @ 2022-01-25 6:29 UTC (permalink / raw)
To: 53517
On 1/24/2022 9:19 PM, Jim Porter wrote:
> On 1/24/2022 5:11 PM, Jim Porter wrote:
>> (It might be nice to fix that too, but I think it's still useful to
>> keep the original raw args around unchanged. Improving "-" support
>> could be done later.)
>
> Actually, I'll do that now. Doing so fixes an issue I found in the
> implementation of `eshell/cat'. "cat -" should read from stdin (and the
> Eshell implementation should use the external /bin/cat to do this), but
> it was getting parsed as "cat", so it erroneously thought there were
> *no* input files.
Just a note: `cat' with no args *should* read from stdin, but this
doesn't actually work in Eshell yet; to read from stdin, you need to
explicitly say "cat -" or "*cat". I plan to fix these bits in a separate
bug, since I have a WIP patch series to allow piping to Lisp functions
in Eshell. That involves rewriting most of `eshell/cat', so I figured I
may as well just fix it once (later) instead of twice.
^ permalink raw reply [flat|nested] 4+ messages in thread
* bug#53517: 29.0.50; [PATCH] `eshell-eval-using-options' :preserve-args breaks :external handling in some cases
2022-01-25 5:19 ` Jim Porter
2022-01-25 6:29 ` Jim Porter
@ 2022-01-25 12:30 ` Lars Ingebrigtsen
1 sibling, 0 replies; 4+ messages in thread
From: Lars Ingebrigtsen @ 2022-01-25 12:30 UTC (permalink / raw)
To: Jim Porter; +Cc: 53517
Jim Porter <jporterbugs@gmail.com> writes:
> Actually, I'll do that now. Doing so fixes an issue I found in the
> implementation of `eshell/cat'. "cat -" should read from stdin (and
> the Eshell implementation should use the external /bin/cat to do
> this), but it was getting parsed as "cat", so it erroneously thought
> there were *no* input files. Previously, `eshell--process-args' saw
> the "-" and started to look for short options, found none, and then
> just shrugged and ignored that token entirely.
>
> I also fixed an issue with the first patch where I had some misplaced
> parens in test/lisp/eshell/esh-opt-tests.el.
Thanks; pushed to Emacs 28.
Jim Porter <jporterbugs@gmail.com> writes:
> There's just one remaining issue that I'm not sure how to fix: since I
> updated the `eshell-eval-using-options' macro, any file that uses it
> needs to be recompiled to get the new version. What's the right way to
> tell the build system about this? I just ran `touch lisp/eshell/*.el'
> locally, but I'm sure there's a better way so that people can just run
> `make' after pulling this patch.
We unfortunately don't have any system for handling build issues like
this, so people just have to say "make bootstrap" (or touch .el files or
delete .elc files) in these cases.
--
(domestic pets only, the antidote for overdose, milk.)
bloggy blog: http://lars.ingebrigtsen.no
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2022-01-25 12:30 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-25 1:11 bug#53517: 29.0.50; [PATCH] `eshell-eval-using-options' :preserve-args breaks :external handling in some cases Jim Porter
2022-01-25 5:19 ` Jim Porter
2022-01-25 6:29 ` Jim Porter
2022-01-25 12:30 ` Lars Ingebrigtsen
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).