From 84372672e3946ca0ea1a0ef05e84760a357dedd1 Mon Sep 17 00:00:00 2001 From: Jim Porter Date: Fri, 1 Nov 2024 10:40:25 -0700 Subject: [PATCH] Improve evaluation of conditional Eshell forms This simplifies the logic for building these forms and also fixes an issue where a subcommand in a "&&" or "||" conditional had its output suppressed. * lisp/eshell/esh-cmd.el (eshell-structure-basic-command): Make obsolete. (eshell-silence-test-command): New function... (eshell-rewrite-while-command, eshell-rewrite-if-command): ... use it, and make the command form ourselves. (eshell-parse-pipeline): Use 'and' and 'or' to make the conditional command sequence. (eshell-command-success): New macro. (eshell-do-eval): Add support for 'and' and 'or' forms. * test/lisp/eshell/esh-cmd-tests.el (esh-cmd-test/and-operator/output) (esh-cmd-test/or-operator/output): New tests. (cherry picked from commit 673c906a5b7255987c09f534de45e555fc7fc9ae) --- lisp/eshell/esh-cmd.el | 78 +++++++++++++++++++++---------- test/lisp/eshell/esh-cmd-tests.el | 30 ++++++++++++ 2 files changed, 84 insertions(+), 24 deletions(-) diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el index 137abe6eb75..6541430014a 100644 --- a/lisp/eshell/esh-cmd.el +++ b/lisp/eshell/esh-cmd.el @@ -556,6 +556,7 @@ implemented via rewriting, rather than as a function." The first of NAMES should be the positive form, and the second the negative. It's not likely that users should ever need to call this function." + (declare (obsolete nil "31.1")) (unless test (error "Missing test for `%s' command" keyword)) @@ -586,6 +587,12 @@ function." ;; Finally, create the form that represents this structured command. `(,func ,test ,@body)) +(defun eshell-silence-test-command (terms) + "If TERMS is a subcommand, wrap it in `eshell-commands' to silence output." + (if (memq (car-safe terms) '(eshell-as-subcommand eshell-lisp-command)) + `(eshell-command-success (eshell-commands ,terms t)) + terms)) + (defun eshell-rewrite-while-command (terms) "Rewrite a `while' command into its equivalent Eshell command form. Because the implementation of `while' relies upon conditional @@ -593,10 +600,13 @@ evaluation of its argument (i.e., use of a Lisp special form), it must be implemented via rewriting, rather than as a function." (when (and (stringp (car terms)) (member (car terms) '("while" "until"))) - (eshell-structure-basic-command - 'while '("while" "until") (car terms) - (cadr terms) - (caddr terms)))) + (unless (cadr terms) + (error "Missing test for `while' command")) + (let ((condition (eshell-silence-test-command (cadr terms)))) + (unless (string= (car terms) "while") + (setq condition `(not ,condition))) + `(while ,condition + ,(caddr terms))))) (defun eshell-rewrite-if-command (terms) "Rewrite an `if' command into its equivalent Eshell command form. @@ -605,18 +615,21 @@ evaluation of its argument (i.e., use of a Lisp special form), it must be implemented via rewriting, rather than as a function." (when (and (stringp (car terms)) (member (car terms) '("if" "unless"))) - (eshell-structure-basic-command - 'if '("if" "unless") (car terms) - (cadr terms) - (caddr terms) - (if (equal (nth 3 terms) "else") - ;; If there's an "else" keyword, allow chaining together - ;; multiple "if" forms... - (or (eshell-rewrite-if-command (nthcdr 4 terms)) - (nth 4 terms)) - ;; ... otherwise, only allow a single "else" block (without the - ;; keyword) as before for compatibility. - (nth 3 terms))))) + (unless (cadr terms) + (error "Missing test for `while' command")) + (let ((condition (eshell-silence-test-command (cadr terms))) + (then (caddr terms)) + (else (if (equal (nth 3 terms) "else") + ;; If there's an "else" keyword, allow chaining + ;; together multiple "if" forms... + (or (eshell-rewrite-if-command (nthcdr 4 terms)) + (nth 4 terms)) + ;; ... otherwise, only allow a single "else" block + ;; (without the keyword) as before for compatibility. + (nth 3 terms)))) + (unless (string= (car terms) "if") + (setq condition `(not ,condition))) + `(if ,condition ,then ,else)))) (defun eshell-set-exit-info (status &optional result) "Set the exit status and result for the last command. @@ -665,9 +678,10 @@ This means an exit code of 0." sep-terms (nreverse sep-terms)) (while results (cl-assert (car sep-terms)) - (setq final (eshell-structure-basic-command - 'if (string= (pop sep-terms) "&&") "if" - (pop results) final))) + (setq final `(,(if (string= (pop sep-terms) "&&") 'and 'or) + (eshell-command-success + (eshell-deferrable ,(pop results))) + ,final))) final)) (defun eshell-parse-subcommand-argument () @@ -751,12 +765,12 @@ if none)." ;; `eshell-do-eval' [Iterative evaluation]: ;; ;; @ Don't use special forms that conditionally evaluate their -;; arguments, such as `let*', unless Eshell explicitly supports -;; them. Eshell supports the following special forms: `catch', -;; `condition-case', `if', `let', `prog1', `progn', `quote', `setq', -;; `unwind-protect', and `while'. +;; arguments, such as `let*', unless Eshell explicitly supports them. +;; Eshell supports the following special forms: `and', `catch', +;; `condition-case', `if', `let', `or', `prog1', `progn', `quote', +;; `setq', `unwind-protect', and `while'. ;; -;; @ The two `special' variables are `eshell-current-handles' and +;; @ The two "special" variables are `eshell-current-handles' and ;; `eshell-current-subjob-p'. Bind them locally with a `let' if you ;; need to change them. Change them directly only if your intention ;; is to change the calling environment. @@ -803,6 +817,10 @@ returning it as (:eshell-background . PROCESSES)." (eshell-with-handles (,(not silent) 'append) ,object))) +(defmacro eshell-command-success (command) + "Return non-nil if COMMAND exits successfully." + `(progn ,command (eshell-exit-success-p))) + (defvar eshell-this-command-hook nil) (defmacro eshell-do-command (object) @@ -1182,6 +1200,18 @@ have been replaced by constants." (setcar form (car new-form)) (setcdr form (cdr new-form)))) (eshell-do-eval form synchronous-p)) + ((memq (car form) '(and or)) + (eshell-manipulate form (format-message "evaluating %s form" (car form)) + (let* ((result (eshell-do-eval (car args) synchronous-p)) + (value (cadr result))) + (if (or (null (cdr args)) + (if (eq (car form) 'or) value (not value))) + ;; If this is the last sub-form or we short-circuited, + ;; just return the result. + result + ;; Otherwise, remove this sub-form and re-evaluate. + (setcdr form (cdr args)) + (eshell-do-eval form synchronous-p))))) ((eq (car form) 'setcar) (setcar (cdr args) (eshell-do-eval (cadr args) synchronous-p)) (eval form)) diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el index 0f388a9eba4..8b68013c60d 100644 --- a/test/lisp/eshell/esh-cmd-tests.el +++ b/test/lisp/eshell/esh-cmd-tests.el @@ -176,6 +176,21 @@ bug#59469." (eshell-match-command-output "[ foo = bar ] && echo hi" "\\`\\'"))) +(ert-deftest esh-cmd-test/and-operator/output () + "Test output with logical && operator." + (skip-unless (executable-find "sh")) + (with-temp-eshell + ;; Direct commands + (eshell-match-command-output "sh -c 'echo one; exit 1' && echo two" + "\\`one\n\\'") + (eshell-match-command-output "echo one && echo two" + "\\`one\ntwo\n\\'") + ;; Subcommands + (eshell-match-command-output "{ sh -c 'echo one; exit 1' } && echo two" + "\\`one\n\\'") + (eshell-match-command-output "{ echo one } && echo two" + "\\`one\ntwo\n\\'"))) + (ert-deftest esh-cmd-test/or-operator () "Test logical || operator." (skip-unless (executable-find "[")) @@ -185,6 +200,21 @@ bug#59469." (eshell-match-command-output "[ foo = bar ] || echo hi" "hi\n"))) +(ert-deftest esh-cmd-test/or-operator/output () + "Test output with logical || operator." + (skip-unless (executable-find "sh")) + (with-temp-eshell + ;; Direct commands + (eshell-match-command-output "sh -c 'echo one; exit 1' || echo two" + "\\`one\ntwo\n\\'") + (eshell-match-command-output "echo one || echo two" + "\\`one\n\\'") + ;; Subcommands + (eshell-match-command-output "{ sh -c 'echo one; exit 1' } || echo two" + "\\`one\ntwo\n\\'") + (eshell-match-command-output "{ echo one } || echo two" + "\\`one\n\\'"))) + ;; Pipelines -- 2.39.5