bug#65110: eshell variable triggers error

bug#65110: eshell variable triggers error

[Raoul Comninos]

[-- Attachment #1: Type: text/plain, Size: 340 bytes --]

If I set this variable:

(setq eshell-list-files-after-cd t)

Emacs egnerates an error in eshell when changing directories:

Assertion failed: (> (cdar handle) 0), 0

The error disappears if the variable is set to nil.

I am running:GNU Emacs 30.0.50 (build 1, x86_64-w64-mingw32) of 2023-08-03

Windows 11

Kindest regards,
Raoul Comninos

[-- Attachment #2: Type: text/html, Size: 2069 bytes --]

bug#65110: eshell variable triggers error

[Jim Porter]

On 8/6/2023 1:51 AM, Raoul Comninos wrote:
> If I set this variable:
> 
> (setq eshell-list-files-after-cd t)
> 
> Emacs egnerates an error in eshell when changing directories:
> 
> Assertion failed: (> (cdar handle) 0), 0

It looks like this regressed due to commit 
073da412a139e317959f56e359ed12de726a0a35, which fixed bug#59545. I'll 
take a look at resolving this.

The I/O refcounting code in Eshell is pretty tricky. I have a WIP patch 
that will hopefully make it less error-prone, but I'll worry about that 
later...

bug#65110: eshell variable triggers error

[Jim Porter]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

On 8/6/2023 12:20 PM, Jim Porter wrote:
> The I/O refcounting code in Eshell is pretty tricky. I have a WIP patch 
> that will hopefully make it less error-prone, but I'll worry about that 
> later...

Happily, this issue *wasn't* a bug in the I/O refcounting code. The 
failed assertion (which is new in Emacs 30 and exists for exactly this 
reason) just revealed a bug in another part of Eshell. Fix attached 
(plus fixing another issue mentioned in the code comments) with some 
regression tests.

Let me know if this works for you, or if you see any further issues.

[-- Attachment #2: 0001-Fix-listing-of-directory-contents-after-cd-in-Eshell.patch --]
[-- Type: text/plain, Size: 2961 bytes --]

From b6460d4a47fa048b06872d89721578401793ec98 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sun, 6 Aug 2023 13:34:18 -0700
Subject: [PATCH] Fix listing of directory contents after "cd" in Eshell

* lisp/eshell/em-dirs.el (eshell/cd): Ensure we don't close the I/O
handles prematurely.  Additionally, don't clobber the "cd" command's
last-command info.

* test/lisp/eshell/em-dirs-tests.el (em-dirs-test/cd):
(em-dirs-test/cd/list-files-after-cd): New tests (bug#65110).
---
 lisp/eshell/em-dirs.el            | 10 +++++++---
 test/lisp/eshell/em-dirs-tests.el | 23 +++++++++++++++++++++++
 2 files changed, 30 insertions(+), 3 deletions(-)

diff --git a/lisp/eshell/em-dirs.el b/lisp/eshell/em-dirs.el
index 5284df9ab59..640d3676750 100644
--- a/lisp/eshell/em-dirs.el
+++ b/lisp/eshell/em-dirs.el
@@ -429,9 +429,13 @@ eshell/cd
 	  (and eshell-cd-shows-directory
 	       (eshell-printn result)))
 	(run-hooks 'eshell-directory-change-hook)
-	(if eshell-list-files-after-cd
-	    ;; Let-bind eshell-last-command around this?
-	    (eshell-plain-command "ls" (cdr args)))
+        (when eshell-list-files-after-cd
+          ;; Call "ls", but don't update the last-command information.
+          (let ((eshell-last-command-name)
+                (eshell-last-command-status)
+                (eshell-last-arguments))
+            (eshell-protect
+             (eshell-plain-command "ls" (cdr args)))))
 	nil))))
 
 (put 'eshell/cd 'eshell-no-numeric-conversions t)
diff --git a/test/lisp/eshell/em-dirs-tests.el b/test/lisp/eshell/em-dirs-tests.el
index d30b3d7d73f..9864b72ba78 100644
--- a/test/lisp/eshell/em-dirs-tests.el
+++ b/test/lisp/eshell/em-dirs-tests.el
@@ -99,4 +99,27 @@ em-dirs-test/directory-ring-var-indices
      (eshell-match-command-output "echo $-[1][/ 1 3]"
                                   "(\"some\" \"here\")\n"))))
 
+(ert-deftest em-dirs-test/cd ()
+  "Test that changing directories with `cd' works."
+  (ert-with-temp-directory tmpdir
+    (write-region "text" nil (expand-file-name "file.txt" tmpdir))
+    (with-temp-eshell
+     (eshell-match-command-output (format "cd '%s'" tmpdir)
+                                  "\\`\\'")
+     (should (equal default-directory tmpdir)))))
+
+(ert-deftest em-dirs-test/cd/list-files-after-cd ()
+  "Test that listing files after `cd' works."
+  (let ((eshell-list-files-after-cd t))
+    (ert-with-temp-directory tmpdir
+      (write-region "text" nil (expand-file-name "file.txt" tmpdir))
+      (with-temp-eshell
+       (eshell-match-command-output (format "cd '%s'" tmpdir)
+                                    "file.txt\n")
+       (should (equal default-directory tmpdir))
+       ;; Make sure we didn't update the last-command information when
+       ;; running "ls".
+       (should (equal eshell-last-command-name "#<function eshell/cd>"))
+       (should (equal eshell-last-arguments (list tmpdir)))))))
+
 ;; em-dirs-tests.el ends here
-- 
2.25.1

bug#65110: eshell variable triggers error

[Jim Porter]

On 8/6/2023 1:44 PM, Jim Porter wrote:
> Happily, this issue *wasn't* a bug in the I/O refcounting code. The 
> failed assertion (which is new in Emacs 30 and exists for exactly this 
> reason) just revealed a bug in another part of Eshell. Fix attached 
> (plus fixing another issue mentioned in the code comments) with some 
> regression tests.
> 
> Let me know if this works for you, or if you see any further issues.

After discussing this off-list, this looks good (thanks for testing!). 
Merged to master as 301e6a747ac, and closing this now.