Subject: Re: bug#57129: 29.0.50; Improve behavior of conditionals in Eshell

From: Jim Porter <[email-masked]>
Date: Wed, 10 Aug 2022 19:46:41 -0700

Here are some patches to fix this.

The first patch adds tests/documentation for the current state so that 
the subsequent patches are clearer. It also improves 
'eshell-close-handles' so that you can set the exit status elsewhere, 
which makes some of the code simpler.

The second patch fixes the use of variables in conditionals (e.g. if 
statements, while loops). The only non-test code change here is that 
'eshell-structure-basic-command' needed to be taught that forms 
beginning with 'eshell-escape-arg' should be treated as data, much like 
'eshell-convert'.

The third patch fixes the behavior of the '$?' and '$$' variables to 
match the manual: '$$' is t when an external command succeeds, and '$?' 
is 1 when a Lisp command signals an error. I also added a new feature 
that '$?' is 2 when an actual Lisp *form* returns nil. This lets you use 
Lisp forms as conditionals in Eshell much like you would in regular 
Lisp. However, it doesn't apply to the command syntax, even if it runs 
Lisp code:

   ~ $ (zerop 1); echo $?
   2
   ~ $ zerop 1; echo $?
   0

That's because Eshell prints the return value of Lisp commands (unless 
it's nil), so a Lisp command that wants to silently succeed would return 
nil. For example, the Lisp version of 'cat' returns nil. We want that to 
have an exit status of 0. However, when writing it as a Lisp form with 
parentheses, I think it makes more sense that nil is treated as false 
for conditionals. The overall goal is so that this prints hi:

   cat foo.txt && echo hi

And that this doesn't:

   (zerop 1) && echo hi

For people who don't like this behavior, they can customize the new 
'eshell-lisp-form-nil-is-failure' variable.
From 17d15df2368a2b078e90d39cb8018e274e422602 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Sat, 6 Aug 2022 13:37:28 -0700
Subject: [PATCH 1/3] Only set Eshell execution result metavariables when
 non-nil

This simplifies usage of 'eshell-close-handles' in several places and
makes it work more like the docstring indicated it would.

* lisp/eshell/esh-io.el (eshell-close-handles): Only store EXIT-CODE
and RESULT if they're non-nil.  Also, use 'dotimes' and 'dolist' to
simplify the implementation.

* lisp/eshell/em-alias.el (eshell-write-aliases-list):
* lisp/eshell/esh-cmd.el (eshell-rewrite-for-command)
(eshell-structure-basic-command): Adapt calls to
'eshell-close-handles'.

* test/lisp/eshell/eshell-tests.el (eshell-test/simple-command-result)
(eshell-test/lisp-command, eshell-test/lisp-command-with-quote)
(eshell-test/for-loop, eshell-test/for-name-loop)
(eshell-test/for-name-shadow-loop, eshell-test/lisp-command-args)
(eshell-test/subcommand, eshell-test/subcommand-args)
(eshell-test/subcommand-lisp): Move from here...

* test/lisp/eshell/esh-cmd-tests.el
(esh-cmd-test/simple-command-result, esh-cmd-test/lisp-command)
(esh-cmd-test/lisp-command-with-quote, esh-cmd-test/for-loop)
(esh-cmd-test/for-name-loop, esh-cmd-test/for-name-shadow-loop)
(esh-cmd-test/lisp-command-args, esh-cmd-test/subcommand)
(esh-cmd-test/subcommand-args, esh-cmd-test/subcommand-lisp): ... to
here.
(esh-cmd-test/and-operator, esh-cmd-test/or-operator)
(esh-cmd-test/for-loop-list, esh-cmd-test/for-loop-multiple-args)
(esh-cmd-test/while-loop, esh-cmd-test/until-loop)
(esh-cmd-test/if-statement, esh-cmd-test/if-else-statement)
(esh-cmd-test/unless-statement, esh-cmd-test/unless-else-statement):
New tests.

* doc/misc/eshell.texi (Invocation): Explain '&&' and '||'.
(for loop): Move from here...
(Control Flow): ... to here, and add documentation for other control
flow forms.
---
 doc/misc/eshell.texi              |  62 +++++++---
 lisp/eshell/em-alias.el           |   2 +-
 lisp/eshell/esh-cmd.el            |   8 +-
 lisp/eshell/esh-io.el             |  50 ++++----
 test/lisp/eshell/esh-cmd-tests.el | 189 ++++++++++++++++++++++++++++++
 test/lisp/eshell/eshell-tests.el  |  53 ---------
 6 files changed, 261 insertions(+), 103 deletions(-)
 create mode 100644 test/lisp/eshell/esh-cmd-tests.el

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index 9f9c88582f..d643cb5096 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -201,7 +201,7 @@ Commands
 * Aliases::
 * History::
 * Completion::
-* for loop::
+* Control Flow::
 * Scripts::
 @end menu
 
@@ -219,12 +219,18 @@ Invocation
 external command.
 
 The semicolon (@code{;}) can be used to separate multiple command
-invocations on a single line.  A command invocation followed by an
-ampersand (@code{&}) will be run in the background.  Eshell has no job
-control, so you can not suspend or background the current process, or
-bring a background process into the foreground.  That said, background
-processes invoked from Eshell can be controlled the same way as any
-other background process in Emacs.
+invocations on a single line.  You can also separate commands with
+@code{&&} or @code{||}. When using @code{&&}, Eshell will execute the
+second command only if the first succeeds (i.e.@: has an exit
+status of 0); with @code{||}, Eshell will execute the second command
+only if the first fails.
+
+A command invocation followed by an ampersand (@code{&}) will be run
+in the background.  Eshell has no job control, so you can not suspend
+or background the current process, or bring a background process into
+the foreground.  That said, background processes invoked from Eshell
+can be controlled the same way as any other background process in
+Emacs.
 
 @node Arguments
 @section Arguments
@@ -1008,19 +1014,41 @@ Completion
 the function @code{pcomplete/MAJOR-MODE/COMMAND} to define completions
 for a specific major mode.
 
-@node for loop
-@section @code{for} loop
+@node Control Flow
+@section Control Flow
 Because Eshell commands can not (easily) be combined with lisp forms,
-Eshell provides a command-oriented @command{for}-loop for convenience.
-The syntax is as follows:
+Eshell provides command-oriented control flow statements for
+convenience.
 
-@example
-@code{for VAR in TOKENS @{ command invocation(s) @}}
-@end example
+@table @code
+
+@item if @{ @var{conditional} @} @{ @var{true-commands} @}
+@itemx if @{ @var{conditional} @} @{ @var{true-commands} @} @{ @var{false-commands} @}
+Evaluate @var{true-commands} if @var{conditional} returns success
+(i.e.@: its exit code is zero); otherwise, evaluate
+@var{false-commands}.
+
+@item unless @{ @var{conditional} @} @{ @var{false-commands} @}
+@itemx unless @{ @var{conditional} @} @{ @var{false-commands} @} @{ @var{true-commands} @}
+Evaluate @var{false-commands} if @var{conditional} returns failure
+(i.e.@: its exit code is non-zero); otherwise, evaluate
+@var{true-commands}.
 
-where @samp{TOKENS} is a space-separated sequence of values of
-@var{VAR} for each iteration.  This can even be the output of a
-command if @samp{TOKENS} is replaced with @samp{@{ command invocation @}}.
+@item while @{ @var{conditional} @} @{ @var{commands} @}
+Repeatedly evaluate @var{commands} so long as @var{conditional}
+returns success.
+
+@item until @{ @var{conditional} @} @{ @var{commands} @}
+Repeatedly evaluate @var{commands} so long as @var{conditional}
+returns failure.
+
+@item for @var{var} in @var{list}@dots{} @{ @var{commands} @}
+Iterate over each element of of @var{list}, storing the element in
+@var{var} and evaluating @var{commands}.  If @var{list} is not a list,
+treat it as a list of one element.  If you specify multiple
+@var{lists}, this will iterate over each of them in turn.
+
+@end table
 
 @node Scripts
 @section Scripts
diff --git a/lisp/eshell/em-alias.el b/lisp/eshell/em-alias.el
index 5d3aaf7c81..9ad218d598 100644
--- a/lisp/eshell/em-alias.el
+++ b/lisp/eshell/em-alias.el
@@ -206,7 +206,7 @@ eshell-write-aliases-list
       (let ((eshell-current-handles
 	     (eshell-create-handles eshell-aliases-file 'overwrite)))
 	(eshell/alias)
-	(eshell-close-handles 0))))
+	(eshell-close-handles 0 'nil))))
 
 (defsubst eshell-lookup-alias (name)
   "Check whether NAME is aliased.  Return the alias if there is one."
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 775e4c1057..96272ca1a3 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -541,9 +541,7 @@ eshell-rewrite-for-command
 	   	,(eshell-invokify-arg body t)))
 	     (setcar for-items (cadr for-items))
 	     (setcdr for-items (cddr for-items)))
-           (eshell-close-handles
-            eshell-last-command-status
-            (list 'quote eshell-last-command-result))))))
+           (eshell-close-handles)))))
 
 (defun eshell-structure-basic-command (func names keyword test body
 					    &optional else)
@@ -574,9 +572,7 @@ eshell-structure-basic-command
   `(let ((eshell-command-body '(nil))
          (eshell-test-body '(nil)))
      (,func ,test ,body ,else)
-     (eshell-close-handles
-        eshell-last-command-status
-        (list 'quote eshell-last-command-result))))
+     (eshell-close-handles)))
 
 (defun eshell-rewrite-while-command (terms)
   "Rewrite a `while' command into its equivalent Eshell command form.
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 68e52a2c9c..27703976f6 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -254,6 +254,30 @@ eshell-protect-handles
       (setq idx (1+ idx))))
   handles)
 
+(defun eshell-close-handles (&optional exit-code result handles)
+  "Close all of the current HANDLES, taking refcounts into account.
+If HANDLES is nil, use `eshell-current-handles'.
+
+EXIT-CODE is the process exit code (zero, if the command
+completed successfully).  If nil, then use the exit code already
+set in `eshell-last-command-status'.
+
+RESULT is the quoted value of the last command.  If nil, then use
+the value already set in `eshell-last-command-result'."
+  (when exit-code
+    (setq eshell-last-command-status exit-code))
+  (when result
+    (cl-assert (eq (car result) 'quote))
+    (setq eshell-last-command-result (cadr result)))
+  (let ((handles (or handles eshell-current-handles)))
+    (dotimes (idx eshell-number-of-handles)
+      (when-let ((handle (aref handles idx)))
+        (setcdr handle (1- (cdr handle)))
+	(when (= (cdr handle) 0)
+          (dolist (target (ensure-list (car (aref handles idx))))
+            (eshell-close-target target (= eshell-last-command-status 0)))
+          (setcar handle nil))))))
+
 (defun eshell-close-target (target status)
   "Close an output TARGET, passing STATUS as the result.
 STATUS should be non-nil on successful termination of the output."
@@ -305,32 +329,6 @@ eshell-close-target
    ((consp target)
     (apply (car target) status (cdr target)))))
 
-(defun eshell-close-handles (exit-code &optional result handles)
-  "Close all of the current handles, taking refcounts into account.
-EXIT-CODE is the process exit code; mainly, it is zero, if the command
-completed successfully.  RESULT is the quoted value of the last
-command.  If nil, then the meta variables for keeping track of the
-last execution result should not be changed."
-  (let ((idx 0))
-    (cl-assert (or (not result) (eq (car result) 'quote)))
-    (setq eshell-last-command-status exit-code
-	  eshell-last-command-result (cadr result))
-    (while (< idx eshell-number-of-handles)
-      (let ((handles (or handles eshell-current-handles)))
-	(when (aref handles idx)
-	  (setcdr (aref handles idx)
-		  (1- (cdr (aref handles idx))))
-	  (when (= (cdr (aref handles idx)) 0)
-	    (let ((target (car (aref handles idx))))
-	      (if (not (listp target))
-		  (eshell-close-target target (= exit-code 0))
-		(while target
-		  (eshell-close-target (car target) (= exit-code 0))
-		  (setq target (cdr target)))))
-	    (setcar (aref handles idx) nil))))
-      (setq idx (1+ idx)))
-    nil))
-
 (defun eshell-kill-append (string)
   "Call `kill-append' with STRING, if it is indeed a string."
   (if (stringp string)
diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el
new file mode 100644
index 0000000000..1d5cd29d7c
--- /dev/null
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -0,0 +1,189 @@
+;;; esh-cmd-tests.el --- esh-cmd 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/>.
+
+;;; Commentary:
+
+;; Tests for Eshell's command invocation.
+
+;;; Code:
+
+(require 'ert)
+(require 'esh-mode)
+(require 'eshell)
+
+(require 'eshell-tests-helpers
+         (expand-file-name "eshell-tests-helpers"
+                           (file-name-directory (or load-file-name
+                                                    default-directory))))
+
+(defvar eshell-test-value nil)
+
+;;; Tests:
+
+
+;; Command invocation
+
+(ert-deftest esh-cmd-test/simple-command-result ()
+  "Test invocation with a simple command."
+  (should (equal (eshell-test-command-result "+ 1 2") 3)))
+
+(ert-deftest esh-cmd-test/lisp-command ()
+  "Test invocation with an elisp command."
+  (should (equal (eshell-test-command-result "(+ 1 2)") 3)))
+
+(ert-deftest esh-cmd-test/lisp-command-with-quote ()
+  "Test invocation with an elisp command containing a quote."
+  (should (equal (eshell-test-command-result "(eq 'foo nil)") nil)))
+
+(ert-deftest esh-cmd-test/lisp-command-args ()
+  "Test invocation with elisp and trailing args.
+Test that trailing arguments outside the S-expression are
+ignored.  e.g. \"(+ 1 2) 3\" => 3"
+  (should (equal (eshell-test-command-result "(+ 1 2) 3") 3)))
+
+(ert-deftest esh-cmd-test/subcommand ()
+  "Test invocation with a simple subcommand."
+  (should (equal (eshell-test-command-result "{+ 1 2}") 3)))
+
+(ert-deftest esh-cmd-test/subcommand-args ()
+  "Test invocation with a subcommand and trailing args.
+Test that trailing arguments outside the subcommand are ignored.
+e.g. \"{+ 1 2} 3\" => 3"
+  (should (equal (eshell-test-command-result "{+ 1 2} 3") 3)))
+
+(ert-deftest esh-cmd-test/subcommand-lisp ()
+  "Test invocation with an elisp subcommand and trailing args.
+Test that trailing arguments outside the subcommand are ignored.
+e.g. \"{(+ 1 2)} 3\" => 3"
+  (should (equal (eshell-test-command-result "{(+ 1 2)} 3") 3)))
+
+
+;; Logical operators
+
+(ert-deftest esh-cmd-test/and-operator ()
+  "Test logical && operator."
+  (skip-unless (executable-find "["))
+  (with-temp-eshell
+   (eshell-command-result-p "[ foo = foo ] && echo hi"
+                            "hi\n")
+   (eshell-command-result-p "[ foo = bar ] && echo hi"
+                            "\\`\\'")))
+
+(ert-deftest esh-cmd-test/or-operator ()
+  "Test logical || operator."
+  (skip-unless (executable-find "["))
+  (with-temp-eshell
+   (eshell-command-result-p "[ foo = foo ] || echo hi"
+                            "\\`\\'")
+   (eshell-command-result-p "[ foo = bar ] || echo hi"
+                            "hi\n")))
+
+
+;; Control flow statements
+
+(ert-deftest esh-cmd-test/for-loop ()
+  "Test invocation of a for loop."
+  (with-temp-eshell
+   (eshell-command-result-p "for i in 5 { echo $i }"
+                            "5\n")))
+
+(ert-deftest esh-cmd-test/for-loop-list ()
+  "Test invocation of a for loop iterating over a list."
+  (with-temp-eshell
+   (eshell-command-result-p "for i in (list 1 2 (list 3 4)) { echo $i }"
+                            "1\n2\n(3 4)\n")))
+
+(ert-deftest esh-cmd-test/for-loop-multiple-args ()
+  "Test invocation of a for loop iterating over multiple arguments."
+  (with-temp-eshell
+   (eshell-command-result-p "for i in 1 2 (list 3 4) { echo $i }"
+                            "1\n2\n3\n4\n")))
+
+(ert-deftest esh-cmd-test/for-name-loop () ; bug#15231
+  "Test invocation of a for loop using `name'."
+  (let ((process-environment (cons "name" process-environment)))
+    (should (equal (eshell-test-command-result
+                    "for name in 3 { echo $name }")
+                   3))))
+
+(ert-deftest esh-cmd-test/for-name-shadow-loop () ; bug#15372
+  "Test invocation of a for loop using an env-var."
+  (let ((process-environment (cons "name=env-value" process-environment)))
+    (with-temp-eshell
+     (eshell-command-result-p
+      "echo $name; for name in 3 { echo $name }; echo $name"
+      "env-value\n3\nenv-value\n"))))
+
+(ert-deftest esh-cmd-test/while-loop ()
+  "Test invocation of a while loop."
+  (skip-unless (executable-find "["))
+  (with-temp-eshell
+   (let ((eshell-test-value 0))
+     (eshell-command-result-p
+      (concat "while {[ $eshell-test-value -ne 3 ]} "
+              "{ setq eshell-test-value (1+ eshell-test-value) }")
+      "1\n2\n3\n"))))
+
+(ert-deftest esh-cmd-test/until-loop ()
+  "Test invocation of an until loop."
+  (skip-unless (executable-find "["))
+  (with-temp-eshell
+   (let ((eshell-test-value 0))
+     (eshell-command-result-p
+      (concat "until {[ $eshell-test-value -eq 3 ]} "
+              "{ setq eshell-test-value (1+ eshell-test-value) }")
+      "1\n2\n3\n"))))
+
+(ert-deftest esh-cmd-test/if-statement ()
+  "Test invocation of an if statement."
+  (skip-unless (executable-find "["))
+  (with-temp-eshell
+   (eshell-command-result-p "if {[ foo = foo ]} {echo yes}"
+                            "yes\n")
+   (eshell-command-result-p "if {[ foo = bar ]} {echo yes}"
+                            "\\`\\'")))
+
+(ert-deftest esh-cmd-test/if-else-statement ()
+  "Test invocation of an if/else statement."
+  (skip-unless (executable-find "["))
+  (with-temp-eshell
+   (eshell-command-result-p "if {[ foo = foo ]} {echo yes} {echo no}"
+                            "yes\n")
+   (eshell-command-result-p "if {[ foo = bar ]} {echo yes} {echo no}"
+                            "no\n")))
+
+(ert-deftest esh-cmd-test/unless-statement ()
+  "Test invocation of an unless statement."
+  (skip-unless (executable-find "["))
+  (with-temp-eshell
+   (eshell-command-result-p "unless {[ foo = foo ]} {echo no}"
+                            "\\`\\'")
+   (eshell-command-result-p "unless {[ foo = bar ]} {echo no}"
+                            "no\n")))
+
+(ert-deftest esh-cmd-test/unless-else-statement ()
+  "Test invocation of an unless/else statement."
+  (skip-unless (executable-find "["))
+  (with-temp-eshell
+   (eshell-command-result-p "unless {[ foo = foo ]} {echo no} {echo yes}"
+                            "yes\n")
+   (eshell-command-result-p "unless {[ foo = bar ]} {echo no} {echo yes}"
+                            "no\n")))
+
+;; esh-cmd-tests.el ends here
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index 5dc1877548..8423500ea7 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -36,59 +36,6 @@
 
 ;;; Tests:
 
-(ert-deftest eshell-test/simple-command-result ()
-  "Test `eshell-command-result' with a simple command."
-  (should (equal (eshell-test-command-result "+ 1 2") 3)))
-
-(ert-deftest eshell-test/lisp-command ()
-  "Test `eshell-command-result' with an elisp command."
-  (should (equal (eshell-test-command-result "(+ 1 2)") 3)))
-
-(ert-deftest eshell-test/lisp-command-with-quote ()
-  "Test `eshell-command-result' with an elisp command containing a quote."
-  (should (equal (eshell-test-command-result "(eq 'foo nil)") nil)))
-
-(ert-deftest eshell-test/for-loop ()
-  "Test `eshell-command-result' with a for loop.."
-  (let ((process-environment (cons "foo" process-environment)))
-    (should (equal (eshell-test-command-result
-                    "for foo in 5 { echo $foo }") 5))))
-
-(ert-deftest eshell-test/for-name-loop () ;Bug#15231
-  "Test `eshell-command-result' with a for loop using `name'."
-  (let ((process-environment (cons "name" process-environment)))
-    (should (equal (eshell-test-command-result
-                    "for name in 3 { echo $name }") 3))))
-
-(ert-deftest eshell-test/for-name-shadow-loop () ; bug#15372
-  "Test `eshell-command-result' with a for loop using an env-var."
-  (let ((process-environment (cons "name=env-value" process-environment)))
-    (with-temp-eshell
-     (eshell-command-result-p "echo $name; for name in 3 { echo $name }; echo $name"
-                              "env-value\n3\nenv-value\n"))))
-
-(ert-deftest eshell-test/lisp-command-args ()
-  "Test `eshell-command-result' with elisp and trailing args.
-Test that trailing arguments outside the S-expression are
-ignored.  e.g. \"(+ 1 2) 3\" => 3"
-  (should (equal (eshell-test-command-result "(+ 1 2) 3") 3)))
-
-(ert-deftest eshell-test/subcommand ()
-  "Test `eshell-command-result' with a simple subcommand."
-  (should (equal (eshell-test-command-result "{+ 1 2}") 3)))
-
-(ert-deftest eshell-test/subcommand-args ()
-  "Test `eshell-command-result' with a subcommand and trailing args.
-Test that trailing arguments outside the subcommand are ignored.
-e.g. \"{+ 1 2} 3\" => 3"
-  (should (equal (eshell-test-command-result "{+ 1 2} 3") 3)))
-
-(ert-deftest eshell-test/subcommand-lisp ()
-  "Test `eshell-command-result' with an elisp subcommand and trailing args.
-Test that trailing arguments outside the subcommand are ignored.
-e.g. \"{(+ 1 2)} 3\" => 3"
-  (should (equal (eshell-test-command-result "{(+ 1 2)} 3") 3)))
-
 (ert-deftest eshell-test/pipe-headproc ()
   "Check that piping a non-process to a process command waits for the process"
   (skip-unless (executable-find "cat"))
-- 
2.25.1

From ffc7688370f852d7425b7c67e6a53cf94a3e4208 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Mon, 8 Aug 2022 21:24:27 -0700
Subject: [PATCH 2/3] Allow using dollar expansions in Eshell conditionals

* lisp/eshell/esh-cmd.el (eshell-structure-basic-command): Forms
beginning with 'eshell-escape-arg' are "data-wise".

* test/lisp/eshell/esh-cmd-tests.el (esh-cmd-test/while-loop)
(esh-cmd-test/until-loop, esh-cmd-test/if-statement)
(esh-cmd-test/if-else-statement, esh-cmd-test/unless-statement)
(esh-cmd-test/unless-else-statement): Use variable interpolation.
(esh-cmd-test/while-loop-ext-cmd, esh-cmd-test/until-loop-ext-cmd)
(esh-cmd-test/if-else-statement-ext-cmd)
(esh-cmd-test/unless-else-statement-ext-cmd): New tests, adapted from
the existing ones.

* doc/misc/eshell.texi (Control Flow): Update documentation for
conditionals (bug#57129).
---
 doc/misc/eshell.texi              | 43 ++++++++++++----------
 lisp/eshell/esh-cmd.el            |  9 ++---
 test/lisp/eshell/esh-cmd-tests.el | 60 +++++++++++++++++++++++++------
 3 files changed, 79 insertions(+), 33 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index d643cb5096..141c30ae9b 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -1020,27 +1020,32 @@ Control Flow
 Eshell provides command-oriented control flow statements for
 convenience.
 
+Most of Eshell's control flow statements accept a @var{conditional}.
+This can take a few different forms.  If @var{conditional} is a dollar
+expansion, the condition is satisfied if the result is a
+non-@code{nil} value.  If @var{conditional} is a @samp{@{
+@var{subcommand} @}}, the condition is satisfied if the
+@var{subcommand}'s exit status is 0.
+
 @table @code
 
-@item if @{ @var{conditional} @} @{ @var{true-commands} @}
-@itemx if @{ @var{conditional} @} @{ @var{true-commands} @} @{ @var{false-commands} @}
-Evaluate @var{true-commands} if @var{conditional} returns success
-(i.e.@: its exit code is zero); otherwise, evaluate
-@var{false-commands}.
-
-@item unless @{ @var{conditional} @} @{ @var{false-commands} @}
-@itemx unless @{ @var{conditional} @} @{ @var{false-commands} @} @{ @var{true-commands} @}
-Evaluate @var{false-commands} if @var{conditional} returns failure
-(i.e.@: its exit code is non-zero); otherwise, evaluate
-@var{true-commands}.
-
-@item while @{ @var{conditional} @} @{ @var{commands} @}
-Repeatedly evaluate @var{commands} so long as @var{conditional}
-returns success.
-
-@item until @{ @var{conditional} @} @{ @var{commands} @}
-Repeatedly evaluate @var{commands} so long as @var{conditional}
-returns failure.
+@item if @var{conditional} @{ @var{true-commands} @}
+@itemx if @var{conditional} @{ @var{true-commands} @} @{ @var{false-commands} @}
+Evaluate @var{true-commands} if @var{conditional} is satisfied;
+otherwise, evaluate @var{false-commands}.
+
+@item unless @var{conditional} @{ @var{false-commands} @}
+@itemx unless @var{conditional} @{ @var{false-commands} @} @{ @var{true-commands} @}
+Evaluate @var{false-commands} if @var{conditional} is not satisfied;
+otherwise, evaluate @var{true-commands}.
+
+@item while @var{conditional} @{ @var{commands} @}
+Repeatedly evaluate @var{commands} so long as @var{conditional} is
+satisfied.
+
+@item until @var{conditional} @{ @var{commands} @}
+Repeatedly evaluate @var{commands} until @var{conditional} is
+satisfied.
 
 @item for @var{var} in @var{list}@dots{} @{ @var{commands} @}
 Iterate over each element of of @var{list}, storing the element in
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 96272ca1a3..454a90e91d 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -549,10 +549,11 @@ eshell-structure-basic-command
 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."
-  ;; If the test form begins with `eshell-convert', it means
-  ;; something data-wise will be returned, and we should let
-  ;; that determine the truth of the statement.
-  (unless (eq (car test) 'eshell-convert)
+  ;; If the test form begins with `eshell-convert' or
+  ;; `eshell-escape-arg', it means something data-wise will be
+  ;; returned, and we should let that determine the truth of the
+  ;; statement.
+  (unless (memq (car test) '(eshell-convert eshell-escape-arg))
     (setq test
 	  `(progn ,test
                   (eshell-exit-success-p))))
diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el
index 1d5cd29d7c..b31159a1a8 100644
--- a/test/lisp/eshell/esh-cmd-tests.el
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -132,6 +132,15 @@ esh-cmd-test/for-name-shadow-loop
 
 (ert-deftest esh-cmd-test/while-loop ()
   "Test invocation of a while loop."
+  (with-temp-eshell
+   (let ((eshell-test-value '(0 1 2)))
+     (eshell-command-result-p
+      (concat "while $eshell-test-value "
+              "{ setq eshell-test-value (cdr eshell-test-value) }")
+      "(1 2)\n(2)\n"))))
+
+(ert-deftest esh-cmd-test/while-loop-ext-cmd ()
+  "Test invocation of a while loop using an external command."
   (skip-unless (executable-find "["))
   (with-temp-eshell
    (let ((eshell-test-value 0))
@@ -142,6 +151,15 @@ esh-cmd-test/while-loop
 
 (ert-deftest esh-cmd-test/until-loop ()
   "Test invocation of an until loop."
+  (with-temp-eshell
+   (let ((eshell-test-value nil))
+     (eshell-command-result-p
+      (concat "until $eshell-test-value "
+              "{ setq eshell-test-value t }")
+      "t\n"))))
+
+(ert-deftest esh-cmd-test/until-loop-ext-cmd ()
+  "Test invocation of an until loop using an external command."
   (skip-unless (executable-find "["))
   (with-temp-eshell
    (let ((eshell-test-value 0))
@@ -152,15 +170,26 @@ esh-cmd-test/until-loop
 
 (ert-deftest esh-cmd-test/if-statement ()
   "Test invocation of an if statement."
-  (skip-unless (executable-find "["))
   (with-temp-eshell
-   (eshell-command-result-p "if {[ foo = foo ]} {echo yes}"
-                            "yes\n")
-   (eshell-command-result-p "if {[ foo = bar ]} {echo yes}"
-                            "\\`\\'")))
+   (let ((eshell-test-value t))
+     (eshell-command-result-p "if $eshell-test-value {echo yes}"
+                              "yes\n"))
+   (let ((eshell-test-value nil))
+     (eshell-command-result-p "if $eshell-test-value {echo yes}"
+                              "\\`\\'"))))
 
 (ert-deftest esh-cmd-test/if-else-statement ()
   "Test invocation of an if/else statement."
+  (with-temp-eshell
+   (let ((eshell-test-value t))
+     (eshell-command-result-p "if $eshell-test-value {echo yes} {echo no}"
+                              "yes\n"))
+   (let ((eshell-test-value nil))
+     (eshell-command-result-p "if $eshell-test-value {echo yes} {echo no}"
+                              "no\n"))))
+
+(ert-deftest esh-cmd-test/if-else-statement-ext-cmd ()
+  "Test invocation of an if/else statement using an external command."
   (skip-unless (executable-find "["))
   (with-temp-eshell
    (eshell-command-result-p "if {[ foo = foo ]} {echo yes} {echo no}"
@@ -170,15 +199,26 @@ esh-cmd-test/if-else-statement
 
 (ert-deftest esh-cmd-test/unless-statement ()
   "Test invocation of an unless statement."
-  (skip-unless (executable-find "["))
   (with-temp-eshell
-   (eshell-command-result-p "unless {[ foo = foo ]} {echo no}"
-                            "\\`\\'")
-   (eshell-command-result-p "unless {[ foo = bar ]} {echo no}"
-                            "no\n")))
+   (let ((eshell-test-value t))
+     (eshell-command-result-p "unless $eshell-test-value {echo no}"
+                              "\\`\\'"))
+   (let ((eshell-test-value nil))
+     (eshell-command-result-p "unless $eshell-test-value {echo no}"
+                              "no\n"))))
 
 (ert-deftest esh-cmd-test/unless-else-statement ()
   "Test invocation of an unless/else statement."
+  (with-temp-eshell
+   (let ((eshell-test-value t))
+     (eshell-command-result-p "unless $eshell-test-value {echo no} {echo yes}"
+                              "yes\n"))
+   (let ((eshell-test-value nil))
+     (eshell-command-result-p "unless $eshell-test-value {echo no} {echo yes}"
+                              "no\n"))))
+
+(ert-deftest esh-cmd-test/unless-else-statement-ext-cmd ()
+  "Test invocation of an unless/else statement using an external command."
   (skip-unless (executable-find "["))
   (with-temp-eshell
    (eshell-command-result-p "unless {[ foo = foo ]} {echo no} {echo yes}"
-- 
2.25.1

From 8233c6f41415e7b4379c652bd6b6ba26c514dd61 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Tue, 9 Aug 2022 20:09:57 -0700
Subject: [PATCH 3/3] Make '$?' and '$$' variables more consistent in Eshell

Previously, '$?' (last exit code) was only useful for external
commands, and '$$' (last result) was only useful for Lisp commands.

* lisp/eshell/esh-cmd.el (eshell-lisp-form-nil-is-failure): New
option.
(eshell-lisp-command): Set last exit code to 1 when the command
signals an error, and 2 if it returns nil (for Lisp forms only).

* lisp/eshell/esh-proc.el (eshell-sentinel): Set last result to t if
the command succeeded.

* test/lisp/eshell/esh-cmd-tests.el
(esh-cmd-test/while-loop-lisp-form, esh-cmd-test/until-loop-lisp-form)
(esh-cmd-test/if-else-statement-lisp-form)
(esh-cmd-test/if-else-statement-lisp-form-2)
(esh-cmd-test/unless-else-statement-lisp-form): New tests.

* test/lisp/eshell/esh-var-tests.el
(esh-var-test/last-status-var-lisp-command)
(esh-var-test/last-status-var-lisp-form)
(esh-var-test/last-status-var-lisp-form-2)
(esh-var-test/last-status-var-ext-cmd)
(esh-var-test/last-status-var-ext-cmd): New tests.
(esh-var-test/last-result-var2): Rename from this...
( esh-var-test/last-result-var-twice): ... to this.

* doc/misc/eshell.texi (Variables): Update documentation about '$?'
and '$$'.
(Control Flow): Mention that '(lisp forms)' can be used as
conditionals.

* etc/NEWS: Announce this change (bug#57129).
---
 doc/misc/eshell.texi              | 14 ++++---
 etc/NEWS                          |  7 ++++
 lisp/eshell/esh-cmd.el            | 68 +++++++++++++++++++------------
 lisp/eshell/esh-proc.el           | 68 +++++++++++++++----------------
 test/lisp/eshell/esh-cmd-tests.el | 54 ++++++++++++++++++++++++
 test/lisp/eshell/esh-var-tests.el | 56 ++++++++++++++++++++++++-
 6 files changed, 199 insertions(+), 68 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index 141c30ae9b..aae779575d 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -890,14 +890,18 @@ Variables
 
 @vindex $$
 @item $$
-This is the result of the last command.  In case of an external
-command, it is @code{t} or @code{nil}.
+This is the result of the last command.  For external commands, it is
+@code{t} if the exit code was 0 or @code{nil} otherwise.
 
+@vindex eshell-lisp-form-nil-is-failure
 @vindex $?
 @item $?
 This variable contains the exit code of the last command.  If the last
 command was a Lisp function, it is 0 for successful completion or 1
-otherwise.
+otherwise.  If @code{eshell-lisp-form-nil-is-failure} is
+non-@code{nil}, then a command with a Lisp form, like
+@samp{(@var{command} @var{args}@dots{})}, that returns @code{nil} will
+set this variable to 2.
 
 @vindex $COLUMNS
 @vindex $LINES
@@ -1024,8 +1028,8 @@ Control Flow
 This can take a few different forms.  If @var{conditional} is a dollar
 expansion, the condition is satisfied if the result is a
 non-@code{nil} value.  If @var{conditional} is a @samp{@{
-@var{subcommand} @}}, the condition is satisfied if the
-@var{subcommand}'s exit status is 0.
+@var{subcommand} @}} or @samp{(@var{lisp form})}, the condition is
+satisfied if the command's exit status is 0.
 
 @table @code
 
diff --git a/etc/NEWS b/etc/NEWS
index 8d54ccc0dc..21bf1cc050 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -2149,6 +2149,13 @@ Additionally, globs ending with '**/' or '***/' no longer raise an
 error, and now expand to all directories recursively (following
 symlinks in the latter case).
 
++++
+*** Lisp forms in Eshell now treat a 'nil' result as a failed exit status.
+When executing a command that looks like '(lisp form)', Eshell will
+set the exit status (available in the '$?' variable) to 2.  This
+allows commands like that to be used as conditionals.  To change this
+behavior, customize the new 'eshell-lisp-form-nil-is-failure' option.
+
 ** Shell
 
 ---
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 454a90e91d..62c95056fd 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -133,6 +133,10 @@ eshell-lisp-regexp
 Such arguments will be passed to `read', and then evaluated."
   :type 'regexp)
 
+(defcustom eshell-lisp-form-nil-is-failure t
+  "If non-nil, Lisp forms like (COMMAND ARGS) treat a nil result as failure."
+  :type 'boolean)
+
 (defcustom eshell-pre-command-hook nil
   "A hook run before each interactive command is invoked."
   :type 'hook)
@@ -1412,43 +1416,53 @@ eshell-last-output-end
 (defun eshell-lisp-command (object &optional args)
   "Insert Lisp OBJECT, using ARGS if a function."
   (catch 'eshell-external               ; deferred to an external command
+    (setq eshell-last-command-status 0
+          eshell-last-arguments args)
     (let* ((eshell-ensure-newline-p (eshell-interactive-output-p))
+           (command-form-p (functionp object))
            (result
-            (if (functionp object)
-                (progn
-                  (setq eshell-last-arguments args
-                        eshell-last-command-name
+            (if command-form-p
+                (let ((numeric (not (get object
+                                         'eshell-no-numeric-conversions)))
+                      (fname-args (get object 'eshell-filename-arguments)))
+                  (when (or numeric fname-args)
+                    (while args
+                      (let ((arg (car args)))
+                        (cond
+                         ((and numeric (stringp arg) (> (length arg) 0)
+                               (text-property-any 0 (length arg)
+                                                  'number t arg))
+                          ;; If any of the arguments are flagged as
+                          ;; numbers waiting for conversion, convert
+                          ;; them now.
+                          (setcar args (string-to-number arg)))
+                         ((and fname-args (stringp arg)
+                               (string-equal arg "~"))
+                          ;; If any of the arguments match "~",
+                          ;; prepend "./" to treat it as a regular
+                          ;; file name.
+                          (setcar args (concat "./" arg)))))
+                      (setq args (cdr args))))
+                  (setq eshell-last-command-name
                         (concat "#<function " (symbol-name object) ">"))
-                  (let ((numeric (not (get object
-                                           'eshell-no-numeric-conversions)))
-                        (fname-args (get object 'eshell-filename-arguments)))
-                    (when (or numeric fname-args)
-                      (while args
-                        (let ((arg (car args)))
-                          (cond ((and numeric (stringp arg) (> (length arg) 0)
-                                      (text-property-any 0 (length arg)
-                                                         'number t arg))
-                                 ;; If any of the arguments are
-                                 ;; flagged as numbers waiting for
-                                 ;; conversion, convert them now.
-                                 (setcar args (string-to-number arg)))
-                                ((and fname-args (stringp arg)
-                                      (string-equal arg "~"))
-                                 ;; If any of the arguments match "~",
-                                 ;; prepend "./" to treat it as a
-                                 ;; regular file name.
-                                 (setcar args (concat "./" arg)))))
-                        (setq args (cdr args)))))
                   (eshell-apply object eshell-last-arguments))
-              (setq eshell-last-arguments args
-                    eshell-last-command-name "#<Lisp object>")
+              (setq eshell-last-command-name "#<Lisp object>")
               (eshell-eval object))))
       (if (and eshell-ensure-newline-p
 	       (save-excursion
 		 (goto-char eshell-last-output-end)
 		 (not (bolp))))
 	  (eshell-print "\n"))
-      (eshell-close-handles 0 (list 'quote result)))))
+      (eshell-close-handles
+       ;; If `eshell-lisp-form-nil-is-failure' is non-nil, Lisp forms
+       ;; that succeeded but have a nil result should have an exit
+       ;; status of 2.
+       (when (and eshell-lisp-form-nil-is-failure
+                  (not command-form-p)
+                  (= eshell-last-command-status 0)
+                  (not result))
+         2)
+       (list 'quote result)))))
 
 (defalias 'eshell-lisp-command* #'eshell-lisp-command)
 
diff --git a/lisp/eshell/esh-proc.el b/lisp/eshell/esh-proc.el
index 99b43661f2..c367b5cd64 100644
--- a/lisp/eshell/esh-proc.el
+++ b/lisp/eshell/esh-proc.el
@@ -346,7 +346,9 @@ eshell-gather-process-output
         (defvar eshell-last-output-end)         ;Defined in esh-mode.el.
 	(eshell-update-markers eshell-last-output-end)
 	;; Simulate the effect of eshell-sentinel.
-	(eshell-close-handles (if (numberp exit-status) exit-status -1))
+	(eshell-close-handles
+         (if (numberp exit-status) exit-status -1)
+         (list 'quote (and (numberp exit-status) (= exit-status 0))))
 	(eshell-kill-process-function command exit-status)
 	(or (bound-and-true-p eshell-in-pipeline-p)
 	    (setq eshell-last-sync-output-start nil))
@@ -398,40 +400,36 @@ eshell-sentinel
   (when (buffer-live-p (process-buffer proc))
     (with-current-buffer (process-buffer proc)
       (unwind-protect
-          (let ((entry (assq proc eshell-process-list)))
-;	    (if (not entry)
-;		(error "Sentinel called for unowned process `%s'"
-;		       (process-name proc))
-	    (when entry
-	      (unwind-protect
-		  (progn
-		    (unless (string= string "run")
-                      ;; Write the exit message if the status is
-                      ;; abnormal and the process is already writing
-                      ;; to the terminal.
-                      (when (and (eq proc (eshell-tail-process))
-                                 (not (string-match "^\\(finished\\|exited\\)"
-                                                    string)))
-                        (funcall (process-filter proc) proc string))
-                      (let ((handles (nth 1 entry))
-                            (str (prog1 (nth 3 entry)
-                                   (setf (nth 3 entry) nil)))
-                            (status (process-exit-status proc)))
-                        ;; If we're in the middle of handling output
-                        ;; from this process then schedule the EOF for
-                        ;; later.
-                        (letrec ((finish-io
-                                  (lambda ()
-                                    (if (nth 4 entry)
-                                        (run-at-time 0 nil finish-io)
-                                      (when str
-                                        (ignore-error 'eshell-pipe-broken
-                                          (eshell-output-object
-                                           str nil handles)))
-                                      (eshell-close-handles
-                                       status 'nil handles)))))
-                          (funcall finish-io)))))
-		(eshell-remove-process-entry entry))))
+          (when-let ((entry (assq proc eshell-process-list)))
+	    (unwind-protect
+		(unless (string= string "run")
+                  ;; Write the exit message if the status is
+                  ;; abnormal and the process is already writing
+                  ;; to the terminal.
+                  (when (and (eq proc (eshell-tail-process))
+                             (not (string-match "^\\(finished\\|exited\\)"
+                                                string)))
+                    (funcall (process-filter proc) proc string))
+                  (let ((handles (nth 1 entry))
+                        (str (prog1 (nth 3 entry)
+                               (setf (nth 3 entry) nil)))
+                        (status (process-exit-status proc)))
+                    ;; If we're in the middle of handling output
+                    ;; from this process then schedule the EOF for
+                    ;; later.
+                    (letrec ((finish-io
+                              (lambda ()
+                                (if (nth 4 entry)
+                                    (run-at-time 0 nil finish-io)
+                                  (when str
+                                    (ignore-error 'eshell-pipe-broken
+                                      (eshell-output-object
+                                       str nil handles)))
+                                  (eshell-close-handles
+                                   status (list 'quote (= status 0))
+                                   handles)))))
+                      (funcall finish-io))))
+	      (eshell-remove-process-entry entry)))
 	(eshell-kill-process-function proc string)))))
 
 (defun eshell-process-interact (func &optional all query)
diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el
index b31159a1a8..e86985ec71 100644
--- a/test/lisp/eshell/esh-cmd-tests.el
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -139,6 +139,15 @@ esh-cmd-test/while-loop
               "{ setq eshell-test-value (cdr eshell-test-value) }")
       "(1 2)\n(2)\n"))))
 
+(ert-deftest esh-cmd-test/while-loop-lisp-form ()
+  "Test invocation of a while loop using a Lisp form."
+  (with-temp-eshell
+   (let ((eshell-test-value 0))
+     (eshell-command-result-p
+      (concat "while (/= eshell-test-value 3) "
+              "{ setq eshell-test-value (1+ eshell-test-value) }")
+      "1\n2\n3\n"))))
+
 (ert-deftest esh-cmd-test/while-loop-ext-cmd ()
   "Test invocation of a while loop using an external command."
   (skip-unless (executable-find "["))
@@ -158,6 +167,16 @@ esh-cmd-test/until-loop
               "{ setq eshell-test-value t }")
       "t\n"))))
 
+(ert-deftest esh-cmd-test/until-loop-lisp-form ()
+  "Test invocation of an until loop using a Lisp form."
+  (skip-unless (executable-find "["))
+  (with-temp-eshell
+   (let ((eshell-test-value 0))
+     (eshell-command-result-p
+      (concat "until (= eshell-test-value 3) "
+              "{ setq eshell-test-value (1+ eshell-test-value) }")
+      "1\n2\n3\n"))))
+
 (ert-deftest esh-cmd-test/until-loop-ext-cmd ()
   "Test invocation of an until loop using an external command."
   (skip-unless (executable-find "["))
@@ -188,6 +207,30 @@ esh-cmd-test/if-else-statement
      (eshell-command-result-p "if $eshell-test-value {echo yes} {echo no}"
                               "no\n"))))
 
+(ert-deftest esh-cmd-test/if-else-statement-lisp-form ()
+  "Test invocation of an if/else statement using a Lisp form."
+  (with-temp-eshell
+   (eshell-command-result-p "if (zerop 0) {echo yes} {echo no}"
+                            "yes\n")
+   (eshell-command-result-p "if (zerop 1) {echo yes} {echo no}"
+                            "no\n")
+   (let ((debug-on-error nil))
+     (eshell-command-result-p "if (zerop \"foo\") {echo yes} {echo no}"
+                              "no\n"))))
+
+(ert-deftest esh-cmd-test/if-else-statement-lisp-form-2 ()
+  "Test invocation of an if/else statement using a Lisp form.
+This tests when `eshell-lisp-form-nil-is-failure' is nil."
+  (let ((eshell-lisp-form-nil-is-failure nil))
+    (with-temp-eshell
+     (eshell-command-result-p "if (zerop 0) {echo yes} {echo no}"
+                              "yes\n")
+     (eshell-command-result-p "if (zerop 1) {echo yes} {echo no}"
+                              "yes\n")
+     (let ((debug-on-error nil))
+       (eshell-command-result-p "if (zerop \"foo\") {echo yes} {echo no}"
+                                "no\n")))))
+
 (ert-deftest esh-cmd-test/if-else-statement-ext-cmd ()
   "Test invocation of an if/else statement using an external command."
   (skip-unless (executable-find "["))
@@ -217,6 +260,17 @@ esh-cmd-test/unless-else-statement
      (eshell-command-result-p "unless $eshell-test-value {echo no} {echo yes}"
                               "no\n"))))
 
+(ert-deftest esh-cmd-test/unless-else-statement-lisp-form ()
+  "Test invocation of an unless/else statement using a Lisp form."
+  (with-temp-eshell
+   (eshell-command-result-p "unless (zerop 0) {echo no} {echo yes}"
+                            "yes\n")
+   (eshell-command-result-p "unless (zerop 1) {echo no} {echo yes}"
+                            "no\n")
+   (let ((debug-on-error nil))
+     (eshell-command-result-p "unless (zerop \"foo\") {echo no} {echo yes}"
+                              "no\n"))))
+
 (ert-deftest esh-cmd-test/unless-else-statement-ext-cmd ()
   "Test invocation of an unless/else statement using an external command."
   (skip-unless (executable-find "["))
diff --git a/test/lisp/eshell/esh-var-tests.el b/test/lisp/eshell/esh-var-tests.el
index 54e701a6aa..66dabd424b 100644
--- a/test/lisp/eshell/esh-var-tests.el
+++ b/test/lisp/eshell/esh-var-tests.el
@@ -500,18 +500,72 @@ esh-var-test/inside-emacs-var-split-indices
    (eshell-command-result-p "echo $INSIDE_EMACS[, 1]"
                             "eshell")))
 
+(ert-deftest esh-var-test/last-status-var-lisp-command ()
+  "Test using the \"last exit status\" ($?) variable with a Lisp command"
+  (with-temp-eshell
+   (eshell-command-result-p "zerop 0; echo $?"
+                            "t\n0\n")
+   (eshell-command-result-p "zerop 1; echo $?"
+                            "0\n")
+   (let ((debug-on-error nil))
+     (eshell-command-result-p "zerop foo; echo $?"
+                              "1\n"))))
+
+(ert-deftest esh-var-test/last-status-var-lisp-form ()
+  "Test using the \"last exit status\" ($?) variable with a Lisp form"
+  (let ((eshell-lisp-form-nil-is-failure t))
+  (with-temp-eshell
+   (eshell-command-result-p "(zerop 0); echo $?"
+                            "t\n0\n")
+   (eshell-command-result-p "(zerop 1); echo $?"
+                            "2\n")
+   (let ((debug-on-error nil))
+     (eshell-command-result-p "(zerop \"foo\"); echo $?"
+                              "1\n")))))
+
+(ert-deftest esh-var-test/last-status-var-lisp-form-2 ()
+  "Test using the \"last exit status\" ($?) variable with a Lisp form.
+This tests when `eshell-lisp-form-nil-is-failure' is nil."
+  (let ((eshell-lisp-form-nil-is-failure nil))
+    (with-temp-eshell
+     (eshell-command-result-p "(zerop 0); echo $?"
+                              "0\n")
+     (eshell-command-result-p "(zerop 0); echo $?"
+                              "0\n")
+     (let ((debug-on-error nil))
+       (eshell-command-result-p "(zerop \"foo\"); echo $?"
+                                "1\n")))))
+
+(ert-deftest esh-var-test/last-status-var-ext-cmd ()
+  "Test using the \"last exit status\" ($?) variable with an external command"
+  (skip-unless (executable-find "["))
+  (with-temp-eshell
+   (eshell-command-result-p "[ foo = foo ]; echo $?"
+                            "0\n")
+   (eshell-command-result-p "[ foo = bar ]; echo $?"
+                            "1\n")))
+
 (ert-deftest esh-var-test/last-result-var ()
   "Test using the \"last result\" ($$) variable"
   (with-temp-eshell
    (eshell-command-result-p "+ 1 2; + $$ 2"
                             "3\n5\n")))
 
-(ert-deftest esh-var-test/last-result-var2 ()
+(ert-deftest esh-var-test/last-result-var-twice ()
   "Test using the \"last result\" ($$) variable twice"
   (with-temp-eshell
    (eshell-command-result-p "+ 1 2; + $$ $$"
                             "3\n6\n")))
 
+(ert-deftest esh-var-test/last-result-var-ext-cmd ()
+  "Test using the \"last result\" ($$) variable with an external command"
+  (skip-unless (executable-find "["))
+  (with-temp-eshell
+   (eshell-command-result-p "[ foo = foo ]; format \"%s\" $$"
+                            "t\n")
+   (eshell-command-result-p "[ foo = bar ]; format \"%s\" $$"
+                            "nil\n")))
+
 (ert-deftest esh-var-test/last-result-var-split-indices ()
   "Test using the \"last result\" ($$) variable with split indices"
   (with-temp-eshell
-- 
2.25.1