On 11/21/2022 11:18 PM, Milan Zimmermann wrote:
> But it sounds to me like your intuition about this could be fixed by 
> rewriting the core 'eshell-do-eval' loop in bug#57635 can be correct.  I 
> would enjoy helping with  it, but at the moment it is above my time and 
> elisp abilities.

After digging through 'eshell-do-eval' for another issue, I think I 
mostly understand it now. I still think bug#57635 is the way to go 
long-term (either that or use real threads), but since that's a big 
change, it might be best to give users some time where they could opt 
out of some new Eshell evaluation backend, just in case we break 
something with it. As such, I want to make sure the existing backend is 
as bug-free as possible so that (ideally) switching between the two has 
no behavioral difference.

With that said, here's a patch that fixes this, plus a regression test. 
One question for anyone reviewing the patch: is there a better way to do 
the "catch and rethrow" dance I do in 'eshell-do-eval'? It seems kind of 
clumsy...

----------------------------------------

The patch has a description of the problem, but I'll describe it in more 
detail here for anyone curious. Eshell turns the command in the original 
message into a form like this (note: this is very approximate):

   (let ((process-environment process-environment))
     (setenv "var" "value")
     (eshell-external-command "grep") ; This throws 'eshell-defer'
     (eshell/echo (getenv "var")))

'eshell-do-eval' gradually steps through this form, evaluating subforms 
and replacing them with their (quoted) results. This way, when a command 
throws 'eshell-defer', you can resume this form simply by calling 
'eshell-do-eval' again. So at the point 'eshell-defer' gets thrown, the 
form has been updated to:

   (let ((process-environment process-environment))
     '"value"   ; The quoted result of 'setenv' above.
     (eshell-external-command "grep")
     (eshell/echo (getenv "var")))

But since 'process-environment' is let-bound, when we resume evaluation, 
it's as though 'setenv' had never been called at all!

The fix here is that when we're inside a 'let' and see 'eshell-defer' 
get thrown, update the let-bindings in place. So now the updated form 
would look like:

   (let ((process-environment (cons "var=value" process-environment)))
     '"value"   ; Not really necessary, but it doesn't hurt anything.
     (eshell-external-command "grep")
     (eshell/echo (getenv "var")))

And so with this, it all works.