From e2ea5a24a01d008c7f2e8d0db59f19b9b042af5a Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 24 Jan 2023 17:14:54 -0800
Subject: [PATCH] Ensure that deferred commands don't make Eshell forget
 let-bound values

* lisp/eshell/esh-cmd.el (eshell-do-eval): Provide more detail in
docstring.  Handle 'eshell-defer' inside 'let' forms.
* test/lisp/eshell/esh-cmd-tests.el
(esh-cmd-test/let-rebinds-after-defer): New test (bug#59469).
---
 lisp/eshell/esh-cmd.el            | 61 ++++++++++++++++++++++++-------
 test/lisp/eshell/esh-cmd-tests.el | 17 +++++++++
 2 files changed, 65 insertions(+), 13 deletions(-)

diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 99c3d7f627d..414934081b0 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -1095,9 +1095,17 @@ eshell-manipulate
        (eshell-debug-command ,(concat "done " (eval tag)) form))))
 
 (defun eshell-do-eval (form &optional synchronous-p)
-  "Evaluate form, simplifying it as we go.
+  "Evaluate FORM, simplifying it as we go.
 Unless SYNCHRONOUS-P is non-nil, throws `eshell-defer' if it needs to
-be finished later after the completion of an asynchronous subprocess."
+be finished later after the completion of an asynchronous subprocess.
+
+As this function evaluates FORM, it will gradually replace
+subforms with the (quoted) result of evaluating them.  For
+example, a function call is replaced with the result of the call.
+This allows us to resume evaluation of FORM after something
+inside throws `eshell-defer' simply by calling this function
+again.  Any forms preceding one that throw `eshell-defer' will
+have been replaced by constants."
   (cond
    ((not (listp form))
     (list 'quote (eval form)))
@@ -1161,21 +1169,48 @@ eshell-do-eval
 	(setcar (cdr args) (eshell-do-eval (cadr args) synchronous-p))
 	(eval form))
        ((eq (car form) 'let)
-	(if (not (eq (car (cadr args)) 'eshell-do-eval))
-	    (eshell-manipulate "evaluating let args"
-	      (dolist (letarg (car args))
-		(if (and (listp letarg)
-			 (not (eq (cadr letarg) 'quote)))
-		    (setcdr letarg
-			    (list (eshell-do-eval
-				   (cadr letarg) synchronous-p)))))))
+        (when (not (eq (car (cadr args)) 'eshell-do-eval))
+          (eshell-manipulate "evaluating let args"
+            (dolist (letarg (car args))
+              (when (and (listp letarg)
+                         (not (eq (cadr letarg) 'quote)))
+                (setcdr letarg
+                        (list (eshell-do-eval
+                               (cadr letarg) synchronous-p)))))))
         (cl-progv
-            (mapcar (lambda (binding) (if (consp binding) (car binding) binding))
+            (mapcar (lambda (binding)
+                      (if (consp binding) (car binding) binding))
                     (car args))
             ;; These expressions should all be constants now.
-            (mapcar (lambda (binding) (if (consp binding) (eval (cadr binding))))
+            (mapcar (lambda (binding)
+                      (when (consp binding) (eval (cadr binding))))
                     (car args))
-	  (eshell-do-eval (macroexp-progn (cdr args)) synchronous-p)))
+          (let (deferred result)
+            ;; Evaluate the `let' body, catching `eshell-defer' so we
+            ;; can handle it below.
+            (setq deferred
+                  (catch 'eshell-defer
+                    (ignore (setq result (eshell-do-eval
+                                          (macroexp-progn (cdr args))
+                                          synchronous-p)))))
+            ;; If something threw `eshell-defer', we need to update
+            ;; the let-bindings' values so that those values are
+            ;; correct when we resume evaluation of this form.
+            (when deferred
+              (eshell-manipulate "rebinding let args after `eshell-defer'"
+                (let ((bindings (car args)))
+                  (while bindings
+                    (let ((binding (if (consp (car bindings))
+                                       (caar bindings)
+                                     (car bindings))))
+                      (setcar bindings
+                              (list binding
+                                    (list 'quote (symbol-value binding)))))
+                    (pop bindings))))
+              (throw 'eshell-defer deferred))
+            ;; If we get here, there was no `eshell-defer' thrown, so
+            ;; just return the `let' body's result.
+            result)))
        ((memq (car form) '(catch condition-case unwind-protect))
 	;; `condition-case' and `unwind-protect' have to be
 	;; handled specially, because we only want to call
diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el
index bcecc9a531f..94763954622 100644
--- a/test/lisp/eshell/esh-cmd-tests.el
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -73,6 +73,23 @@ esh-cmd-test/subcommand-lisp
 e.g. \"{(+ 1 2)} 3\" => 3"
   (eshell-command-result-equal "{(+ 1 2)} 3" 3))
 
+(ert-deftest esh-cmd-test/let-rebinds-after-defer ()
+  "Test that let-bound values are properly updated after `eshell-defer'.
+When inside a `let' block in an Eshell command form, we need to
+ensure that deferred commands update any let-bound variables so
+they have the correct values when resuming evaluation.  See
+bug#59469."
+  (skip-unless (executable-find "echo"))
+  (with-temp-eshell
+   (eshell-match-command-output
+    (concat "{"
+            "  export LOCAL=value; "
+            "  echo \"$LOCAL\"; "
+            "  *echo external; "        ; This will throw `eshell-defer'.
+            "  echo \"$LOCAL\"; "
+            "}")
+    "value\nexternal\nvalue\n")))
+
 
 ;; Lisp forms
 
-- 
2.25.1