From c1260450dbaac75693563e998c9b9936c5e96c78 Mon Sep 17 00:00:00 2001 From: Jim Porter Date: Sun, 9 Jun 2024 15:21:08 -0700 Subject: [PATCH] Improve correctness of 'eshell-quote-argument' * lisp/eshell/esh-arg.el (eshell-quote-argument): Mention that this function is for use within Eshell buffers. (eshell-quote-backslash): Properly quote newlines. * lisp/eshell/em-unix.el (eshell/cat, eshell/du): Throw 'eshell-external' instead; that's what it's here for. * test/lisp/eshell/esh-proc-tests.el (esh-proc-test-quote-argument): Remove. (esh-proc-test/emacs-command): * test/lisp/eshell/esh-var-tests.el (esh-var-test/path-var/set) (esh-var-test/path-var/set-locally): Use 'eshell-quote-argument'. * test/lisp/eshell/em-unix-tests.el (em-unix-test/compile/interactive): Use 'shell-quote-argument' (Note: *not* 'eshell-...'). (cherry picked from commit 32a75ecc73b78ad922b2ae66d30b907b13e19cb8) --- lisp/eshell/em-unix.el | 6 ++---- lisp/eshell/esh-arg.el | 16 ++++++++++------ test/lisp/eshell/em-unix-tests.el | 9 +++------ test/lisp/eshell/esh-proc-tests.el | 10 +++------- test/lisp/eshell/esh-var-tests.el | 27 +++++++++++++++------------ 5 files changed, 33 insertions(+), 35 deletions(-) diff --git a/lisp/eshell/em-unix.el b/lisp/eshell/em-unix.el index e6bd0381a14..671573f38c5 100644 --- a/lisp/eshell/em-unix.el +++ b/lisp/eshell/em-unix.el @@ -654,8 +654,7 @@ symlink, then revert to the system's definition of cat." (throw 'special t))))) (let ((ext-cat (eshell-search-path "cat"))) (if ext-cat - (throw 'eshell-replace-command - (eshell-parse-command (eshell-quote-argument ext-cat) args)) + (throw 'eshell-external (eshell-external-command ext-cat args)) (if eshell-in-pipeline-p (error "Eshell's `cat' does not work in pipelines") (error "Eshell's `cat' cannot display one of the files given")))) @@ -921,8 +920,7 @@ external command." (if (string-equal (file-remote-p (expand-file-name arg) 'method) "ftp") (throw 'have-ange-path t)))))) - (throw 'eshell-replace-command - (eshell-parse-command (eshell-quote-argument ext-du) args)) + (throw 'eshell-external (eshell-external-command ext-du args)) (eshell-eval-using-options "du" args '((?a "all" nil show-all diff --git a/lisp/eshell/esh-arg.el b/lisp/eshell/esh-arg.el index 865c0cdf5f1..6fc700cce89 100644 --- a/lisp/eshell/esh-arg.el +++ b/lisp/eshell/esh-arg.el @@ -351,7 +351,8 @@ argument list in place of the value of the current argument." (defun eshell-quote-argument (string) "Return STRING with magic characters quoted. -Magic characters are those in `eshell-special-chars-outside-quoting'." +Magic characters are those in `eshell-special-chars-outside-quoting'. +For consistent results, only call this function within an Eshell buffer." (let ((index 0)) (mapconcat (lambda (c) (prog1 @@ -448,12 +449,15 @@ Point is left at the end of the arguments." (defun eshell-quote-backslash (string &optional index) "Intelligently backslash the character occurring in STRING at INDEX. -If the character is itself a backslash, it needs no escaping." +If the character is itself a backslash, it needs no escaping. If the +character is a newline, quote it using single-quotes." (let ((char (aref string index))) - (if (eq char ?\\) - (char-to-string char) - (if (memq char eshell-special-chars-outside-quoting) - (string ?\\ char))))) + (cond ((eq char ?\\) + (char-to-string char)) + ((eq char ?\n) + "'\n'") + ((memq char eshell-special-chars-outside-quoting) + (string ?\\ char))))) (defun eshell-parse-backslash () "Parse a single backslash (\\) character and the character after. diff --git a/test/lisp/eshell/em-unix-tests.el b/test/lisp/eshell/em-unix-tests.el index 7312fb831cd..490de17a76e 100644 --- a/test/lisp/eshell/em-unix-tests.el +++ b/test/lisp/eshell/em-unix-tests.el @@ -43,12 +43,9 @@ "#") (with-current-buffer "*compilation*" (forward-line 3) - (should (looking-at - ;; MS-Windows/DOS quote by unconditionally enclosing in - ;; double quotes. - (if (memq system-type '(windows-nt ms-dos)) - "\"echo\" \"hello\"" - "echo hello")))))) + (should (looking-at (regexp-quote + (mapconcat #'shell-quote-argument + '("echo" "hello") " "))))))) (ert-deftest em-unix-test/compile/noninteractive () "Check that `eshell/compile' writes to stdout noninteractively." diff --git a/test/lisp/eshell/esh-proc-tests.el b/test/lisp/eshell/esh-proc-tests.el index c1e8800f706..90bbb4fa14e 100644 --- a/test/lisp/eshell/esh-proc-tests.el +++ b/test/lisp/eshell/esh-proc-tests.el @@ -196,15 +196,11 @@ pipeline." ;; against; that way, users don't need to have GNU coreutils (or ;; similar) installed. -;; This is needed because system shell quoting semantics is not relevant -;; when Eshell is the shell. -(defun esh-proc-test-quote-argument (argument) - "Quote ARGUMENT using Posix semantics." - (shell-quote-argument argument t)) - (defsubst esh-proc-test/emacs-command (command) "Evaluate COMMAND in a new Emacs batch instance." - (mapconcat #'esh-proc-test-quote-argument + ;; Call `eshell-quote-argument' from within an Eshell buffer since its + ;; behavior depends on activating various Eshell modules. + (mapconcat (lambda (arg) (with-temp-eshell (eshell-quote-argument arg))) `(,(expand-file-name invocation-name invocation-directory) "-Q" "--batch" "--eval" ,(prin1-to-string command)) " ")) diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el index 8b2f882f37e..6b0e225f05f 100644 --- a/test/lisp/eshell/esh-var-tests.el +++ b/test/lisp/eshell/esh-var-tests.el @@ -855,24 +855,27 @@ the value of the $PAGER env var." (let* ((path-to-set-list '("/some/path" "/other/path")) (path-to-set (string-join path-to-set-list (path-separator)))) (with-temp-eshell - ;; Quote PATH value, because on Windows path-separator is ';'. - (eshell-match-command-output (concat "set PATH \"" path-to-set "\"") - (concat path-to-set "\n")) - (eshell-match-command-output "echo $PATH" (concat path-to-set "\n")) - (should (equal (eshell-get-path t) path-to-set-list))))) + ;; Quote PATH value, because on Windows path-separator is ';'. + (eshell-match-command-output + (concat "set PATH " (eshell-quote-argument path-to-set) "") + (concat path-to-set "\n")) + (eshell-match-command-output "echo $PATH" (concat path-to-set "\n")) + (should (equal (eshell-get-path t) path-to-set-list))))) (ert-deftest esh-var-test/path-var/set-locally () "Test setting $PATH temporarily for a single command." (let* ((path-to-set-list '("/some/path" "/other/path")) (path-to-set (string-join path-to-set-list (path-separator)))) (with-temp-eshell - (eshell-match-command-output (concat "set PATH \"" path-to-set "\"") - (concat path-to-set "\n")) - (eshell-match-command-output "PATH=/local/path env" - "PATH=/local/path\n") - ;; After the last command, the previous $PATH value should be restored. - (eshell-match-command-output "echo $PATH" (concat path-to-set "\n")) - (should (equal (eshell-get-path t) path-to-set-list))))) + ;; As above, quote PATH value. + (eshell-match-command-output + (concat "set PATH " (eshell-quote-argument path-to-set) "") + (concat path-to-set "\n")) + (eshell-match-command-output "PATH=/local/path env" + "PATH=/local/path\n") + ;; After the last command, the previous $PATH value should be restored. + (eshell-match-command-output "echo $PATH" (concat path-to-set "\n")) + (should (equal (eshell-get-path t) path-to-set-list))))) (ert-deftest esh-var-test/path-var/preserve-across-hosts () "Test that $PATH can be set independently on multiple hosts." -- 2.39.2