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)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 137abe6eb7598fa0aee46f41053fb6c72744c6e5..6541430014a1cf95eec3b557796cd87e35d47474 100644 (file)
--- 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 0f388a9eba4158ae83c96ee2c3c3cbde72731973..8b68013c60d84463e1962f0d40ca28d40524b36c 100644 (file)
--- 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\\'")))
+
 \f
 ;; Pipelines