* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
@ 2022-11-24 15:49 Milan Zimmermann
2022-12-21 0:18 ` Jim Porter
0 siblings, 1 reply; 14+ messages in thread
From: Milan Zimmermann @ 2022-11-24 15:49 UTC (permalink / raw)
To: 59545
[-- Attachment #1: Type: text/plain, Size: 5616 bytes --]
Emacs 29 Eshell Bug: Sourcing 'redirect-echo.esh' and redirecting output
to a file, results in the first echo string ('hello') showing in eshell,
only the second ('there')(presumably because it is last) showing in the
output file.
To reproduce: Create a eshell script named 'redirect-echo.esh' with two
echo commands then source the script, redirecting the stdout to another
file or buffer,
Actual result:
tmp $ cat redirect-echo.esh
echo hello
echo there
tmp $ source redirect-echo.esh > redirect-echo.out
hello
tmp $ cat redirect-echo.out
theretmp $
Expected result:
tmp $ cat redirect-echo.esh
echo hello
echo there
tmp $ source redirect-echo.esh > redirect-echo.out
tmp $ cat redirect-echo.out
hello
there
tmp $
As a note, the same behavior if elisp "print" is used instead of
echo. Also the same behavior if I redirect output to an Emacs buffer
instead of the file.
In GNU Emacs 29.0.50 (build 1, x86_64-suse-linux-gnu, GTK+ Version
3.24.34, cairo version 1.17.6)
System Description: openSUSE Tumbleweed
Configured using:
'configure --host=x86_64-suse-linux-gnu --build=x86_64-suse-linux-gnu
--program-prefix= --disable-dependency-tracking --prefix=/usr
--exec-prefix=/usr --bindir=/usr/bin --sbindir=/usr/sbin
--sysconfdir=/etc --datadir=/usr/share --includedir=/usr/include
--libdir=/usr/lib64 --libexecdir=/usr/libexec --localstatedir=/var
--sharedstatedir=/var/lib --mandir=/usr/share/man
--infodir=/usr/share/info --disable-build-details --without-pop
--with-mailutils --without-hesiod --with-gameuser=:games
--with-kerberos --with-kerberos5 --with-file-notification=inotify
--with-modules --enable-autodepend --prefix=/usr
--mandir=/usr/share/man --infodir=/usr/share/info --datadir=/usr/share
--localstatedir=/var --sharedstatedir=/var/lib
--libexecdir=/usr/libexec --with-file-notification=yes
--enable-locallisppath=/usr/share/emacs/29.0.50/site-lisp:/usr/share/emacs/site-lisp
--without-x --with-json --without-xim --with-sound --with-xpm
--with-jpeg --with-tiff --with-gif --with-png --with-rsvg --with-dbus
--without-xft --without-gpm --with-pgtk --without-native-compilation
--with-toolkit-scroll-bars --with-libotf --with-m17n-flt --with-cairo
--without-xwidgets --with-dumping=pdumper 'CFLAGS=-O2 -Wall
-U_FORTIFY_SOURCE -D_FORTIFY_SOURCE=3 -fstack-protector-strong
-funwind-tables -fasynchronous-unwind-tables -fstack-clash-protection
-Werror=return-type -flto=auto -D_GNU_SOURCE
-DGDK_DISABLE_DEPRECATION_WARNINGS -DGLIB_DISABLE_DEPRECATION_WARNINGS'
LDFLAGS=-flto=auto'
Configured features:
ACL CAIRO DBUS FREETYPE GIF GLIB GMP GNUTLS GSETTINGS HARFBUZZ JPEG JSON
LCMS2 LIBOTF LIBSELINUX LIBSYSTEMD LIBXML2 MODULES NOTIFY INOTIFY
PDUMPER PGTK PNG RSVG SECCOMP SOUND SQLITE3 THREADS TIFF
TOOLKIT_SCROLL_BARS WEBP XIM GTK3 ZLIB
Important settings:
value of $LANG: en_CA.UTF-8
value of $XMODIFIERS: @im=ibus
locale-coding-system: utf-8-unix
Major mode: Eshell
Minor modes in effect:
shell-dirtrack-mode: t
eshell-prompt-mode: t
eshell-hist-mode: t
eshell-pred-mode: t
eshell-cmpl-mode: t
eshell-proc-mode: t
eshell-arg-mode: t
tooltip-mode: t
global-eldoc-mode: t
show-paren-mode: t
electric-indent-mode: t
mouse-wheel-mode: t
tool-bar-mode: t
menu-bar-mode: t
file-name-shadow-mode: t
global-font-lock-mode: t
font-lock-mode: t
blink-cursor-mode: t
line-number-mode: t
indent-tabs-mode: t
transient-mark-mode: t
auto-composition-mode: t
auto-encryption-mode: t
auto-compression-mode: t
Load-path shadows:
None found.
Features:
(shadow sort mail-extr emacsbug message mailcap yank-media puny dired
dired-loaddefs rfc822 mml mml-sec password-cache epa derived epg rfc6068
epg-config gnus-util text-property-search time-date mm-decode mm-bodies
mm-encode mail-parse rfc2231 mailabbrev gmm-utils mailheader sendmail
rfc2047 rfc2045 ietf-drums mm-util mail-prsvr mail-utils pcmpl-unix
em-unix em-term term disp-table shell subr-x ehelp em-script em-prompt
em-ls em-hist em-pred em-glob em-extpipe em-cmpl em-dirs esh-var
pcomplete comint ansi-osc ansi-color ring em-basic em-banner em-alias
esh-mode eshell esh-cmd generator esh-ext esh-opt esh-proc esh-io
esh-arg esh-module esh-groups esh-util cus-edit pp cus-start cus-load
icons wid-edit cl-loaddefs cl-lib files-x rmc iso-transl tooltip cconv
eldoc paren electric uniquify ediff-hook vc-hooks lisp-float-type
elisp-mode mwheel term/pgtk-win pgtk-win term/common-win pgtk-dnd
tool-bar dnd fontset image regexp-opt fringe tabulated-list replace
newcomment text-mode lisp-mode prog-mode register page tab-bar menu-bar
rfn-eshadow isearch easymenu timer select scroll-bar mouse jit-lock
font-lock syntax font-core term/tty-colors frame minibuffer nadvice seq
simple cl-generic indonesian philippine cham georgian utf-8-lang
misc-lang vietnamese tibetan thai tai-viet lao korean japanese eucjp-ms
cp51932 hebrew greek romanian slovak czech european ethiopic indian
cyrillic chinese composite emoji-zwj charscript charprop case-table
epa-hook jka-cmpr-hook help abbrev obarray oclosure cl-preloaded button
loaddefs theme-loaddefs faces cus-face macroexp files window
text-properties overlay sha1 md5 base64 format env code-pages mule
custom widget keymap hashtable-print-readable backquote threads dbusbind
inotify dynamic-setting system-font-setting font-render-setting cairo
gtk pgtk lcms2 multi-tty make-network-process emacs)
Memory information:
((conses 16 77374 9505)
(symbols 48 8750 0)
(strings 32 24407 2625)
(string-bytes 1 719677)
(vectors 16 15061)
(vector-slots 8 203823 6855)
(floats 8 34 37)
(intervals 56 583 0)
(buffers 984 12))
[-- Attachment #2: Type: text/html, Size: 6331 bytes --]
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
2022-11-24 15:49 bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file Milan Zimmermann
@ 2022-12-21 0:18 ` Jim Porter
2022-12-21 0:29 ` Jim Porter
0 siblings, 1 reply; 14+ messages in thread
From: Jim Porter @ 2022-12-21 0:18 UTC (permalink / raw)
To: Milan Zimmermann, 59545
[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]
On 11/24/2022 7:49 AM, Milan Zimmermann wrote:
> Emacs 29 Eshell Bug: Sourcing 'redirect-echo.esh' and redirecting output
> to a file, results in the first echo string ('hello') showing in eshell,
> only the second ('there')(presumably because it is last) showing in the
> output file.
It turns out there's an even simpler way to reproduce this:
~ $ {echo hi; echo bye} > #<buf>
hi ;; Buffer "buf" now contains the string "bye".
Initially[1], I said that this was an issue with the implementation of
'eshell-protect', but it turns out that it's actually an issue in an
adjacent part of the Eshell I/O code. Specifically, every statement in
Eshell gets its own set of default I/O handles, when it should actually
inherit the handles from its parent. So in the example above, "echo hi"
has the default I/O handles (pointing to the terminal), when its stdout
handle should point to the buffer "buf".
Attached is a patch series to fix this, with a bunch of new tests. I
also fixed a related issue where redirecting to /dev/null could clobber
your other redirects. (There's *also* an issue that should be fixed for
the release branch; I'll send that in a separate message.)
[1] https://lists.gnu.org/archive/html/emacs-devel/2022-11/msg01504.html
[-- Attachment #2: 0001-Add-eshell-duplicate-handles-to-return-a-copy-of-fil.patch --]
[-- Type: text/plain, Size: 9435 bytes --]
From 997c4e7e8343e87978fb6d921b0988c07dc6069d Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 19 Dec 2022 22:21:10 -0800
Subject: [PATCH 1/3] Add 'eshell-duplicate-handles' to return a copy of file
handles
* lisp/eshell/esh-io.el (eshell-create-handles): Support creating with
multiple targets for stdout and/or stderr. Make the targets for a
handle always be a list, and store whether the targets are the default
in a separate 'default' field.
(eshell-protect-handles, eshell-close-handles)
(eshell-copy-output-handle, eshell-interactive-output-p)
(eshell-output-object): Update for changes in 'eshell-create-handles'.
(eshell-duplicate-handles, eshell-get-targets): New functions.
* lisp/eshell/esh-cmd.el (eshell-copy-handles): Rename and alias to...
(eshell-with-copied-handles): ... this function, and use
'eshell-duplicate-handles'.
(eshell-execute-pipeline): Use 'eshell-duplicate-handles'.
---
lisp/eshell/esh-cmd.el | 20 ++++------
lisp/eshell/esh-io.el | 83 ++++++++++++++++++++++++++----------------
2 files changed, 60 insertions(+), 43 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 1fb84991120..03388236b06 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -788,16 +788,15 @@ eshell-trap-errors
(defvar eshell-output-handle) ;Defined in esh-io.el.
(defvar eshell-error-handle) ;Defined in esh-io.el.
-(defmacro eshell-copy-handles (object)
+(defmacro eshell-with-copied-handles (object)
"Duplicate current I/O handles, so OBJECT works with its own copy."
`(let ((eshell-current-handles
- (eshell-create-handles
- (car (aref eshell-current-handles
- eshell-output-handle)) nil
- (car (aref eshell-current-handles
- eshell-error-handle)) nil)))
+ (eshell-duplicate-handles eshell-current-handles)))
,object))
+(define-obsolete-function-alias 'eshell-copy-handles
+ #'eshell-with-copied-handles "30.1")
+
(defmacro eshell-protect (object)
"Protect I/O handles, so they aren't get closed after eval'ing OBJECT."
`(progn
@@ -808,7 +807,7 @@ eshell-do-pipelines
"Execute the commands in PIPELINE, connecting each to one another.
This macro calls itself recursively, with NOTFIRST non-nil."
(when (setq pipeline (cadr pipeline))
- `(eshell-copy-handles
+ `(eshell-with-copied-handles
(progn
,(when (cdr pipeline)
`(let ((nextproc
@@ -880,11 +879,8 @@ eshell-execute-pipeline
(progn
,(if (fboundp 'make-process)
`(eshell-do-pipelines ,pipeline)
- `(let ((tail-handles (eshell-create-handles
- (car (aref eshell-current-handles
- ,eshell-output-handle)) nil
- (car (aref eshell-current-handles
- ,eshell-error-handle)) nil)))
+ `(let ((tail-handles (eshell-duplicate-handles
+ eshell-current-handles)))
(eshell-do-pipelines-synchronously ,pipeline)))
(eshell-process-identity (cons (symbol-value headproc)
(symbol-value tailproc))))))
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 4620565f857..58084db28a8 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -291,25 +291,42 @@ eshell--apply-redirections
(defun eshell-create-handles
(stdout output-mode &optional stderr error-mode)
"Create a new set of file handles for a command.
-The default location for standard output and standard error will go to
-STDOUT and STDERR, respectively.
-OUTPUT-MODE and ERROR-MODE are either `overwrite', `append' or `insert';
-a nil value of mode defaults to `insert'."
+The default target for standard output and standard error will
+go to STDOUT and STDERR, respectively. OUTPUT-MODE and
+ERROR-MODE are either `overwrite', `append' or `insert'; a nil
+value of mode defaults to `insert'.
+
+The result is a vector of file handles. Each handle is of the form:
+
+ (TARGETS DEFAULT REF-COUNT)
+
+TARGETS is a list of destinations for output. DEFAULT is non-nil
+if handle has its initial default value (always t after calling
+this function). REF-COUNT is the number of references to this
+handle (initially 1); see `eshell-protect-handles' and
+`eshell-close-handles'."
(let* ((handles (make-vector eshell-number-of-handles nil))
- (output-target (eshell-get-target stdout output-mode))
+ (output-target (eshell-get-targets stdout output-mode))
(error-target (if stderr
- (eshell-get-target stderr error-mode)
+ (eshell-get-targets stderr error-mode)
output-target)))
- (aset handles eshell-output-handle (cons output-target 1))
- (aset handles eshell-error-handle (cons error-target 1))
+ (aset handles eshell-output-handle (list output-target t 1))
+ (aset handles eshell-error-handle (list error-target t 1))
handles))
+(defun eshell-duplicate-handles (handles)
+ "Create a duplicate of the file handles in HANDLES.
+This will copy the targets of each handle in HANDLES, setting the
+DEFAULT field to t (see `eshell-create-handles')."
+ (eshell-create-handles
+ (car (aref handles eshell-output-handle)) nil
+ (car (aref handles eshell-error-handle)) nil))
+
(defun eshell-protect-handles (handles)
"Protect the handles in HANDLES from a being closed."
(dotimes (idx eshell-number-of-handles)
- (when (aref handles idx)
- (setcdr (aref handles idx)
- (1+ (cdr (aref handles idx))))))
+ (when-let ((handle (aref handles idx)))
+ (setcar (nthcdr 2 handle) (1+ (nth 2 handle)))))
handles)
(defun eshell-close-handles (&optional exit-code result handles)
@@ -330,8 +347,8 @@ eshell-close-handles
(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)
+ (setcar (nthcdr 2 handle) (1- (nth 2 handle)))
+ (when (= (nth 2 handle) 0)
(dolist (target (ensure-list (car (aref handles idx))))
(eshell-close-target target (= eshell-last-command-status 0)))
(setcar handle nil))))))
@@ -344,15 +361,17 @@ eshell-set-output-handle
(if (and (stringp target)
(string= target (null-device)))
(aset handles index nil)
- (let ((where (eshell-get-target target mode))
- (current (car (aref handles index))))
- (if (listp current)
+ (let* ((where (eshell-get-target target mode))
+ (handle (or (aref handles index)
+ (aset handles index (list nil nil 1))))
+ (current (car handle))
+ (defaultp (cadr handle)))
+ (if (not defaultp)
(unless (member where current)
(setq current (append current (list where))))
(setq current (list where)))
- (if (not (aref handles index))
- (aset handles index (cons nil 1)))
- (setcar (aref handles index) current))))))
+ (setcar handle current)
+ (setcar (cdr handle) nil))))))
(defun eshell-copy-output-handle (index index-to-copy &optional handles)
"Copy the handle INDEX-TO-COPY to INDEX for the current HANDLES.
@@ -482,6 +501,13 @@ eshell-get-target
(error "Invalid redirection target: %s"
(eshell-stringify target)))))
+(defun eshell-get-targets (targets &optional mode)
+ "Convert TARGETS into valid output targets.
+TARGETS can be a single raw target or a list thereof. MODE is either
+`overwrite', `append' or `insert'; if it is omitted or nil, it
+defaults to `insert'."
+ (mapcar (lambda (i) (eshell-get-target i mode)) (ensure-list targets)))
+
(defun eshell-interactive-output-p (&optional index handles)
"Return non-nil if the specified handle is bound for interactive display.
HANDLES is the set of handles to check; if nil, use
@@ -493,9 +519,9 @@ eshell-interactive-output-p
(let ((handles (or handles eshell-current-handles))
(index (or index eshell-output-handle)))
(if (eq index 'all)
- (and (eq (car (aref handles eshell-output-handle)) t)
- (eq (car (aref handles eshell-error-handle)) t))
- (eq (car (aref handles index)) t))))
+ (and (equal (car (aref handles eshell-output-handle)) '(t))
+ (equal (car (aref handles eshell-error-handle)) '(t)))
+ (equal (car (aref handles index)) '(t)))))
(defvar eshell-print-queue nil)
(defvar eshell-print-queue-count -1)
@@ -602,15 +628,10 @@ eshell-output-object
If HANDLE-INDEX is nil, output to `eshell-output-handle'.
HANDLES is the set of file handles to use; if nil, use
`eshell-current-handles'."
- (let ((target (car (aref (or handles eshell-current-handles)
- (or handle-index eshell-output-handle)))))
- (if (listp target)
- (while target
- (eshell-output-object-to-target object (car target))
- (setq target (cdr target)))
- (eshell-output-object-to-target object target)
- ;; Explicitly return nil to match the list case above.
- nil)))
+ (let ((targets (car (aref (or handles eshell-current-handles)
+ (or handle-index eshell-output-handle)))))
+ (dolist (target targets)
+ (eshell-output-object-to-target object target))))
(provide 'esh-io)
;;; esh-io.el ends here
--
2.25.1
[-- Attachment #3: 0002-Fix-handling-of-output-handles-in-nested-Eshell-form.patch --]
[-- Type: text/plain, Size: 12878 bytes --]
From 43e9664a7a0c528a11e86dd6f2d07b3afe233f54 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 20 Dec 2022 09:39:07 -0800
Subject: [PATCH 2/3] Fix handling of output handles in nested Eshell forms
Previously, the output handles in nested forms would be reset to the
default, leading to wrong behavior for commands like
{echo a; echo b} > file
"b" would be written to "file" as expected, but "a" would go to
standard output (bug#59545).
* lisp/eshell/esh-cmd.el (eshell-parse-command): Use
'eshell-with-copied-handles' for each statement within the whole
Eshell command.
* test/lisp/eshell/esh-io-tests.el (esh-io-test/redirect-subcommands)
(esh-io-test/redirect-subcommands/override)
(esh-io-test/redirect-subcommands/interpolated): New tests.
* test/lisp/eshell/em-script-tests.el
(em-script-test/source-script/redirect)
(em-script-test/source-script/redirect/dev-null): New tests.
(em-script-test/source-script, em-script-test/source-script/arg-vars)
(em-script-test/source-script/all-args-var): Tweak names/docstrings.
* test/lisp/eshell/em-extpipe-tests.el (em-extpipe-tests--deftest):
Skip over the newly-added 'eshell-with-copied-handles' form when
checking the parse results.
* test/lisp/eshell/em-tramp-tests.el (em-tramp-test/su-default)
(em-tramp-test/su-user, em-tramp-test/su-login)
(em-tramp-test/sudo-shell, em-tramp-test/sudo-user-shell)
(em-tramp-test/doas-shell, em-tramp-test/doas-user-shell): Update
expected command forms.
---
doc/misc/eshell.texi | 5 --
lisp/eshell/esh-cmd.el | 8 ++-
test/lisp/eshell/em-extpipe-tests.el | 2 +-
test/lisp/eshell/em-script-tests.el | 32 ++++++++++--
test/lisp/eshell/em-tramp-tests.el | 75 +++++++++++++++-------------
test/lisp/eshell/esh-io-tests.el | 28 +++++++++++
6 files changed, 103 insertions(+), 47 deletions(-)
diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index f9796d69a9a..118ee80acb9 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -2162,11 +2162,6 @@ Bugs and ideas
@item Allow all Eshell buffers to share the same history and list-dir
-@item There is a problem with script commands that output to @file{/dev/null}
-
-If a script file, somewhere in the middle, uses @samp{> /dev/null},
-output from all subsequent commands is swallowed.
-
@item Split up parsing of text after @samp{$} in @file{esh-var.el}
Make it similar to the way that @file{esh-arg.el} is structured.
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 03388236b06..79957aeb416 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -418,8 +418,12 @@ eshell-parse-command
(eshell-separate-commands terms "[&;]" nil 'eshell--sep-terms))))
(let ((cmd commands))
(while cmd
- (if (cdr cmd)
- (setcar cmd `(eshell-commands ,(car cmd))))
+ ;; Copy I/O handles so each full statement can manipulate them
+ ;; if they like. As a small optimization, skip this for the
+ ;; last top-level one; we won't use these handles again
+ ;; anyway.
+ (when (or (not toplevel) (cdr cmd))
+ (setcar cmd `(eshell-with-copied-handles ,(car cmd))))
(setq cmd (cdr cmd))))
(if toplevel
`(eshell-commands (progn
diff --git a/test/lisp/eshell/em-extpipe-tests.el b/test/lisp/eshell/em-extpipe-tests.el
index 04e78279427..a2646a0296b 100644
--- a/test/lisp/eshell/em-extpipe-tests.el
+++ b/test/lisp/eshell/em-extpipe-tests.el
@@ -42,7 +42,7 @@ em-extpipe-tests--deftest
(shell-command-switch "-c"))
;; Strip `eshell-trap-errors'.
(should (equal ,expected
- (cadr (eshell-parse-command input))))))
+ (cadadr (eshell-parse-command input))))))
(with-substitute-for-temp (&rest body)
;; Substitute name of an actual temporary file and/or
;; buffer into `input'. The substitution logic is
diff --git a/test/lisp/eshell/em-script-tests.el b/test/lisp/eshell/em-script-tests.el
index b837d464ccd..f720f697c67 100644
--- a/test/lisp/eshell/em-script-tests.el
+++ b/test/lisp/eshell/em-script-tests.el
@@ -35,21 +35,43 @@
;;; Tests:
(ert-deftest em-script-test/source-script ()
- "Test sourcing script with no argumentss"
+ "Test sourcing a simple script."
(ert-with-temp-file temp-file :text "echo hi"
(with-temp-eshell
(eshell-match-command-output (format "source %s" temp-file)
"hi\n"))))
-(ert-deftest em-script-test/source-script-arg-vars ()
- "Test sourcing script with $0, $1, ... variables"
+(ert-deftest em-script-test/source-script/redirect ()
+ "Test sourcing a script and redirecting its output."
+ (ert-with-temp-file temp-file
+ :text "echo hi\necho bye"
+ (eshell-with-temp-buffer bufname "old"
+ (with-temp-eshell
+ (eshell-match-command-output
+ (format "source %s > #<%s>" temp-file bufname)
+ "\\`\\'"))
+ (should (equal (buffer-string) "hibye")))))
+
+(ert-deftest em-script-test/source-script/redirect/dev-null ()
+ "Test sourcing a script and redirecting its output, including to /dev/null."
+ (ert-with-temp-file temp-file
+ :text "echo hi\necho bad > /dev/null\necho bye"
+ (eshell-with-temp-buffer bufname "old"
+ (with-temp-eshell
+ (eshell-match-command-output
+ (format "source %s > #<%s>" temp-file bufname)
+ "\\`\\'"))
+ (should (equal (buffer-string) "hibye")))))
+
+(ert-deftest em-script-test/source-script/arg-vars ()
+ "Test sourcing script with $0, $1, ... variables."
(ert-with-temp-file temp-file :text "printnl $0 \"$1 $2\""
(with-temp-eshell
(eshell-match-command-output (format "source %s one two" temp-file)
(format "%s\none two\n" temp-file)))))
-(ert-deftest em-script-test/source-script-all-args-var ()
- "Test sourcing script with the $* variable"
+(ert-deftest em-script-test/source-script/all-args-var ()
+ "Test sourcing script with the $* variable."
(ert-with-temp-file temp-file :text "printnl $*"
(with-temp-eshell
(eshell-match-command-output (format "source %s" temp-file)
diff --git a/test/lisp/eshell/em-tramp-tests.el b/test/lisp/eshell/em-tramp-tests.el
index 6cc35ecdb1b..982a1eba279 100644
--- a/test/lisp/eshell/em-tramp-tests.el
+++ b/test/lisp/eshell/em-tramp-tests.el
@@ -27,21 +27,23 @@ em-tramp-test/su-default
"Test Eshell `su' command with no arguments."
(should (equal
(catch 'eshell-replace-command (eshell/su))
- `(eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:root@%s:%s"
- tramp-default-host default-directory)))))))
+ `(eshell-with-copied-handles
+ (eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/su:root@%s:%s"
+ tramp-default-host default-directory))))))))
(ert-deftest em-tramp-test/su-user ()
"Test Eshell `su' command with USER argument."
(should (equal
(catch 'eshell-replace-command (eshell/su "USER"))
- `(eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:USER@%s:%s"
- tramp-default-host default-directory)))))))
+ `(eshell-with-copied-handles
+ (eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/su:USER@%s:%s"
+ tramp-default-host default-directory))))))))
(ert-deftest em-tramp-test/su-login ()
"Test Eshell `su' command with -/-l/--login option."
@@ -50,10 +52,11 @@ em-tramp-test/su-login
("-")))
(should (equal
(catch 'eshell-replace-command (apply #'eshell/su args))
- `(eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:root@%s:~/" tramp-default-host))))))))
+ `(eshell-with-copied-handles
+ (eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/su:root@%s:~/" tramp-default-host)))))))))
(defun mock-eshell-named-command (&rest args)
"Dummy function to test Eshell `sudo' command rewriting."
@@ -91,21 +94,23 @@ em-tramp-test/sudo-shell
("-s")))
(should (equal
(catch 'eshell-replace-command (apply #'eshell/sudo args))
- `(eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/sudo:root@%s:%s"
- tramp-default-host default-directory))))))))
+ `(eshell-with-copied-handles
+ (eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/sudo:root@%s:%s"
+ tramp-default-host default-directory)))))))))
(ert-deftest em-tramp-test/sudo-user-shell ()
"Test Eshell `sudo' command with -s and -u options."
(should (equal
(catch 'eshell-replace-command (eshell/sudo "-u" "USER" "-s"))
- `(eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/sudo:USER@%s:%s"
- tramp-default-host default-directory)))))))
+ `(eshell-with-copied-handles
+ (eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/sudo:USER@%s:%s"
+ tramp-default-host default-directory))))))))
(ert-deftest em-tramp-test/doas-basic ()
"Test Eshell `doas' command with default user."
@@ -144,20 +149,22 @@ em-tramp-test/doas-shell
("-s")))
(should (equal
(catch 'eshell-replace-command (apply #'eshell/doas args))
- `(eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/doas:root@%s:%s"
- tramp-default-host default-directory))))))))
+ `(eshell-with-copied-handles
+ (eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/doas:root@%s:%s"
+ tramp-default-host default-directory)))))))))
(ert-deftest em-tramp-test/doas-user-shell ()
"Test Eshell `doas' command with -s and -u options."
(should (equal
(catch 'eshell-replace-command (eshell/doas "-u" "USER" "-s"))
- `(eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/doas:USER@%s:%s"
- tramp-default-host default-directory)))))))
+ `(eshell-with-copied-handles
+ (eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/doas:USER@%s:%s"
+ tramp-default-host default-directory))))))))
;;; em-tramp-tests.el ends here
diff --git a/test/lisp/eshell/esh-io-tests.el b/test/lisp/eshell/esh-io-tests.el
index 37b234eaf06..ccf8ac1b9a1 100644
--- a/test/lisp/eshell/esh-io-tests.el
+++ b/test/lisp/eshell/esh-io-tests.el
@@ -146,6 +146,34 @@ esh-io-test/redirect-multiple/repeat
(should (equal (buffer-string) "new"))
(should (equal eshell-test-value "new")))))
+(ert-deftest esh-io-test/redirect-subcommands ()
+ "Check that redirecting subcommands applies to all subcommands."
+ (eshell-with-temp-buffer bufname "old"
+ (with-temp-eshell
+ (eshell-insert-command (format "{echo foo; echo bar} > #<%s>" bufname)))
+ (should (equal (buffer-string) "foobar"))))
+
+(ert-deftest esh-io-test/redirect-subcommands/override ()
+ "Check that redirecting subcommands applies to all subcommands.
+Include a redirect to another location in the subcommand to
+ensure only its statement is redirected."
+ (eshell-with-temp-buffer bufname "old"
+ (eshell-with-temp-buffer bufname-2 "also old"
+ (with-temp-eshell
+ (eshell-insert-command
+ (format "{echo foo; echo bar > #<%s>; echo baz} > #<%s>"
+ bufname-2 bufname)))
+ (should (equal (buffer-string) "bar")))
+ (should (equal (buffer-string) "foobaz"))))
+
+(ert-deftest esh-io-test/redirect-subcommands/interpolated ()
+ "Check that redirecting interpolated subcommands applies to all subcommands."
+ (eshell-with-temp-buffer bufname "old"
+ (with-temp-eshell
+ (eshell-insert-command
+ (format "echo ${echo foo; echo bar} > #<%s>" bufname)))
+ (should (equal (buffer-string) "foobar"))))
+
\f
;; Redirecting specific handles
--
2.25.1
[-- Attachment #4: 0003-Simplify-handling-of-dev-null-redirection-in-Eshell.patch --]
[-- Type: text/plain, Size: 6302 bytes --]
From f591a65476a6283de8614fa71fe4ad3375b998a5 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 20 Dec 2022 13:47:20 -0800
Subject: [PATCH 3/3] Simplify handling of /dev/null redirection in Eshell
This also fixes an issue where "echo hi > foo > /dev/null" didn't
write to the file "foo".
* lisp/eshell/esh-io.el (eshell-virtual-targets): Add "/dev/null".
(eshell-set-output-handle): Handle 'eshell-null-device'.
* test/lisp/eshell/esh-io-tests.el
(esh-io-test/redirect-subcommands/dev-null)
(esh-io-test/virtual/dev-null, esh-io-test/virtual/dev-null/multiple):
New tests.
---
lisp/eshell/esh-io.el | 51 +++++++++++++++-----------------
test/lisp/eshell/esh-io-tests.el | 33 +++++++++++++++++++--
2 files changed, 55 insertions(+), 29 deletions(-)
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 58084db28a8..dc433de09b0 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -116,16 +116,20 @@ eshell-print-queue-size
:group 'eshell-io)
(defcustom eshell-virtual-targets
- '(("/dev/eshell" eshell-interactive-print nil)
+ '(;; This should be the literal string "/dev/null", not `null-device'.
+ ("/dev/null" (lambda (mode) (throw 'eshell-null-device t)) t)
+ ("/dev/eshell" eshell-interactive-print nil)
("/dev/kill" (lambda (mode)
- (if (eq mode 'overwrite)
- (kill-new ""))
- 'eshell-kill-append) t)
+ (when (eq mode 'overwrite)
+ (kill-new ""))
+ #'eshell-kill-append)
+ t)
("/dev/clip" (lambda (mode)
- (if (eq mode 'overwrite)
- (let ((select-enable-clipboard t))
- (kill-new "")))
- 'eshell-clipboard-append) t))
+ (when (eq mode 'overwrite)
+ (let ((select-enable-clipboard t))
+ (kill-new "")))
+ #'eshell-clipboard-append)
+ t))
"Map virtual devices name to Emacs Lisp functions.
If the user specifies any of the filenames above as a redirection
target, the function in the second element will be called.
@@ -138,10 +142,7 @@ eshell-virtual-targets
The output function is then called repeatedly with single strings,
which represents successive pieces of the output of the command, until nil
-is passed, meaning EOF.
-
-NOTE: /dev/null is handled specially as a virtual target, and should
-not be added to this variable."
+is passed, meaning EOF."
:type '(repeat
(list (string :tag "Target")
function
@@ -357,21 +358,17 @@ eshell-set-output-handle
"Set handle INDEX for the current HANDLES to point to TARGET using MODE.
If HANDLES is nil, use `eshell-current-handles'."
(when target
- (let ((handles (or handles eshell-current-handles)))
- (if (and (stringp target)
- (string= target (null-device)))
- (aset handles index nil)
- (let* ((where (eshell-get-target target mode))
- (handle (or (aref handles index)
- (aset handles index (list nil nil 1))))
- (current (car handle))
- (defaultp (cadr handle)))
- (if (not defaultp)
- (unless (member where current)
- (setq current (append current (list where))))
- (setq current (list where)))
- (setcar handle current)
- (setcar (cdr handle) nil))))))
+ (let* ((handles (or handles eshell-current-handles))
+ (handle (or (aref handles index)
+ (aset handles index (list nil nil 1))))
+ (defaultp (cadr handle))
+ (current (unless defaultp (car handle))))
+ (catch 'eshell-null-device
+ (let ((where (eshell-get-target target mode)))
+ (unless (member where current)
+ (setq current (append current (list where))))))
+ (setcar handle current)
+ (setcar (cdr handle) nil))))
(defun eshell-copy-output-handle (index index-to-copy &optional handles)
"Copy the handle INDEX-TO-COPY to INDEX for the current HANDLES.
diff --git a/test/lisp/eshell/esh-io-tests.el b/test/lisp/eshell/esh-io-tests.el
index ccf8ac1b9a1..9a3c14f365f 100644
--- a/test/lisp/eshell/esh-io-tests.el
+++ b/test/lisp/eshell/esh-io-tests.el
@@ -166,6 +166,17 @@ esh-io-test/redirect-subcommands/override
(should (equal (buffer-string) "bar")))
(should (equal (buffer-string) "foobaz"))))
+(ert-deftest esh-io-test/redirect-subcommands/dev-null ()
+ "Check that redirecting subcommands applies to all subcommands.
+Include a redirect to /dev/null to ensure it only applies to its
+statement."
+ (eshell-with-temp-buffer bufname "old"
+ (with-temp-eshell
+ (eshell-insert-command
+ (format "{echo foo; echo bar > /dev/null; echo baz} > #<%s>"
+ bufname)))
+ (should (equal (buffer-string) "foobaz"))))
+
(ert-deftest esh-io-test/redirect-subcommands/interpolated ()
"Check that redirecting interpolated subcommands applies to all subcommands."
(eshell-with-temp-buffer bufname "old"
@@ -302,12 +313,30 @@ esh-io-test/redirect-pipe
\f
;; Virtual targets
-(ert-deftest esh-io-test/virtual-dev-eshell ()
+(ert-deftest esh-io-test/virtual/dev-null ()
+ "Check that redirecting to /dev/null works."
+ (with-temp-eshell
+ (eshell-match-command-output "echo hi > /dev/null" "\\`\\'")))
+
+(ert-deftest esh-io-test/virtual/dev-null/multiple ()
+ "Check that redirecting to /dev/null works alongside other redirections."
+ (eshell-with-temp-buffer bufname "old"
+ (with-temp-eshell
+ (eshell-match-command-output
+ (format "echo new > /dev/null > #<%s>" bufname) "\\`\\'"))
+ (should (equal (buffer-string) "new")))
+ (eshell-with-temp-buffer bufname "old"
+ (with-temp-eshell
+ (eshell-match-command-output
+ (format "echo new > #<%s> > /dev/null" bufname) "\\`\\'"))
+ (should (equal (buffer-string) "new"))))
+
+(ert-deftest esh-io-test/virtual/dev-eshell ()
"Check that redirecting to /dev/eshell works."
(with-temp-eshell
(eshell-match-command-output "echo hi > /dev/eshell" "hi")))
-(ert-deftest esh-io-test/virtual-dev-kill ()
+(ert-deftest esh-io-test/virtual/dev-kill ()
"Check that redirecting to /dev/kill works."
(with-temp-eshell
(eshell-insert-command "echo one > /dev/kill")
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
2022-12-21 0:18 ` Jim Porter
@ 2022-12-21 0:29 ` Jim Porter
2022-12-21 9:54 ` Michael Albinus
2022-12-21 12:01 ` Eli Zaretskii
0 siblings, 2 replies; 14+ messages in thread
From: Jim Porter @ 2022-12-21 0:29 UTC (permalink / raw)
To: Milan Zimmermann, 59545; +Cc: eliz
[-- Attachment #1: Type: text/plain, Size: 538 bytes --]
On 12/20/2022 4:18 PM, Jim Porter wrote:
> Attached is a patch series to fix this, with a bunch of new tests. I
> also fixed a related issue where redirecting to /dev/null could clobber
> your other redirects. (There's *also* an issue that should be fixed for
> the release branch; I'll send that in a separate message.)
Eli, this is the patch for the release branch (it corresponds to part
0003 of the patch series for master). Is this ok to merge? It's a
regression that was introduced in Emacs 28.1, and the fix is pretty simple.
[-- Attachment #2: 0001-When-redirecting-to-the-null-device-in-Eshell-use-de.patch --]
[-- Type: text/plain, Size: 1263 bytes --]
From b04f42cca272b9a0f3b5e3167ce956523b161a7e Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 20 Dec 2022 16:20:50 -0800
Subject: [PATCH] When redirecting to the null device in Eshell, use
"/dev/null"
This is so that users can type "cmd ... > /dev/null" in Eshell no
matter what their system's null device is called. This partially
reverts 67a8bdb90c9b5865b7f17290c7135b1a5458c36d.
Do not merge to master.
* lisp/eshell/esh-io.el (eshell-set-output-handle): Use "/dev/null"
literally.
---
lisp/eshell/esh-io.el | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 4620565f857..6df40914060 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -342,7 +342,9 @@ eshell-set-output-handle
(when target
(let ((handles (or handles eshell-current-handles)))
(if (and (stringp target)
- (string= target (null-device)))
+ ;; This should be the literal string "/dev/null", not
+ ;; `null-device'.
+ (string= target "/dev/null"))
(aset handles index nil)
(let ((where (eshell-get-target target mode))
(current (car (aref handles index))))
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
2022-12-21 0:29 ` Jim Porter
@ 2022-12-21 9:54 ` Michael Albinus
2022-12-21 18:48 ` Jim Porter
2022-12-21 12:01 ` Eli Zaretskii
1 sibling, 1 reply; 14+ messages in thread
From: Michael Albinus @ 2022-12-21 9:54 UTC (permalink / raw)
To: Jim Porter; +Cc: 59545, eliz, Milan Zimmermann
Jim Porter <jporterbugs@gmail.com> writes:
Hi Jim,
> Eli, this is the patch for the release branch (it corresponds to part
> 0003 of the patch series for master). Is this ok to merge? It's a
> regression that was introduced in Emacs 28.1, and the fix is pretty
> simple.
>
> From b04f42cca272b9a0f3b5e3167ce956523b161a7e Mon Sep 17 00:00:00 2001
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Tue, 20 Dec 2022 16:20:50 -0800
> Subject: [PATCH] When redirecting to the null device in Eshell, use
> "/dev/null"
>
> This is so that users can type "cmd ... > /dev/null" in Eshell no
> matter what their system's null device is called. This partially
> reverts 67a8bdb90c9b5865b7f17290c7135b1a5458c36d.
And when they want to use another value? (null-device) returns
"/dev/null" for local default-directory's if you're not on MS
Windows. On MS Windows, it returns "NUL".
With a remote default-directory, the value is configurable (as
connection-local variable). Per default it is also "/dev/null", but it
could be changed.
Do you want to suppress this mechanism in Eshell? Why? I guess it is
more appropriate to install a handler for the actual value of
(null-device), instead just a handler for "/dev/null" only. And if you
want to make "/dev/null" a system-independant default, add the same
handler for this in parallel. Then both would be equivalent on MS Windows:
--8<---------------cut here---------------start------------->8---
cmd ... > /dev/null
cmd ... > NUL
--8<---------------cut here---------------end--------------->8---
Best regards, Michael.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
2022-12-21 0:29 ` Jim Porter
2022-12-21 9:54 ` Michael Albinus
@ 2022-12-21 12:01 ` Eli Zaretskii
1 sibling, 0 replies; 14+ messages in thread
From: Eli Zaretskii @ 2022-12-21 12:01 UTC (permalink / raw)
To: Jim Porter; +Cc: 59545, milan.zimmermann
> Date: Tue, 20 Dec 2022 16:29:31 -0800
> From: Jim Porter <jporterbugs@gmail.com>
> Cc: eliz@gnu.org
>
> On 12/20/2022 4:18 PM, Jim Porter wrote:
> > Attached is a patch series to fix this, with a bunch of new tests. I
> > also fixed a related issue where redirecting to /dev/null could clobber
> > your other redirects. (There's *also* an issue that should be fixed for
> > the release branch; I'll send that in a separate message.)
>
> Eli, this is the patch for the release branch (it corresponds to part
> 0003 of the patch series for master). Is this ok to merge? It's a
> regression that was introduced in Emacs 28.1, and the fix is pretty simple.
Yes, OK to install this on the release branch.
Thanks.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
2022-12-21 9:54 ` Michael Albinus
@ 2022-12-21 18:48 ` Jim Porter
2022-12-21 19:26 ` Eli Zaretskii
0 siblings, 1 reply; 14+ messages in thread
From: Jim Porter @ 2022-12-21 18:48 UTC (permalink / raw)
To: Michael Albinus; +Cc: 59545, eliz, Milan Zimmermann
[-- Attachment #1: Type: text/plain, Size: 1362 bytes --]
On 12/21/2022 1:54 AM, Michael Albinus wrote:
> And when they want to use another value? (null-device) returns
> "/dev/null" for local default-directory's if you're not on MS
> Windows. On MS Windows, it returns "NUL".
Right. The intent is for the virtual name for the null device to be the
same in Eshell no matter what the system really calls it. On non-MS
systems, this shouldn't actually be necessary, since you could just
write to the *real* /dev/null. The virtual target in Eshell is just so
that /dev/null also works on MS Windows/DOS.
However, I would have thought that you could write to NUL on MS Windows
without any special handling. The Emacs manual has this to say:
"[On MS Windows,] referencing any file whose name matches a DOS
character device, such as NUL or LPT1 or PRN or CON, with or without any
file-name extension, will always resolve to those character devices, in
any directory. Therefore, only use such file names when you want to use
the corresponding character device."
I'd expect that to mean that if you opened a buffer and tried to save it
as "NUL", it would just work, but instead I get:
Write error: Bad file descriptor, c:/NUL
With that in mind, here are two patches (one for 29 and one for master)
to let Eshell handle both "/dev/null" and (on MS systems) "NUL". That
way, users get the best of both worlds.
[-- Attachment #2: 29--0001-When-redirecting-to-the-null-device-in-Eshell-allow-.patch --]
[-- Type: text/plain, Size: 1381 bytes --]
From 3664f2bdfb64ae7d68b6be12e4ed6b027bdc2951 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 20 Dec 2022 16:20:50 -0800
Subject: [PATCH] When redirecting to the null device in Eshell, allow
"/dev/null"
This is so that users can type "cmd ... > /dev/null" in Eshell no
matter what their system's null device is called. This fixes a
regression from 67a8bdb90c9b5865b7f17290c7135b1a5458c36d.
Do not merge to master.
* lisp/eshell/esh-io.el (eshell-set-output-handle): Handle both
'null-device' and the literal "/dev/null".
---
lisp/eshell/esh-io.el | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 4620565f857..b428f068472 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -342,7 +342,10 @@ eshell-set-output-handle
(when target
(let ((handles (or handles eshell-current-handles)))
(if (and (stringp target)
- (string= target (null-device)))
+ ;; Always treat "/dev/null" as the null device for
+ ;; compatibility, even if the system calls it something
+ ;; else.
+ (member target (list "/dev/null" (null-device))))
(aset handles index nil)
(let ((where (eshell-get-target target mode))
(current (car (aref handles index))))
--
2.25.1
[-- Attachment #3: master--0003-Simplify-handling-of-dev-null-redirection-in-Eshell.patch --]
[-- Type: text/plain, Size: 6804 bytes --]
From afe81dc609802c9a3d3d64037d9d9df9a100cd0f Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 20 Dec 2022 13:47:20 -0800
Subject: [PATCH 3/3] Simplify handling of /dev/null redirection in Eshell
This also fixes an issue where "echo hi > foo > /dev/null" didn't
write to the file "foo".
* lisp/eshell/esh-io.el (eshell-virtual-targets): Add "/dev/null".
(eshell-set-output-handle): Handle 'eshell-null-device'.
(eshell-get-target): Map 'null-device' to "/dev/null".
* test/lisp/eshell/esh-io-tests.el
(esh-io-test/redirect-subcommands/dev-null)
(esh-io-test/virtual/dev-null, esh-io-test/virtual/dev-null/multiple):
New tests.
---
lisp/eshell/esh-io.el | 56 +++++++++++++++++---------------
test/lisp/eshell/esh-io-tests.el | 33 +++++++++++++++++--
2 files changed, 60 insertions(+), 29 deletions(-)
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index 58084db28a8..a1dfef07458 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -116,16 +116,20 @@ eshell-print-queue-size
:group 'eshell-io)
(defcustom eshell-virtual-targets
- '(("/dev/eshell" eshell-interactive-print nil)
+ '(;; This should be the literal string "/dev/null", not `null-device'.
+ ("/dev/null" (lambda (mode) (throw 'eshell-null-device t)) t)
+ ("/dev/eshell" eshell-interactive-print nil)
("/dev/kill" (lambda (mode)
- (if (eq mode 'overwrite)
- (kill-new ""))
- 'eshell-kill-append) t)
+ (when (eq mode 'overwrite)
+ (kill-new ""))
+ #'eshell-kill-append)
+ t)
("/dev/clip" (lambda (mode)
- (if (eq mode 'overwrite)
- (let ((select-enable-clipboard t))
- (kill-new "")))
- 'eshell-clipboard-append) t))
+ (when (eq mode 'overwrite)
+ (let ((select-enable-clipboard t))
+ (kill-new "")))
+ #'eshell-clipboard-append)
+ t))
"Map virtual devices name to Emacs Lisp functions.
If the user specifies any of the filenames above as a redirection
target, the function in the second element will be called.
@@ -138,10 +142,7 @@ eshell-virtual-targets
The output function is then called repeatedly with single strings,
which represents successive pieces of the output of the command, until nil
-is passed, meaning EOF.
-
-NOTE: /dev/null is handled specially as a virtual target, and should
-not be added to this variable."
+is passed, meaning EOF."
:type '(repeat
(list (string :tag "Target")
function
@@ -357,21 +358,17 @@ eshell-set-output-handle
"Set handle INDEX for the current HANDLES to point to TARGET using MODE.
If HANDLES is nil, use `eshell-current-handles'."
(when target
- (let ((handles (or handles eshell-current-handles)))
- (if (and (stringp target)
- (string= target (null-device)))
- (aset handles index nil)
- (let* ((where (eshell-get-target target mode))
- (handle (or (aref handles index)
- (aset handles index (list nil nil 1))))
- (current (car handle))
- (defaultp (cadr handle)))
- (if (not defaultp)
- (unless (member where current)
- (setq current (append current (list where))))
- (setq current (list where)))
- (setcar handle current)
- (setcar (cdr handle) nil))))))
+ (let* ((handles (or handles eshell-current-handles))
+ (handle (or (aref handles index)
+ (aset handles index (list nil nil 1))))
+ (defaultp (cadr handle))
+ (current (unless defaultp (car handle))))
+ (catch 'eshell-null-device
+ (let ((where (eshell-get-target target mode)))
+ (unless (member where current)
+ (setq current (append current (list where))))))
+ (setcar handle current)
+ (setcar (cdr handle) nil))))
(defun eshell-copy-output-handle (index index-to-copy &optional handles)
"Copy the handle INDEX-TO-COPY to INDEX for the current HANDLES.
@@ -458,6 +455,11 @@ eshell-get-target
(setq mode (or mode 'insert))
(cond
((stringp target)
+ ;; Always treat the `null-device' as the virtual target
+ ;; "/dev/null". This way, systems that call their null device
+ ;; something else can use either form.
+ (when (string= target (null-device))
+ (setq target "/dev/null"))
(let ((redir (assoc target eshell-virtual-targets)))
(if redir
(if (nth 2 redir)
diff --git a/test/lisp/eshell/esh-io-tests.el b/test/lisp/eshell/esh-io-tests.el
index ccf8ac1b9a1..9a3c14f365f 100644
--- a/test/lisp/eshell/esh-io-tests.el
+++ b/test/lisp/eshell/esh-io-tests.el
@@ -166,6 +166,17 @@ esh-io-test/redirect-subcommands/override
(should (equal (buffer-string) "bar")))
(should (equal (buffer-string) "foobaz"))))
+(ert-deftest esh-io-test/redirect-subcommands/dev-null ()
+ "Check that redirecting subcommands applies to all subcommands.
+Include a redirect to /dev/null to ensure it only applies to its
+statement."
+ (eshell-with-temp-buffer bufname "old"
+ (with-temp-eshell
+ (eshell-insert-command
+ (format "{echo foo; echo bar > /dev/null; echo baz} > #<%s>"
+ bufname)))
+ (should (equal (buffer-string) "foobaz"))))
+
(ert-deftest esh-io-test/redirect-subcommands/interpolated ()
"Check that redirecting interpolated subcommands applies to all subcommands."
(eshell-with-temp-buffer bufname "old"
@@ -302,12 +313,30 @@ esh-io-test/redirect-pipe
\f
;; Virtual targets
-(ert-deftest esh-io-test/virtual-dev-eshell ()
+(ert-deftest esh-io-test/virtual/dev-null ()
+ "Check that redirecting to /dev/null works."
+ (with-temp-eshell
+ (eshell-match-command-output "echo hi > /dev/null" "\\`\\'")))
+
+(ert-deftest esh-io-test/virtual/dev-null/multiple ()
+ "Check that redirecting to /dev/null works alongside other redirections."
+ (eshell-with-temp-buffer bufname "old"
+ (with-temp-eshell
+ (eshell-match-command-output
+ (format "echo new > /dev/null > #<%s>" bufname) "\\`\\'"))
+ (should (equal (buffer-string) "new")))
+ (eshell-with-temp-buffer bufname "old"
+ (with-temp-eshell
+ (eshell-match-command-output
+ (format "echo new > #<%s> > /dev/null" bufname) "\\`\\'"))
+ (should (equal (buffer-string) "new"))))
+
+(ert-deftest esh-io-test/virtual/dev-eshell ()
"Check that redirecting to /dev/eshell works."
(with-temp-eshell
(eshell-match-command-output "echo hi > /dev/eshell" "hi")))
-(ert-deftest esh-io-test/virtual-dev-kill ()
+(ert-deftest esh-io-test/virtual/dev-kill ()
"Check that redirecting to /dev/kill works."
(with-temp-eshell
(eshell-insert-command "echo one > /dev/kill")
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
2022-12-21 18:48 ` Jim Porter
@ 2022-12-21 19:26 ` Eli Zaretskii
2022-12-22 1:20 ` Jim Porter
0 siblings, 1 reply; 14+ messages in thread
From: Eli Zaretskii @ 2022-12-21 19:26 UTC (permalink / raw)
To: Jim Porter; +Cc: 59545, milan.zimmermann, michael.albinus
> Date: Wed, 21 Dec 2022 10:48:18 -0800
> Cc: 59545@debbugs.gnu.org, eliz@gnu.org,
> Milan Zimmermann <milan.zimmermann@gmail.com>
> From: Jim Porter <jporterbugs@gmail.com>
>
> However, I would have thought that you could write to NUL on MS Windows
> without any special handling. The Emacs manual has this to say:
>
> "[On MS Windows,] referencing any file whose name matches a DOS
> character device, such as NUL or LPT1 or PRN or CON, with or without any
> file-name extension, will always resolve to those character devices, in
> any directory. Therefore, only use such file names when you want to use
> the corresponding character device."
>
> I'd expect that to mean that if you opened a buffer and tried to save it
> as "NUL", it would just work, but instead I get:
>
> Write error: Bad file descriptor, c:/NUL
It's a bug, unrelated to the actual writing. I fixed it now on the
emacs-29 branch. You should be able now to save "NUL" on MS-Windows.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
2022-12-21 19:26 ` Eli Zaretskii
@ 2022-12-22 1:20 ` Jim Porter
2022-12-22 20:02 ` Jim Porter
0 siblings, 1 reply; 14+ messages in thread
From: Jim Porter @ 2022-12-22 1:20 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 59545, milan.zimmermann, michael.albinus
On 12/21/2022 11:26 AM, Eli Zaretskii wrote:
> It's a bug, unrelated to the actual writing. I fixed it now on the
> emacs-29 branch. You should be able now to save "NUL" on MS-Windows.
Ah, good that we caught that bug then. With that change, my original
patches should be sufficient. I've merged my patch for the emacs-29
branch as d6c8d5dbc9fc4786e91b76654058e904c96f0e11 (with some additional
improvements to the comment/commit message).
I'll wait to merge my changes to master for a couple days to give others
time to comment though, since those changes are more extensive.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
2022-12-22 1:20 ` Jim Porter
@ 2022-12-22 20:02 ` Jim Porter
2022-12-24 7:29 ` Jim Porter
0 siblings, 1 reply; 14+ messages in thread
From: Jim Porter @ 2022-12-22 20:02 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: michael.albinus, 59545-done, milan.zimmermann
On 12/21/2022 5:20 PM, Jim Porter wrote:
> I'll wait to merge my changes to master for a couple days to give others
> time to comment though, since those changes are more extensive.
Merged to master as 17bf6a829ca2fd2920c01e1aee30ab16b9c672eb. (I guess
that was only a day and not "a couple days", but I think it would be
good to get this on the master branch so that a wider audience can test
it to verify that everything is ok.)
Closing this now.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
2022-12-22 20:02 ` Jim Porter
@ 2022-12-24 7:29 ` Jim Porter
2022-12-25 1:36 ` Jim Porter
0 siblings, 1 reply; 14+ messages in thread
From: Jim Porter @ 2022-12-24 7:29 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 59545, milan.zimmermann, michael.albinus
reopen 59545
thanks
On 12/22/2022 12:02 PM, Jim Porter wrote:
> Merged to master as 17bf6a829ca2fd2920c01e1aee30ab16b9c672eb. (I guess
> that was only a day and not "a couple days", but I think it would be
> good to get this on the master branch so that a wider audience can test
> it to verify that everything is ok.)
I found a problem with the patch on master:
~ $ {echo hello; echo world} | rev
olleh ;; "dlrow" is missing!
This happens because the way I'm copying output handles around results
in EOF being sent to "rev" after "echo hello". To be fair, this didn't
work correctly before either, but the problem was different. Prior to my
patch:
~ $ {echo hello; echo world} | rev
hello
dlrow
(Only "echo world" is actually piped through "rev".) I'll try to come up
with a fix in the next couple days.
^ permalink raw reply [flat|nested] 14+ messages in thread
* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
2022-12-24 7:29 ` Jim Porter
@ 2022-12-25 1:36 ` Jim Porter
2022-12-25 21:49 ` Jim Porter
0 siblings, 1 reply; 14+ messages in thread
From: Jim Porter @ 2022-12-25 1:36 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 59545, milan.zimmermann, michael.albinus
[-- Attachment #1: Type: text/plain, Size: 817 bytes --]
On 12/23/2022 11:29 PM, Jim Porter wrote:
> I found a problem with the patch on master:
>
> ~ $ {echo hello; echo world} | rev
> olleh ;; "dlrow" is missing!
>
> This happens because the way I'm copying output handles around results
> in EOF being sent to "rev" after "echo hello".
Attached is a patch to fix this. I'm going to look into adding more test
cases if I can think of any before merging this.
I'll also see if I can fix the FIXME comment I added, but this is a part
of Eshell that's fairly brittle, and I think the *real* fix for that is
moving to running Eshell commands in a separate thread, as discussed on
emacs-devel. (I have a very WIP patch for this that already works
surprisingly well, but it's going to require a lot more work before it's
even worth making a feature branch.)
[-- Attachment #2: 0001-Fix-reference-counting-of-Eshell-I-O-handles.patch --]
[-- Type: text/plain, Size: 19366 bytes --]
From 7c2a087f7534d70893cd533c42a3a7c78682cb9a Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 24 Dec 2022 14:31:50 -0800
Subject: [PATCH] Fix reference-counting of Eshell I/O handles
This ensures that output targets in Eshell are only closed when Eshell
is actually done with them. In particular, this means that
"{ echo foo; echo bar } | rev" prints "raboof" as expected
(bug#59545).
* lisp/eshell/esh-io.el (eshell-create-handles): Structure the handles
differently so the targets and their ref-count can be shared.
(eshell-duplicate-handles): Reimplement this to share targets between
the original and new handle sets. Add STEAL-P argument.
(eshell-protect-handles, eshell-close-handles)
(eshell-set-output-handle, eshell-copy-output-handle)
(eshell-interactive-output-p, eshell-output-object): Account for
changes to the handle structure.
(eshell-get-targets): Remove. This only existed to make the previous
implementation of 'eshell-duplicate-handles' work.
* lisp/eshell/esh-cmd.el (eshell-with-copied-handles): New argument
STEAL-P.
(eshell-do-pipelines): Use STEAL-P for the last item in the pipeline.
(eshell-parse-command): Don't copy handles for the last command in the
list; explain why we can't use STEAL-P here.
* test/lisp/eshell/em-extpipe-tests.el (em-extpipe-tests--deftest)
* test/lisp/eshemm/em-tramp-tests.el (em-tramp-test/su-default)
(em-tramp-test/su-user, em-tramp-test/su-login)
(em-tramp-test/sudo-shell, em-tramp-test/sudo-user-shell)
(em-tramp-test/doas-shell, em-tramp-test/doas-user-shell): Account for
changes to the handle structure.
* test/lisp/eshell/esh-io-tests.el (esh-io-test/redirect-pipe): Split
into...
(esh-io-test/pipeline/default, esh-io-test/pipeline/all): ... these.
(esh-io-test/pipeline/subcommands): New test.
---
lisp/eshell/esh-cmd.el | 22 ++++--
lisp/eshell/esh-io.el | 106 +++++++++++++++------------
test/lisp/eshell/em-extpipe-tests.el | 2 +-
test/lisp/eshell/em-tramp-tests.el | 75 +++++++++----------
test/lisp/eshell/esh-io-tests.el | 23 ++++--
5 files changed, 127 insertions(+), 101 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 79957aeb416..4420657488b 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -419,10 +419,12 @@ eshell-parse-command
(let ((cmd commands))
(while cmd
;; Copy I/O handles so each full statement can manipulate them
- ;; if they like. As a small optimization, skip this for the
- ;; last top-level one; we won't use these handles again
- ;; anyway.
- (when (or (not toplevel) (cdr cmd))
+ ;; if they like. Skip this for the last command in the list
+ ;; though; we won't use these handles again anyway.
+ ;; FIXME: We could just call `eshell-with-copied-handles' with
+ ;; a non-nil STEAL-P argument here, except that this confuses
+ ;; Eshell's iterative evaluation when queuing input.
+ (when (cdr cmd)
(setcar cmd `(eshell-with-copied-handles ,(car cmd))))
(setq cmd (cdr cmd))))
(if toplevel
@@ -792,10 +794,12 @@ eshell-trap-errors
(defvar eshell-output-handle) ;Defined in esh-io.el.
(defvar eshell-error-handle) ;Defined in esh-io.el.
-(defmacro eshell-with-copied-handles (object)
- "Duplicate current I/O handles, so OBJECT works with its own copy."
+(defmacro eshell-with-copied-handles (object &optional steal-p)
+ "Duplicate current I/O handles, so OBJECT works with its own copy.
+If STEAL-P is non-nil, these new handles will be stolen from the
+current ones (see `eshell-duplicate-handles')."
`(let ((eshell-current-handles
- (eshell-duplicate-handles eshell-current-handles)))
+ (eshell-duplicate-handles eshell-current-handles ,steal-p)))
,object))
(define-obsolete-function-alias 'eshell-copy-handles
@@ -836,7 +840,9 @@ eshell-do-pipelines
(let ((proc ,(car pipeline)))
(set headproc (or proc (symbol-value headproc)))
(set tailproc (or (symbol-value tailproc) proc))
- proc))))))
+ proc)))
+ ;; Steal handles if this is the last item in the pipeline.
+ ,(null (cdr pipeline)))))
(defmacro eshell-do-pipelines-synchronously (pipeline)
"Execute the commands in PIPELINE in sequence synchronously.
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index f2bc87374c1..5002cc50dc3 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -302,35 +302,51 @@ eshell-create-handles
The result is a vector of file handles. Each handle is of the form:
- (TARGETS DEFAULT REF-COUNT)
+ ((TARGETS . REF-COUNT) DEFAULT)
-TARGETS is a list of destinations for output. DEFAULT is non-nil
-if handle has its initial default value (always t after calling
-this function). REF-COUNT is the number of references to this
-handle (initially 1); see `eshell-protect-handles' and
-`eshell-close-handles'."
+TARGETS is a list of destinations for output. REF-COUNT is the
+number of references to this handle (initially 1); see
+`eshell-protect-handles' and `eshell-close-handles'. DEFAULT is
+non-nil if handle has its initial default value (always t after
+calling this function)."
(let* ((handles (make-vector eshell-number-of-handles nil))
- (output-target (eshell-get-targets stdout output-mode))
- (error-target (if stderr
- (eshell-get-targets stderr error-mode)
- output-target)))
- (aset handles eshell-output-handle (list output-target t 1))
- (aset handles eshell-error-handle (list error-target t 1))
+ (output-target
+ (let ((target (eshell-get-target stdout output-mode)))
+ (cons (when target (list target)) 1)))
+ (error-target
+ (if stderr
+ (let ((target (eshell-get-target stderr error-mode)))
+ (cons (when target (list target)) 1))
+ (cl-incf (cdr output-target))
+ output-target)))
+ (aset handles eshell-output-handle (list output-target t))
+ (aset handles eshell-error-handle (list error-target t))
handles))
-(defun eshell-duplicate-handles (handles)
+(defun eshell-duplicate-handles (handles &optional steal-p)
"Create a duplicate of the file handles in HANDLES.
-This will copy the targets of each handle in HANDLES, setting the
-DEFAULT field to t (see `eshell-create-handles')."
- (eshell-create-handles
- (car (aref handles eshell-output-handle)) nil
- (car (aref handles eshell-error-handle)) nil))
+This uses the targets of each handle in HANDLES, incrementing its
+reference count by one (unless STEAL-P is non-nil). These
+targets are shared between the original set of handles and the
+new one, so the targets are only closed when the reference count
+drops to 0 (see `eshell-close-handles').
+
+This function also sets the DEFAULT field for each handle to
+t (see `eshell-create-handles'). Unlike the targets, this value
+is not shared with the original handles."
+ (let ((dup-handles (make-vector eshell-number-of-handles nil)))
+ (dotimes (idx eshell-number-of-handles)
+ (when-let ((handle (aref handles idx)))
+ (unless steal-p
+ (cl-incf (cdar handle)))
+ (aset dup-handles idx (list (car handle) t))))
+ dup-handles))
(defun eshell-protect-handles (handles)
"Protect the handles in HANDLES from a being closed."
(dotimes (idx eshell-number-of-handles)
(when-let ((handle (aref handles idx)))
- (setcar (nthcdr 2 handle) (1+ (nth 2 handle)))))
+ (cl-incf (cdar handle))))
handles)
(defun eshell-close-handles (&optional exit-code result handles)
@@ -351,26 +367,34 @@ eshell-close-handles
(let ((handles (or handles eshell-current-handles)))
(dotimes (idx eshell-number-of-handles)
(when-let ((handle (aref handles idx)))
- (setcar (nthcdr 2 handle) (1- (nth 2 handle)))
- (when (= (nth 2 handle) 0)
- (dolist (target (ensure-list (car (aref handles idx))))
+ (cl-assert (natnump (cdar handle)))
+ (when (and (> (cdar handle) 0)
+ (= (cl-decf (cdar handle)) 0))
+ (dolist (target (caar handle))
(eshell-close-target target (= eshell-last-command-status 0)))
- (setcar handle nil))))))
+ (setcar (car handle) nil))))))
(defun eshell-set-output-handle (index mode &optional target handles)
"Set handle INDEX for the current HANDLES to point to TARGET using MODE.
-If HANDLES is nil, use `eshell-current-handles'."
+If HANDLES is nil, use `eshell-current-handles'.
+
+If the handle is currently set to its default value (see
+`eshell-create-handles'), this will overwrite the targets with
+the new target. Otherwise, it will append the new target to the
+current list of targets."
(when target
(let* ((handles (or handles eshell-current-handles))
(handle (or (aref handles index)
- (aset handles index (list nil nil 1))))
- (defaultp (cadr handle))
- (current (unless defaultp (car handle))))
+ (aset handles index (list (cons nil 1) nil))))
+ (defaultp (cadr handle)))
+ (when defaultp
+ (cl-decf (cdar handle))
+ (setcar handle (cons nil 1)))
(catch 'eshell-null-device
- (let ((where (eshell-get-target target mode)))
+ (let ((current (caar handle))
+ (where (eshell-get-target target mode)))
(unless (member where current)
- (setq current (append current (list where))))))
- (setcar handle current)
+ (setcar (car handle) (append current (list where))))))
(setcar (cdr handle) nil))))
(defun eshell-copy-output-handle (index index-to-copy &optional handles)
@@ -378,10 +402,7 @@ eshell-copy-output-handle
If HANDLES is nil, use `eshell-current-handles'."
(let* ((handles (or handles eshell-current-handles))
(handle-to-copy (car (aref handles index-to-copy))))
- (setcar (aref handles index)
- (if (listp handle-to-copy)
- (copy-sequence handle-to-copy)
- handle-to-copy))))
+ (setcar (aref handles index) handle-to-copy)))
(defun eshell-set-all-output-handles (mode &optional target handles)
"Set output and error HANDLES to point to TARGET using MODE.
@@ -501,13 +522,6 @@ eshell-get-target
(error "Invalid redirection target: %s"
(eshell-stringify target)))))
-(defun eshell-get-targets (targets &optional mode)
- "Convert TARGETS into valid output targets.
-TARGETS can be a single raw target or a list thereof. MODE is either
-`overwrite', `append' or `insert'; if it is omitted or nil, it
-defaults to `insert'."
- (mapcar (lambda (i) (eshell-get-target i mode)) (ensure-list targets)))
-
(defun eshell-interactive-output-p (&optional index handles)
"Return non-nil if the specified handle is bound for interactive display.
HANDLES is the set of handles to check; if nil, use
@@ -519,9 +533,9 @@ eshell-interactive-output-p
(let ((handles (or handles eshell-current-handles))
(index (or index eshell-output-handle)))
(if (eq index 'all)
- (and (equal (car (aref handles eshell-output-handle)) '(t))
- (equal (car (aref handles eshell-error-handle)) '(t)))
- (equal (car (aref handles index)) '(t)))))
+ (and (equal (caar (aref handles eshell-output-handle)) '(t))
+ (equal (caar (aref handles eshell-error-handle)) '(t)))
+ (equal (caar (aref handles index)) '(t)))))
(defvar eshell-print-queue nil)
(defvar eshell-print-queue-count -1)
@@ -628,8 +642,8 @@ eshell-output-object
If HANDLE-INDEX is nil, output to `eshell-output-handle'.
HANDLES is the set of file handles to use; if nil, use
`eshell-current-handles'."
- (let ((targets (car (aref (or handles eshell-current-handles)
- (or handle-index eshell-output-handle)))))
+ (let ((targets (caar (aref (or handles eshell-current-handles)
+ (or handle-index eshell-output-handle)))))
(dolist (target targets)
(eshell-output-object-to-target object target))))
diff --git a/test/lisp/eshell/em-extpipe-tests.el b/test/lisp/eshell/em-extpipe-tests.el
index a2646a0296b..04e78279427 100644
--- a/test/lisp/eshell/em-extpipe-tests.el
+++ b/test/lisp/eshell/em-extpipe-tests.el
@@ -42,7 +42,7 @@ em-extpipe-tests--deftest
(shell-command-switch "-c"))
;; Strip `eshell-trap-errors'.
(should (equal ,expected
- (cadadr (eshell-parse-command input))))))
+ (cadr (eshell-parse-command input))))))
(with-substitute-for-temp (&rest body)
;; Substitute name of an actual temporary file and/or
;; buffer into `input'. The substitution logic is
diff --git a/test/lisp/eshell/em-tramp-tests.el b/test/lisp/eshell/em-tramp-tests.el
index 982a1eba279..6cc35ecdb1b 100644
--- a/test/lisp/eshell/em-tramp-tests.el
+++ b/test/lisp/eshell/em-tramp-tests.el
@@ -27,23 +27,21 @@ em-tramp-test/su-default
"Test Eshell `su' command with no arguments."
(should (equal
(catch 'eshell-replace-command (eshell/su))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:root@%s:%s"
- tramp-default-host default-directory))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/su:root@%s:%s"
+ tramp-default-host default-directory)))))))
(ert-deftest em-tramp-test/su-user ()
"Test Eshell `su' command with USER argument."
(should (equal
(catch 'eshell-replace-command (eshell/su "USER"))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:USER@%s:%s"
- tramp-default-host default-directory))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/su:USER@%s:%s"
+ tramp-default-host default-directory)))))))
(ert-deftest em-tramp-test/su-login ()
"Test Eshell `su' command with -/-l/--login option."
@@ -52,11 +50,10 @@ em-tramp-test/su-login
("-")))
(should (equal
(catch 'eshell-replace-command (apply #'eshell/su args))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:root@%s:~/" tramp-default-host)))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/su:root@%s:~/" tramp-default-host))))))))
(defun mock-eshell-named-command (&rest args)
"Dummy function to test Eshell `sudo' command rewriting."
@@ -94,23 +91,21 @@ em-tramp-test/sudo-shell
("-s")))
(should (equal
(catch 'eshell-replace-command (apply #'eshell/sudo args))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/sudo:root@%s:%s"
- tramp-default-host default-directory)))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/sudo:root@%s:%s"
+ tramp-default-host default-directory))))))))
(ert-deftest em-tramp-test/sudo-user-shell ()
"Test Eshell `sudo' command with -s and -u options."
(should (equal
(catch 'eshell-replace-command (eshell/sudo "-u" "USER" "-s"))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/sudo:USER@%s:%s"
- tramp-default-host default-directory))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/sudo:USER@%s:%s"
+ tramp-default-host default-directory)))))))
(ert-deftest em-tramp-test/doas-basic ()
"Test Eshell `doas' command with default user."
@@ -149,22 +144,20 @@ em-tramp-test/doas-shell
("-s")))
(should (equal
(catch 'eshell-replace-command (apply #'eshell/doas args))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/doas:root@%s:%s"
- tramp-default-host default-directory)))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/doas:root@%s:%s"
+ tramp-default-host default-directory))))))))
(ert-deftest em-tramp-test/doas-user-shell ()
"Test Eshell `doas' command with -s and -u options."
(should (equal
(catch 'eshell-replace-command (eshell/doas "-u" "USER" "-s"))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/doas:USER@%s:%s"
- tramp-default-host default-directory))))))))
+ `(eshell-trap-errors
+ (eshell-named-command
+ "cd"
+ (list ,(format "/doas:USER@%s:%s"
+ tramp-default-host default-directory)))))))
;;; em-tramp-tests.el ends here
diff --git a/test/lisp/eshell/esh-io-tests.el b/test/lisp/eshell/esh-io-tests.el
index 9a3c14f365f..0f09afa19e4 100644
--- a/test/lisp/eshell/esh-io-tests.el
+++ b/test/lisp/eshell/esh-io-tests.el
@@ -301,15 +301,28 @@ esh-io-test/redirect-copy-first
"stderr\n"))
(should (equal (buffer-string) "stdout\n"))))
-(ert-deftest esh-io-test/redirect-pipe ()
- "Check that \"redirecting\" to a pipe works."
- ;; `|' should only redirect stdout.
+\f
+;; Pipelines
+
+(ert-deftest esh-io-test/pipeline/default ()
+ "Check that `|' only pipes stdout."
+ (skip-unless (executable-find "rev"))
(eshell-command-result-equal "test-output | rev"
- "stderr\ntuodts\n")
- ;; `|&' should redirect stdout and stderr.
+ "stderr\ntuodts\n"))
+
+
+(ert-deftest esh-io-test/pipeline/all ()
+ "Check that `|&' only pipes stdout and stderr."
+ (skip-unless (executable-find "rev"))
(eshell-command-result-equal "test-output |& rev"
"tuodts\nrredts\n"))
+(ert-deftest esh-io-test/pipeline/subcommands ()
+ "Chek that all commands in a subcommand are properly piped."
+ (skip-unless (executable-find "rev"))
+ (eshell-command-result-equal "{echo foo; echo bar} | rev"
+ "raboof"))
+
\f
;; Virtual targets
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
2022-12-25 1:36 ` Jim Porter
@ 2022-12-25 21:49 ` Jim Porter
2022-12-26 19:50 ` Jim Porter
0 siblings, 1 reply; 14+ messages in thread
From: Jim Porter @ 2022-12-25 21:49 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 59545, milan.zimmermann, michael.albinus
[-- Attachment #1: Type: text/plain, Size: 966 bytes --]
On 12/24/2022 5:36 PM, Jim Porter wrote:
> I'll also see if I can fix the FIXME comment I added, but this is a part
> of Eshell that's fairly brittle, and I think the *real* fix for that is
> moving to running Eshell commands in a separate thread, as discussed on
> emacs-devel.
Ok, it turns out that the regression test that was failing
(eshell-tests/queue-input) wasn't testing the right thing, so I've fixed
that and also found a real bug in the queued-input code. The FIXME
comment is now resolved, although I admit I'm not 100% sure why it
helped improve things. I still don't entirely understand
'eshell-do-eval's inner workings...
I also added an assertion to make sure we're not trying to close I/O
handles more times than we should, which revealed another bug (this time
with my patch), so I've fixed that too.
I think this should resolve all the issues now, so unless anyone has
objections, I'll merge this to the master branch in a few days.
[-- Attachment #2: 0001-Fix-reference-counting-of-Eshell-I-O-handles.patch --]
[-- Type: text/plain, Size: 24614 bytes --]
From deeeba9613732c1624fe841928574530a4e453ba Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 24 Dec 2022 14:31:50 -0800
Subject: [PATCH] Fix reference-counting of Eshell I/O handles
This ensures that output targets in Eshell are only closed when Eshell
is actually done with them. In particular, this means that
"{ echo foo; echo bar } | rev" prints "raboof" as expected
(bug#59545).
* lisp/eshell/esh-io.el (eshell-create-handles): Structure the handles
differently so the targets and their ref-count can be shared.
(eshell-duplicate-handles): Reimplement this to share targets between
the original and new handle sets. Add STEAL-P argument.
(eshell-protect-handles, eshell-copy-output-handle)
(eshell-interactive-output-p, eshell-output-object): Account for
changes to the handle structure.
(eshell-close-handle): New function...
(eshell-close-handles, eshell-set-output-handle): ... use it.
(eshell-get-targets): Remove. This only existed to make the previous
implementation of 'eshell-duplicate-handles' work.
* lisp/eshell/esh-cmd.el (eshell-with-copied-handles): New argument
STEAL-P.
(eshell-do-pipelines): Use STEAL-P for the last item in the pipeline.
(eshell-parse-command): Don't copy handles for the last command in the
list; explain why we can't use STEAL-P here.
(eshell-eval-command): When queuing input, set 'eshell-command-body'
and 'eshell-test-body' for the 'if' conditional (see
'eshell-do-eval').
* test/lisp/eshell/esh-io-tests.el (esh-io-test/redirect-pipe): Split
into...
(esh-io-test/pipeline/default, esh-io-test/pipeline/all): ... these.
(esh-io-test/pipeline/subcommands): New test.
* test/lisp/eshell/eshell-test-helpers.el
(eshell-test--max-subprocess-time): Rename to...
(eshell-test--max-wait-time): ... this.
(eshell-wait-for): New function...
(eshell-wait-for-subprocess): ... use it.
* test/lisp/eshell/eshell-tests.el (eshell-test/queue-input): Fix this
test. Previously, it didn't correctly verify that the original
command completed.
* test/lisp/eshell/em-tramp-tests.el
(em-tramp-test/should-replace-command): New macro...
(em-tramp-test/su-default, em-tramp-test/su-user)
(em-tramp-test/su-login, em-tramp-test/sudo-shell)
(em-tramp-test/sudo-user-shell, em-tramp-test/doas-shell)
(em-tramp-test/doas-user-shell): ... use it.
---
lisp/eshell/esh-cmd.el | 25 +++--
lisp/eshell/esh-io.el | 123 ++++++++++++++---------
test/lisp/eshell/em-tramp-tests.el | 99 ++++++++----------
test/lisp/eshell/esh-io-tests.el | 23 ++++-
test/lisp/eshell/eshell-tests-helpers.el | 29 ++++--
test/lisp/eshell/eshell-tests.el | 19 ++--
6 files changed, 180 insertions(+), 138 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 79957aeb416..6ba4ee41e70 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -419,11 +419,10 @@ eshell-parse-command
(let ((cmd commands))
(while cmd
;; Copy I/O handles so each full statement can manipulate them
- ;; if they like. As a small optimization, skip this for the
- ;; last top-level one; we won't use these handles again
- ;; anyway.
- (when (or (not toplevel) (cdr cmd))
- (setcar cmd `(eshell-with-copied-handles ,(car cmd))))
+ ;; if they like. Skip this for the last command in the list
+ ;; though; we won't use these handles again anyway.
+ (setcar cmd `(eshell-with-copied-handles
+ ,(car cmd) ,(not (cdr cmd))))
(setq cmd (cdr cmd))))
(if toplevel
`(eshell-commands (progn
@@ -792,10 +791,12 @@ eshell-trap-errors
(defvar eshell-output-handle) ;Defined in esh-io.el.
(defvar eshell-error-handle) ;Defined in esh-io.el.
-(defmacro eshell-with-copied-handles (object)
- "Duplicate current I/O handles, so OBJECT works with its own copy."
+(defmacro eshell-with-copied-handles (object &optional steal-p)
+ "Duplicate current I/O handles, so OBJECT works with its own copy.
+If STEAL-P is non-nil, these new handles will be stolen from the
+current ones (see `eshell-duplicate-handles')."
`(let ((eshell-current-handles
- (eshell-duplicate-handles eshell-current-handles)))
+ (eshell-duplicate-handles eshell-current-handles ,steal-p)))
,object))
(define-obsolete-function-alias 'eshell-copy-handles
@@ -836,7 +837,9 @@ eshell-do-pipelines
(let ((proc ,(car pipeline)))
(set headproc (or proc (symbol-value headproc)))
(set tailproc (or (symbol-value tailproc) proc))
- proc))))))
+ proc)))
+ ;; Steal handles if this is the last item in the pipeline.
+ ,(null (cdr pipeline)))))
(defmacro eshell-do-pipelines-synchronously (pipeline)
"Execute the commands in PIPELINE in sequence synchronously.
@@ -1024,7 +1027,9 @@ eshell-eval-command
;; We can just stick the new command at the end of the current
;; one, and everything will happen as it should.
(setcdr (last (cdr eshell-current-command))
- (list `(let ((here (and (eobp) (point))))
+ (list `(let ((here (and (eobp) (point)))
+ (eshell-command-body '(nil))
+ (eshell-test-body '(nil)))
,(and input
`(insert-and-inherit ,(concat input "\n")))
(if here
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index f2bc87374c1..90826a312b3 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -302,35 +302,51 @@ eshell-create-handles
The result is a vector of file handles. Each handle is of the form:
- (TARGETS DEFAULT REF-COUNT)
+ ((TARGETS . REF-COUNT) DEFAULT)
-TARGETS is a list of destinations for output. DEFAULT is non-nil
-if handle has its initial default value (always t after calling
-this function). REF-COUNT is the number of references to this
-handle (initially 1); see `eshell-protect-handles' and
-`eshell-close-handles'."
+TARGETS is a list of destinations for output. REF-COUNT is the
+number of references to this handle (initially 1); see
+`eshell-protect-handles' and `eshell-close-handles'. DEFAULT is
+non-nil if handle has its initial default value (always t after
+calling this function)."
(let* ((handles (make-vector eshell-number-of-handles nil))
- (output-target (eshell-get-targets stdout output-mode))
- (error-target (if stderr
- (eshell-get-targets stderr error-mode)
- output-target)))
- (aset handles eshell-output-handle (list output-target t 1))
- (aset handles eshell-error-handle (list error-target t 1))
+ (output-target
+ (let ((target (eshell-get-target stdout output-mode)))
+ (cons (when target (list target)) 1)))
+ (error-target
+ (if stderr
+ (let ((target (eshell-get-target stderr error-mode)))
+ (cons (when target (list target)) 1))
+ (cl-incf (cdr output-target))
+ output-target)))
+ (aset handles eshell-output-handle (list output-target t))
+ (aset handles eshell-error-handle (list error-target t))
handles))
-(defun eshell-duplicate-handles (handles)
+(defun eshell-duplicate-handles (handles &optional steal-p)
"Create a duplicate of the file handles in HANDLES.
-This will copy the targets of each handle in HANDLES, setting the
-DEFAULT field to t (see `eshell-create-handles')."
- (eshell-create-handles
- (car (aref handles eshell-output-handle)) nil
- (car (aref handles eshell-error-handle)) nil))
+This uses the targets of each handle in HANDLES, incrementing its
+reference count by one (unless STEAL-P is non-nil). These
+targets are shared between the original set of handles and the
+new one, so the targets are only closed when the reference count
+drops to 0 (see `eshell-close-handles').
+
+This function also sets the DEFAULT field for each handle to
+t (see `eshell-create-handles'). Unlike the targets, this value
+is not shared with the original handles."
+ (let ((dup-handles (make-vector eshell-number-of-handles nil)))
+ (dotimes (idx eshell-number-of-handles)
+ (when-let ((handle (aref handles idx)))
+ (unless steal-p
+ (cl-incf (cdar handle)))
+ (aset dup-handles idx (list (car handle) t))))
+ dup-handles))
(defun eshell-protect-handles (handles)
"Protect the handles in HANDLES from a being closed."
(dotimes (idx eshell-number-of-handles)
(when-let ((handle (aref handles idx)))
- (setcar (nthcdr 2 handle) (1+ (nth 2 handle)))))
+ (cl-incf (cdar handle))))
handles)
(defun eshell-close-handles (&optional exit-code result handles)
@@ -348,29 +364,45 @@ eshell-close-handles
(when result
(cl-assert (eq (car result) 'quote))
(setq eshell-last-command-result (cadr result)))
- (let ((handles (or handles eshell-current-handles)))
+ (let ((handles (or handles eshell-current-handles))
+ (succeeded (= eshell-last-command-status 0)))
(dotimes (idx eshell-number-of-handles)
- (when-let ((handle (aref handles idx)))
- (setcar (nthcdr 2 handle) (1- (nth 2 handle)))
- (when (= (nth 2 handle) 0)
- (dolist (target (ensure-list (car (aref handles idx))))
- (eshell-close-target target (= eshell-last-command-status 0)))
- (setcar handle nil))))))
+ (eshell-close-handle (aref handles idx) succeeded))))
+
+(defun eshell-close-handle (handle status)
+ "Close a single HANDLE, taking refcounts into account.
+This will pass STATUS to each target for the handle, which should
+be a non-nil value on successful termination."
+ (when handle
+ (cl-assert (> (cdar handle) 0)
+ "Attempted to close a handle with 0 references")
+ (when (and (> (cdar handle) 0)
+ (= (cl-decf (cdar handle)) 0))
+ (dolist (target (caar handle))
+ (eshell-close-target target status))
+ (setcar (car handle) nil))))
(defun eshell-set-output-handle (index mode &optional target handles)
"Set handle INDEX for the current HANDLES to point to TARGET using MODE.
-If HANDLES is nil, use `eshell-current-handles'."
+If HANDLES is nil, use `eshell-current-handles'.
+
+If the handle is currently set to its default value (see
+`eshell-create-handles'), this will overwrite the targets with
+the new target. Otherwise, it will append the new target to the
+current list of targets."
(when target
(let* ((handles (or handles eshell-current-handles))
(handle (or (aref handles index)
- (aset handles index (list nil nil 1))))
- (defaultp (cadr handle))
- (current (unless defaultp (car handle))))
+ (aset handles index (list (cons nil 1) nil))))
+ (defaultp (cadr handle)))
+ (when defaultp
+ (cl-decf (cdar handle))
+ (setcar handle (cons nil 1)))
(catch 'eshell-null-device
- (let ((where (eshell-get-target target mode)))
+ (let ((current (caar handle))
+ (where (eshell-get-target target mode)))
(unless (member where current)
- (setq current (append current (list where))))))
- (setcar handle current)
+ (setcar (car handle) (append current (list where))))))
(setcar (cdr handle) nil))))
(defun eshell-copy-output-handle (index index-to-copy &optional handles)
@@ -378,10 +410,10 @@ eshell-copy-output-handle
If HANDLES is nil, use `eshell-current-handles'."
(let* ((handles (or handles eshell-current-handles))
(handle-to-copy (car (aref handles index-to-copy))))
- (setcar (aref handles index)
- (if (listp handle-to-copy)
- (copy-sequence handle-to-copy)
- handle-to-copy))))
+ (when handle-to-copy
+ (cl-incf (cdr handle-to-copy)))
+ (eshell-close-handle (aref handles index) nil)
+ (setcar (aref handles index) handle-to-copy)))
(defun eshell-set-all-output-handles (mode &optional target handles)
"Set output and error HANDLES to point to TARGET using MODE.
@@ -501,13 +533,6 @@ eshell-get-target
(error "Invalid redirection target: %s"
(eshell-stringify target)))))
-(defun eshell-get-targets (targets &optional mode)
- "Convert TARGETS into valid output targets.
-TARGETS can be a single raw target or a list thereof. MODE is either
-`overwrite', `append' or `insert'; if it is omitted or nil, it
-defaults to `insert'."
- (mapcar (lambda (i) (eshell-get-target i mode)) (ensure-list targets)))
-
(defun eshell-interactive-output-p (&optional index handles)
"Return non-nil if the specified handle is bound for interactive display.
HANDLES is the set of handles to check; if nil, use
@@ -519,9 +544,9 @@ eshell-interactive-output-p
(let ((handles (or handles eshell-current-handles))
(index (or index eshell-output-handle)))
(if (eq index 'all)
- (and (equal (car (aref handles eshell-output-handle)) '(t))
- (equal (car (aref handles eshell-error-handle)) '(t)))
- (equal (car (aref handles index)) '(t)))))
+ (and (equal (caar (aref handles eshell-output-handle)) '(t))
+ (equal (caar (aref handles eshell-error-handle)) '(t)))
+ (equal (caar (aref handles index)) '(t)))))
(defvar eshell-print-queue nil)
(defvar eshell-print-queue-count -1)
@@ -628,8 +653,8 @@ eshell-output-object
If HANDLE-INDEX is nil, output to `eshell-output-handle'.
HANDLES is the set of file handles to use; if nil, use
`eshell-current-handles'."
- (let ((targets (car (aref (or handles eshell-current-handles)
- (or handle-index eshell-output-handle)))))
+ (let ((targets (caar (aref (or handles eshell-current-handles)
+ (or handle-index eshell-output-handle)))))
(dolist (target targets)
(eshell-output-object-to-target object target))))
diff --git a/test/lisp/eshell/em-tramp-tests.el b/test/lisp/eshell/em-tramp-tests.el
index 982a1eba279..936397d8869 100644
--- a/test/lisp/eshell/em-tramp-tests.el
+++ b/test/lisp/eshell/em-tramp-tests.el
@@ -23,40 +23,41 @@
(require 'em-tramp)
(require 'tramp)
+(defmacro em-tramp-test/should-replace-command (form replacement)
+ "Check that calling FORM results in it being replaced with REPLACEMENT."
+ (declare (indent 1))
+ `(should (equal
+ (catch 'eshell-replace-command ,form)
+ (list 'eshell-with-copied-handles
+ (list 'eshell-trap-errors
+ ,replacement)
+ t))))
+
(ert-deftest em-tramp-test/su-default ()
"Test Eshell `su' command with no arguments."
- (should (equal
- (catch 'eshell-replace-command (eshell/su))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:root@%s:%s"
- tramp-default-host default-directory))))))))
+ (em-tramp-test/should-replace-command (eshell/su)
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/su:root@%s:%s"
+ tramp-default-host default-directory)))))
(ert-deftest em-tramp-test/su-user ()
"Test Eshell `su' command with USER argument."
- (should (equal
- (catch 'eshell-replace-command (eshell/su "USER"))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:USER@%s:%s"
- tramp-default-host default-directory))))))))
+ (em-tramp-test/should-replace-command (eshell/su "USER")
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/su:USER@%s:%s"
+ tramp-default-host default-directory)))))
(ert-deftest em-tramp-test/su-login ()
"Test Eshell `su' command with -/-l/--login option."
(dolist (args '(("--login")
("-l")
("-")))
- (should (equal
- (catch 'eshell-replace-command (apply #'eshell/su args))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:root@%s:~/" tramp-default-host)))))))))
+ (em-tramp-test/should-replace-command (apply #'eshell/su args)
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/su:root@%s:~/" tramp-default-host))))))
(defun mock-eshell-named-command (&rest args)
"Dummy function to test Eshell `sudo' command rewriting."
@@ -92,25 +93,19 @@ em-tramp-test/sudo-shell
"Test Eshell `sudo' command with -s/--shell option."
(dolist (args '(("--shell")
("-s")))
- (should (equal
- (catch 'eshell-replace-command (apply #'eshell/sudo args))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/sudo:root@%s:%s"
- tramp-default-host default-directory)))))))))
+ (em-tramp-test/should-replace-command (apply #'eshell/sudo args)
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/sudo:root@%s:%s"
+ tramp-default-host default-directory))))))
(ert-deftest em-tramp-test/sudo-user-shell ()
"Test Eshell `sudo' command with -s and -u options."
- (should (equal
- (catch 'eshell-replace-command (eshell/sudo "-u" "USER" "-s"))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/sudo:USER@%s:%s"
- tramp-default-host default-directory))))))))
+ (em-tramp-test/should-replace-command (eshell/sudo "-u" "USER" "-s")
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/sudo:USER@%s:%s"
+ tramp-default-host default-directory)))))
(ert-deftest em-tramp-test/doas-basic ()
"Test Eshell `doas' command with default user."
@@ -147,24 +142,18 @@ em-tramp-test/doas-shell
"Test Eshell `doas' command with -s/--shell option."
(dolist (args '(("--shell")
("-s")))
- (should (equal
- (catch 'eshell-replace-command (apply #'eshell/doas args))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/doas:root@%s:%s"
- tramp-default-host default-directory)))))))))
+ (em-tramp-test/should-replace-command (apply #'eshell/doas args)
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/doas:root@%s:%s"
+ tramp-default-host default-directory))))))
(ert-deftest em-tramp-test/doas-user-shell ()
"Test Eshell `doas' command with -s and -u options."
- (should (equal
- (catch 'eshell-replace-command (eshell/doas "-u" "USER" "-s"))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/doas:USER@%s:%s"
- tramp-default-host default-directory))))))))
+ (em-tramp-test/should-replace-command (eshell/doas "-u" "USER" "-s")
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/doas:USER@%s:%s"
+ tramp-default-host default-directory)))))
;;; em-tramp-tests.el ends here
diff --git a/test/lisp/eshell/esh-io-tests.el b/test/lisp/eshell/esh-io-tests.el
index 9a3c14f365f..0f09afa19e4 100644
--- a/test/lisp/eshell/esh-io-tests.el
+++ b/test/lisp/eshell/esh-io-tests.el
@@ -301,15 +301,28 @@ esh-io-test/redirect-copy-first
"stderr\n"))
(should (equal (buffer-string) "stdout\n"))))
-(ert-deftest esh-io-test/redirect-pipe ()
- "Check that \"redirecting\" to a pipe works."
- ;; `|' should only redirect stdout.
+\f
+;; Pipelines
+
+(ert-deftest esh-io-test/pipeline/default ()
+ "Check that `|' only pipes stdout."
+ (skip-unless (executable-find "rev"))
(eshell-command-result-equal "test-output | rev"
- "stderr\ntuodts\n")
- ;; `|&' should redirect stdout and stderr.
+ "stderr\ntuodts\n"))
+
+
+(ert-deftest esh-io-test/pipeline/all ()
+ "Check that `|&' only pipes stdout and stderr."
+ (skip-unless (executable-find "rev"))
(eshell-command-result-equal "test-output |& rev"
"tuodts\nrredts\n"))
+(ert-deftest esh-io-test/pipeline/subcommands ()
+ "Chek that all commands in a subcommand are properly piped."
+ (skip-unless (executable-find "rev"))
+ (eshell-command-result-equal "{echo foo; echo bar} | rev"
+ "raboof"))
+
\f
;; Virtual targets
diff --git a/test/lisp/eshell/eshell-tests-helpers.el b/test/lisp/eshell/eshell-tests-helpers.el
index 1d9674070c0..a9338050311 100644
--- a/test/lisp/eshell/eshell-tests-helpers.el
+++ b/test/lisp/eshell/eshell-tests-helpers.el
@@ -33,9 +33,9 @@
(defvar eshell-history-file-name nil)
(defvar eshell-last-dir-ring-file-name nil)
-(defvar eshell-test--max-subprocess-time 5
- "The maximum amount of time to wait for a subprocess to finish, in seconds.
-See `eshell-wait-for-subprocess'.")
+(defvar eshell-test--max-wait-time 5
+ "The maximum amount of time to wait for a condition to resolve, in seconds.
+See `eshell-wait-for'.")
(defun eshell-tests-remote-accessible-p ()
"Return if a test involving remote files can proceed.
@@ -73,19 +73,28 @@ eshell-with-temp-buffer
(let ((,bufname (buffer-name)))
,@body)))
+(defun eshell-wait-for (predicate &optional message)
+ "Wait until PREDICATE returns non-nil.
+If this takes longer than `eshell-test--max-wait-time', raise an
+error. MESSAGE is an optional message to use if this times out."
+ (let ((start (current-time))
+ (message (or message "timed out waiting for condition")))
+ (while (not (funcall predicate))
+ (when (> (float-time (time-since start))
+ eshell-test--max-wait-time)
+ (error message))
+ (sit-for 0.1))))
+
(defun eshell-wait-for-subprocess (&optional all)
"Wait until there is no interactive subprocess running in Eshell.
If ALL is non-nil, wait until there are no Eshell subprocesses at
all running.
-If this takes longer than `eshell-test--max-subprocess-time',
+If this takes longer than `eshell-test--max-wait-time',
raise an error."
- (let ((start (current-time)))
- (while (if all eshell-process-list (eshell-interactive-process-p))
- (when (> (float-time (time-since start))
- eshell-test--max-subprocess-time)
- (error "timed out waiting for subprocess(es)"))
- (sit-for 0.1))))
+ (eshell-wait-for
+ (lambda ()
+ (not (if all eshell-process-list (eshell-interactive-process-p))))))
(defun eshell-insert-command (command &optional func)
"Insert a COMMAND at the end of the buffer.
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index c67ac67fd36..dd8be8e65f0 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -128,16 +128,17 @@ eshell-test/forward-arg
(delete-region (point) (point-max))))))
(ert-deftest eshell-test/queue-input ()
- "Test queuing command input"
+ "Test queuing command input.
+This should let the current command finish, then automatically
+insert the queued one at the next prompt, and finally run it."
(with-temp-eshell
- (eshell-insert-command "sleep 2")
- (eshell-insert-command "echo alpha" 'eshell-queue-input)
- (let ((count 10))
- (while (and eshell-current-command
- (> count 0))
- (sit-for 1)
- (setq count (1- count))))
- (should (eshell-match-output "alpha\n"))))
+ (eshell-insert-command "sleep 1; echo slept")
+ (eshell-insert-command "echo alpha" #'eshell-queue-input)
+ (let ((start (marker-position (eshell-beginning-of-output))))
+ (eshell-wait-for (lambda () (not eshell-current-command)))
+ (should (string-match "^slept\n.*echo alpha\nalpha\n$"
+ (buffer-substring-no-properties
+ start (eshell-end-of-output)))))))
(ert-deftest eshell-test/flush-output ()
"Test flushing of previous output"
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
2022-12-25 21:49 ` Jim Porter
@ 2022-12-26 19:50 ` Jim Porter
2022-12-30 6:40 ` Jim Porter
0 siblings, 1 reply; 14+ messages in thread
From: Jim Porter @ 2022-12-26 19:50 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: 59545, milan.zimmermann, michael.albinus
[-- Attachment #1: Type: text/plain, Size: 357 bytes --]
On 12/25/2022 1:49 PM, Jim Porter wrote:
> I think this should resolve all the issues now, so unless anyone has
> objections, I'll merge this to the master branch in a few days.
Attached is the same patch as before, but I added a few new tests to
ensure that I/O handles are properly closed after using control flow
forms like 'while' or 'if' in Eshell.
[-- Attachment #2: 0001-Fix-reference-counting-of-Eshell-I-O-handles.patch --]
[-- Type: text/plain, Size: 27897 bytes --]
From 8979a8d1bdd9bab3dacc8cfbc3dc178e11b0ede2 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 24 Dec 2022 14:31:50 -0800
Subject: [PATCH] Fix reference-counting of Eshell I/O handles
This ensures that output targets in Eshell are only closed when Eshell
is actually done with them. In particular, this means that
"{ echo foo; echo bar } | rev" prints "raboof" as expected
(bug#59545).
* lisp/eshell/esh-io.el (eshell-create-handles): Structure the handles
differently so the targets and their ref-count can be shared.
(eshell-duplicate-handles): Reimplement this to share targets between
the original and new handle sets. Add STEAL-P argument.
(eshell-protect-handles, eshell-copy-output-handle)
(eshell-interactive-output-p, eshell-output-object): Account for
changes to the handle structure.
(eshell-close-handle): New function...
(eshell-close-handles, eshell-set-output-handle): ... use it.
(eshell-get-targets): Remove. This only existed to make the previous
implementation of 'eshell-duplicate-handles' work.
* lisp/eshell/esh-cmd.el (eshell-with-copied-handles): New argument
STEAL-P.
(eshell-do-pipelines): Use STEAL-P for the last item in the pipeline.
(eshell-parse-command): Don't copy handles for the last command in the
list; explain why we can't use STEAL-P here.
(eshell-eval-command): When queuing input, set 'eshell-command-body'
and 'eshell-test-body' for the 'if' conditional (see
'eshell-do-eval').
* test/lisp/eshell/esh-io-tests.el (esh-io-test/redirect-pipe): Split
into...
(esh-io-test/pipeline/default, esh-io-test/pipeline/all): ... these.
(esh-io-test/pipeline/subcommands): New test.
* test/lisp/eshell/esh-cmd-tests.el (esh-cmd-test/for-loop-pipe)
(esh-cmd-test/while-loop-pipe, esh-cmd-test/if-statement-pipe)
esh-cmd-test/if-else-statement-pipe): New tests.
(esh-cmd-test/while-loop): Use 'pop' to simplify the test a bit.
* test/lisp/eshell/eshell-test-helpers.el
(eshell-test--max-subprocess-time): Rename to...
(eshell-test--max-wait-time): ... this.
(eshell-wait-for): New function...
(eshell-wait-for-subprocess): ... use it.
* test/lisp/eshell/eshell-tests.el (eshell-test/queue-input): Fix this
test. Previously, it didn't correctly verify that the original
command completed.
* test/lisp/eshell/em-tramp-tests.el
(em-tramp-test/should-replace-command): New macro...
(em-tramp-test/su-default, em-tramp-test/su-user)
(em-tramp-test/su-login, em-tramp-test/sudo-shell)
(em-tramp-test/sudo-user-shell, em-tramp-test/doas-shell)
(em-tramp-test/doas-user-shell): ... use it.
---
lisp/eshell/esh-cmd.el | 25 +++--
lisp/eshell/esh-io.el | 123 ++++++++++++++---------
test/lisp/eshell/em-tramp-tests.el | 99 ++++++++----------
test/lisp/eshell/esh-cmd-tests.el | 44 +++++++-
test/lisp/eshell/esh-io-tests.el | 23 ++++-
test/lisp/eshell/eshell-tests-helpers.el | 29 ++++--
test/lisp/eshell/eshell-tests.el | 19 ++--
7 files changed, 222 insertions(+), 140 deletions(-)
diff --git a/lisp/eshell/esh-cmd.el b/lisp/eshell/esh-cmd.el
index 79957aeb416..6ba4ee41e70 100644
--- a/lisp/eshell/esh-cmd.el
+++ b/lisp/eshell/esh-cmd.el
@@ -419,11 +419,10 @@ eshell-parse-command
(let ((cmd commands))
(while cmd
;; Copy I/O handles so each full statement can manipulate them
- ;; if they like. As a small optimization, skip this for the
- ;; last top-level one; we won't use these handles again
- ;; anyway.
- (when (or (not toplevel) (cdr cmd))
- (setcar cmd `(eshell-with-copied-handles ,(car cmd))))
+ ;; if they like. Skip this for the last command in the list
+ ;; though; we won't use these handles again anyway.
+ (setcar cmd `(eshell-with-copied-handles
+ ,(car cmd) ,(not (cdr cmd))))
(setq cmd (cdr cmd))))
(if toplevel
`(eshell-commands (progn
@@ -792,10 +791,12 @@ eshell-trap-errors
(defvar eshell-output-handle) ;Defined in esh-io.el.
(defvar eshell-error-handle) ;Defined in esh-io.el.
-(defmacro eshell-with-copied-handles (object)
- "Duplicate current I/O handles, so OBJECT works with its own copy."
+(defmacro eshell-with-copied-handles (object &optional steal-p)
+ "Duplicate current I/O handles, so OBJECT works with its own copy.
+If STEAL-P is non-nil, these new handles will be stolen from the
+current ones (see `eshell-duplicate-handles')."
`(let ((eshell-current-handles
- (eshell-duplicate-handles eshell-current-handles)))
+ (eshell-duplicate-handles eshell-current-handles ,steal-p)))
,object))
(define-obsolete-function-alias 'eshell-copy-handles
@@ -836,7 +837,9 @@ eshell-do-pipelines
(let ((proc ,(car pipeline)))
(set headproc (or proc (symbol-value headproc)))
(set tailproc (or (symbol-value tailproc) proc))
- proc))))))
+ proc)))
+ ;; Steal handles if this is the last item in the pipeline.
+ ,(null (cdr pipeline)))))
(defmacro eshell-do-pipelines-synchronously (pipeline)
"Execute the commands in PIPELINE in sequence synchronously.
@@ -1024,7 +1027,9 @@ eshell-eval-command
;; We can just stick the new command at the end of the current
;; one, and everything will happen as it should.
(setcdr (last (cdr eshell-current-command))
- (list `(let ((here (and (eobp) (point))))
+ (list `(let ((here (and (eobp) (point)))
+ (eshell-command-body '(nil))
+ (eshell-test-body '(nil)))
,(and input
`(insert-and-inherit ,(concat input "\n")))
(if here
diff --git a/lisp/eshell/esh-io.el b/lisp/eshell/esh-io.el
index f2bc87374c1..90826a312b3 100644
--- a/lisp/eshell/esh-io.el
+++ b/lisp/eshell/esh-io.el
@@ -302,35 +302,51 @@ eshell-create-handles
The result is a vector of file handles. Each handle is of the form:
- (TARGETS DEFAULT REF-COUNT)
+ ((TARGETS . REF-COUNT) DEFAULT)
-TARGETS is a list of destinations for output. DEFAULT is non-nil
-if handle has its initial default value (always t after calling
-this function). REF-COUNT is the number of references to this
-handle (initially 1); see `eshell-protect-handles' and
-`eshell-close-handles'."
+TARGETS is a list of destinations for output. REF-COUNT is the
+number of references to this handle (initially 1); see
+`eshell-protect-handles' and `eshell-close-handles'. DEFAULT is
+non-nil if handle has its initial default value (always t after
+calling this function)."
(let* ((handles (make-vector eshell-number-of-handles nil))
- (output-target (eshell-get-targets stdout output-mode))
- (error-target (if stderr
- (eshell-get-targets stderr error-mode)
- output-target)))
- (aset handles eshell-output-handle (list output-target t 1))
- (aset handles eshell-error-handle (list error-target t 1))
+ (output-target
+ (let ((target (eshell-get-target stdout output-mode)))
+ (cons (when target (list target)) 1)))
+ (error-target
+ (if stderr
+ (let ((target (eshell-get-target stderr error-mode)))
+ (cons (when target (list target)) 1))
+ (cl-incf (cdr output-target))
+ output-target)))
+ (aset handles eshell-output-handle (list output-target t))
+ (aset handles eshell-error-handle (list error-target t))
handles))
-(defun eshell-duplicate-handles (handles)
+(defun eshell-duplicate-handles (handles &optional steal-p)
"Create a duplicate of the file handles in HANDLES.
-This will copy the targets of each handle in HANDLES, setting the
-DEFAULT field to t (see `eshell-create-handles')."
- (eshell-create-handles
- (car (aref handles eshell-output-handle)) nil
- (car (aref handles eshell-error-handle)) nil))
+This uses the targets of each handle in HANDLES, incrementing its
+reference count by one (unless STEAL-P is non-nil). These
+targets are shared between the original set of handles and the
+new one, so the targets are only closed when the reference count
+drops to 0 (see `eshell-close-handles').
+
+This function also sets the DEFAULT field for each handle to
+t (see `eshell-create-handles'). Unlike the targets, this value
+is not shared with the original handles."
+ (let ((dup-handles (make-vector eshell-number-of-handles nil)))
+ (dotimes (idx eshell-number-of-handles)
+ (when-let ((handle (aref handles idx)))
+ (unless steal-p
+ (cl-incf (cdar handle)))
+ (aset dup-handles idx (list (car handle) t))))
+ dup-handles))
(defun eshell-protect-handles (handles)
"Protect the handles in HANDLES from a being closed."
(dotimes (idx eshell-number-of-handles)
(when-let ((handle (aref handles idx)))
- (setcar (nthcdr 2 handle) (1+ (nth 2 handle)))))
+ (cl-incf (cdar handle))))
handles)
(defun eshell-close-handles (&optional exit-code result handles)
@@ -348,29 +364,45 @@ eshell-close-handles
(when result
(cl-assert (eq (car result) 'quote))
(setq eshell-last-command-result (cadr result)))
- (let ((handles (or handles eshell-current-handles)))
+ (let ((handles (or handles eshell-current-handles))
+ (succeeded (= eshell-last-command-status 0)))
(dotimes (idx eshell-number-of-handles)
- (when-let ((handle (aref handles idx)))
- (setcar (nthcdr 2 handle) (1- (nth 2 handle)))
- (when (= (nth 2 handle) 0)
- (dolist (target (ensure-list (car (aref handles idx))))
- (eshell-close-target target (= eshell-last-command-status 0)))
- (setcar handle nil))))))
+ (eshell-close-handle (aref handles idx) succeeded))))
+
+(defun eshell-close-handle (handle status)
+ "Close a single HANDLE, taking refcounts into account.
+This will pass STATUS to each target for the handle, which should
+be a non-nil value on successful termination."
+ (when handle
+ (cl-assert (> (cdar handle) 0)
+ "Attempted to close a handle with 0 references")
+ (when (and (> (cdar handle) 0)
+ (= (cl-decf (cdar handle)) 0))
+ (dolist (target (caar handle))
+ (eshell-close-target target status))
+ (setcar (car handle) nil))))
(defun eshell-set-output-handle (index mode &optional target handles)
"Set handle INDEX for the current HANDLES to point to TARGET using MODE.
-If HANDLES is nil, use `eshell-current-handles'."
+If HANDLES is nil, use `eshell-current-handles'.
+
+If the handle is currently set to its default value (see
+`eshell-create-handles'), this will overwrite the targets with
+the new target. Otherwise, it will append the new target to the
+current list of targets."
(when target
(let* ((handles (or handles eshell-current-handles))
(handle (or (aref handles index)
- (aset handles index (list nil nil 1))))
- (defaultp (cadr handle))
- (current (unless defaultp (car handle))))
+ (aset handles index (list (cons nil 1) nil))))
+ (defaultp (cadr handle)))
+ (when defaultp
+ (cl-decf (cdar handle))
+ (setcar handle (cons nil 1)))
(catch 'eshell-null-device
- (let ((where (eshell-get-target target mode)))
+ (let ((current (caar handle))
+ (where (eshell-get-target target mode)))
(unless (member where current)
- (setq current (append current (list where))))))
- (setcar handle current)
+ (setcar (car handle) (append current (list where))))))
(setcar (cdr handle) nil))))
(defun eshell-copy-output-handle (index index-to-copy &optional handles)
@@ -378,10 +410,10 @@ eshell-copy-output-handle
If HANDLES is nil, use `eshell-current-handles'."
(let* ((handles (or handles eshell-current-handles))
(handle-to-copy (car (aref handles index-to-copy))))
- (setcar (aref handles index)
- (if (listp handle-to-copy)
- (copy-sequence handle-to-copy)
- handle-to-copy))))
+ (when handle-to-copy
+ (cl-incf (cdr handle-to-copy)))
+ (eshell-close-handle (aref handles index) nil)
+ (setcar (aref handles index) handle-to-copy)))
(defun eshell-set-all-output-handles (mode &optional target handles)
"Set output and error HANDLES to point to TARGET using MODE.
@@ -501,13 +533,6 @@ eshell-get-target
(error "Invalid redirection target: %s"
(eshell-stringify target)))))
-(defun eshell-get-targets (targets &optional mode)
- "Convert TARGETS into valid output targets.
-TARGETS can be a single raw target or a list thereof. MODE is either
-`overwrite', `append' or `insert'; if it is omitted or nil, it
-defaults to `insert'."
- (mapcar (lambda (i) (eshell-get-target i mode)) (ensure-list targets)))
-
(defun eshell-interactive-output-p (&optional index handles)
"Return non-nil if the specified handle is bound for interactive display.
HANDLES is the set of handles to check; if nil, use
@@ -519,9 +544,9 @@ eshell-interactive-output-p
(let ((handles (or handles eshell-current-handles))
(index (or index eshell-output-handle)))
(if (eq index 'all)
- (and (equal (car (aref handles eshell-output-handle)) '(t))
- (equal (car (aref handles eshell-error-handle)) '(t)))
- (equal (car (aref handles index)) '(t)))))
+ (and (equal (caar (aref handles eshell-output-handle)) '(t))
+ (equal (caar (aref handles eshell-error-handle)) '(t)))
+ (equal (caar (aref handles index)) '(t)))))
(defvar eshell-print-queue nil)
(defvar eshell-print-queue-count -1)
@@ -628,8 +653,8 @@ eshell-output-object
If HANDLE-INDEX is nil, output to `eshell-output-handle'.
HANDLES is the set of file handles to use; if nil, use
`eshell-current-handles'."
- (let ((targets (car (aref (or handles eshell-current-handles)
- (or handle-index eshell-output-handle)))))
+ (let ((targets (caar (aref (or handles eshell-current-handles)
+ (or handle-index eshell-output-handle)))))
(dolist (target targets)
(eshell-output-object-to-target object target))))
diff --git a/test/lisp/eshell/em-tramp-tests.el b/test/lisp/eshell/em-tramp-tests.el
index 982a1eba279..936397d8869 100644
--- a/test/lisp/eshell/em-tramp-tests.el
+++ b/test/lisp/eshell/em-tramp-tests.el
@@ -23,40 +23,41 @@
(require 'em-tramp)
(require 'tramp)
+(defmacro em-tramp-test/should-replace-command (form replacement)
+ "Check that calling FORM results in it being replaced with REPLACEMENT."
+ (declare (indent 1))
+ `(should (equal
+ (catch 'eshell-replace-command ,form)
+ (list 'eshell-with-copied-handles
+ (list 'eshell-trap-errors
+ ,replacement)
+ t))))
+
(ert-deftest em-tramp-test/su-default ()
"Test Eshell `su' command with no arguments."
- (should (equal
- (catch 'eshell-replace-command (eshell/su))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:root@%s:%s"
- tramp-default-host default-directory))))))))
+ (em-tramp-test/should-replace-command (eshell/su)
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/su:root@%s:%s"
+ tramp-default-host default-directory)))))
(ert-deftest em-tramp-test/su-user ()
"Test Eshell `su' command with USER argument."
- (should (equal
- (catch 'eshell-replace-command (eshell/su "USER"))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:USER@%s:%s"
- tramp-default-host default-directory))))))))
+ (em-tramp-test/should-replace-command (eshell/su "USER")
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/su:USER@%s:%s"
+ tramp-default-host default-directory)))))
(ert-deftest em-tramp-test/su-login ()
"Test Eshell `su' command with -/-l/--login option."
(dolist (args '(("--login")
("-l")
("-")))
- (should (equal
- (catch 'eshell-replace-command (apply #'eshell/su args))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/su:root@%s:~/" tramp-default-host)))))))))
+ (em-tramp-test/should-replace-command (apply #'eshell/su args)
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/su:root@%s:~/" tramp-default-host))))))
(defun mock-eshell-named-command (&rest args)
"Dummy function to test Eshell `sudo' command rewriting."
@@ -92,25 +93,19 @@ em-tramp-test/sudo-shell
"Test Eshell `sudo' command with -s/--shell option."
(dolist (args '(("--shell")
("-s")))
- (should (equal
- (catch 'eshell-replace-command (apply #'eshell/sudo args))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/sudo:root@%s:%s"
- tramp-default-host default-directory)))))))))
+ (em-tramp-test/should-replace-command (apply #'eshell/sudo args)
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/sudo:root@%s:%s"
+ tramp-default-host default-directory))))))
(ert-deftest em-tramp-test/sudo-user-shell ()
"Test Eshell `sudo' command with -s and -u options."
- (should (equal
- (catch 'eshell-replace-command (eshell/sudo "-u" "USER" "-s"))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/sudo:USER@%s:%s"
- tramp-default-host default-directory))))))))
+ (em-tramp-test/should-replace-command (eshell/sudo "-u" "USER" "-s")
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/sudo:USER@%s:%s"
+ tramp-default-host default-directory)))))
(ert-deftest em-tramp-test/doas-basic ()
"Test Eshell `doas' command with default user."
@@ -147,24 +142,18 @@ em-tramp-test/doas-shell
"Test Eshell `doas' command with -s/--shell option."
(dolist (args '(("--shell")
("-s")))
- (should (equal
- (catch 'eshell-replace-command (apply #'eshell/doas args))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/doas:root@%s:%s"
- tramp-default-host default-directory)))))))))
+ (em-tramp-test/should-replace-command (apply #'eshell/doas args)
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/doas:root@%s:%s"
+ tramp-default-host default-directory))))))
(ert-deftest em-tramp-test/doas-user-shell ()
"Test Eshell `doas' command with -s and -u options."
- (should (equal
- (catch 'eshell-replace-command (eshell/doas "-u" "USER" "-s"))
- `(eshell-with-copied-handles
- (eshell-trap-errors
- (eshell-named-command
- "cd"
- (list ,(format "/doas:USER@%s:%s"
- tramp-default-host default-directory))))))))
+ (em-tramp-test/should-replace-command (eshell/doas "-u" "USER" "-s")
+ `(eshell-named-command
+ "cd"
+ (list ,(format "/doas:USER@%s:%s"
+ tramp-default-host default-directory)))))
;;; em-tramp-tests.el ends here
diff --git a/test/lisp/eshell/esh-cmd-tests.el b/test/lisp/eshell/esh-cmd-tests.el
index 92d785d7fdf..cc40dde3552 100644
--- a/test/lisp/eshell/esh-cmd-tests.el
+++ b/test/lisp/eshell/esh-cmd-tests.el
@@ -148,14 +148,21 @@ esh-cmd-test/for-name-shadow-loop
"echo $name; for name in 3 { echo $name }; echo $name"
"env-value\n3\nenv-value\n"))))
+(ert-deftest esh-cmd-test/for-loop-pipe ()
+ "Test invocation of a for loop piped to another command."
+ (skip-unless (executable-find "rev"))
+ (with-temp-eshell
+ (eshell-match-command-output "for i in foo bar baz { echo $i } | rev"
+ "zabraboof")))
+
(ert-deftest esh-cmd-test/while-loop ()
"Test invocation of a while loop."
(with-temp-eshell
(let ((eshell-test-value '(0 1 2)))
(eshell-match-command-output
(concat "while $eshell-test-value "
- "{ setq eshell-test-value (cdr eshell-test-value) }")
- "(1 2)\n(2)\n"))))
+ "{ (pop eshell-test-value) }")
+ "0\n1\n2\n"))))
(ert-deftest esh-cmd-test/while-loop-lisp-form ()
"Test invocation of a while loop using a Lisp form."
@@ -176,6 +183,17 @@ esh-cmd-test/while-loop-ext-cmd
"{ setq eshell-test-value (1+ eshell-test-value) }")
"1\n2\n3\n"))))
+(ert-deftest esh-cmd-test/while-loop-pipe ()
+ "Test invocation of a while loop piped to another command."
+ (skip-unless (executable-find "rev"))
+ (with-temp-eshell
+ (let ((eshell-test-value '("foo" "bar" "baz")))
+ (eshell-match-command-output
+ (concat "while $eshell-test-value "
+ "{ (pop eshell-test-value) }"
+ " | rev")
+ "zabraboof"))))
+
(ert-deftest esh-cmd-test/until-loop ()
"Test invocation of an until loop."
(with-temp-eshell
@@ -253,6 +271,28 @@ esh-cmd-test/if-else-statement-ext-cmd
(eshell-command-result-equal "if {[ foo = bar ]} {echo yes} {echo no}"
"no"))
+(ert-deftest esh-cmd-test/if-statement-pipe ()
+ "Test invocation of an if statement piped to another command."
+ (skip-unless (executable-find "rev"))
+ (let ((eshell-test-value t))
+ (eshell-command-result-equal "if $eshell-test-value {echo yes} | rev"
+ "sey"))
+ (let ((eshell-test-value nil))
+ (eshell-command-result-equal "if $eshell-test-value {echo yes} | rev"
+ nil)))
+
+(ert-deftest esh-cmd-test/if-else-statement-pipe ()
+ "Test invocation of an if/else statement piped to another command."
+ (skip-unless (executable-find "rev"))
+ (let ((eshell-test-value t))
+ (eshell-command-result-equal
+ "if $eshell-test-value {echo yes} {echo no} | rev"
+ "sey"))
+ (let ((eshell-test-value nil))
+ (eshell-command-result-equal
+ "if $eshell-test-value {echo yes} {echo no} | rev"
+ "on")))
+
(ert-deftest esh-cmd-test/unless-statement ()
"Test invocation of an unless statement."
(let ((eshell-test-value t))
diff --git a/test/lisp/eshell/esh-io-tests.el b/test/lisp/eshell/esh-io-tests.el
index 9a3c14f365f..0f09afa19e4 100644
--- a/test/lisp/eshell/esh-io-tests.el
+++ b/test/lisp/eshell/esh-io-tests.el
@@ -301,15 +301,28 @@ esh-io-test/redirect-copy-first
"stderr\n"))
(should (equal (buffer-string) "stdout\n"))))
-(ert-deftest esh-io-test/redirect-pipe ()
- "Check that \"redirecting\" to a pipe works."
- ;; `|' should only redirect stdout.
+\f
+;; Pipelines
+
+(ert-deftest esh-io-test/pipeline/default ()
+ "Check that `|' only pipes stdout."
+ (skip-unless (executable-find "rev"))
(eshell-command-result-equal "test-output | rev"
- "stderr\ntuodts\n")
- ;; `|&' should redirect stdout and stderr.
+ "stderr\ntuodts\n"))
+
+
+(ert-deftest esh-io-test/pipeline/all ()
+ "Check that `|&' only pipes stdout and stderr."
+ (skip-unless (executable-find "rev"))
(eshell-command-result-equal "test-output |& rev"
"tuodts\nrredts\n"))
+(ert-deftest esh-io-test/pipeline/subcommands ()
+ "Chek that all commands in a subcommand are properly piped."
+ (skip-unless (executable-find "rev"))
+ (eshell-command-result-equal "{echo foo; echo bar} | rev"
+ "raboof"))
+
\f
;; Virtual targets
diff --git a/test/lisp/eshell/eshell-tests-helpers.el b/test/lisp/eshell/eshell-tests-helpers.el
index 1d9674070c0..a9338050311 100644
--- a/test/lisp/eshell/eshell-tests-helpers.el
+++ b/test/lisp/eshell/eshell-tests-helpers.el
@@ -33,9 +33,9 @@
(defvar eshell-history-file-name nil)
(defvar eshell-last-dir-ring-file-name nil)
-(defvar eshell-test--max-subprocess-time 5
- "The maximum amount of time to wait for a subprocess to finish, in seconds.
-See `eshell-wait-for-subprocess'.")
+(defvar eshell-test--max-wait-time 5
+ "The maximum amount of time to wait for a condition to resolve, in seconds.
+See `eshell-wait-for'.")
(defun eshell-tests-remote-accessible-p ()
"Return if a test involving remote files can proceed.
@@ -73,19 +73,28 @@ eshell-with-temp-buffer
(let ((,bufname (buffer-name)))
,@body)))
+(defun eshell-wait-for (predicate &optional message)
+ "Wait until PREDICATE returns non-nil.
+If this takes longer than `eshell-test--max-wait-time', raise an
+error. MESSAGE is an optional message to use if this times out."
+ (let ((start (current-time))
+ (message (or message "timed out waiting for condition")))
+ (while (not (funcall predicate))
+ (when (> (float-time (time-since start))
+ eshell-test--max-wait-time)
+ (error message))
+ (sit-for 0.1))))
+
(defun eshell-wait-for-subprocess (&optional all)
"Wait until there is no interactive subprocess running in Eshell.
If ALL is non-nil, wait until there are no Eshell subprocesses at
all running.
-If this takes longer than `eshell-test--max-subprocess-time',
+If this takes longer than `eshell-test--max-wait-time',
raise an error."
- (let ((start (current-time)))
- (while (if all eshell-process-list (eshell-interactive-process-p))
- (when (> (float-time (time-since start))
- eshell-test--max-subprocess-time)
- (error "timed out waiting for subprocess(es)"))
- (sit-for 0.1))))
+ (eshell-wait-for
+ (lambda ()
+ (not (if all eshell-process-list (eshell-interactive-process-p))))))
(defun eshell-insert-command (command &optional func)
"Insert a COMMAND at the end of the buffer.
diff --git a/test/lisp/eshell/eshell-tests.el b/test/lisp/eshell/eshell-tests.el
index c67ac67fd36..dd8be8e65f0 100644
--- a/test/lisp/eshell/eshell-tests.el
+++ b/test/lisp/eshell/eshell-tests.el
@@ -128,16 +128,17 @@ eshell-test/forward-arg
(delete-region (point) (point-max))))))
(ert-deftest eshell-test/queue-input ()
- "Test queuing command input"
+ "Test queuing command input.
+This should let the current command finish, then automatically
+insert the queued one at the next prompt, and finally run it."
(with-temp-eshell
- (eshell-insert-command "sleep 2")
- (eshell-insert-command "echo alpha" 'eshell-queue-input)
- (let ((count 10))
- (while (and eshell-current-command
- (> count 0))
- (sit-for 1)
- (setq count (1- count))))
- (should (eshell-match-output "alpha\n"))))
+ (eshell-insert-command "sleep 1; echo slept")
+ (eshell-insert-command "echo alpha" #'eshell-queue-input)
+ (let ((start (marker-position (eshell-beginning-of-output))))
+ (eshell-wait-for (lambda () (not eshell-current-command)))
+ (should (string-match "^slept\n.*echo alpha\nalpha\n$"
+ (buffer-substring-no-properties
+ start (eshell-end-of-output)))))))
(ert-deftest eshell-test/flush-output ()
"Test flushing of previous output"
--
2.25.1
^ permalink raw reply related [flat|nested] 14+ messages in thread
* bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file
2022-12-26 19:50 ` Jim Porter
@ 2022-12-30 6:40 ` Jim Porter
0 siblings, 0 replies; 14+ messages in thread
From: Jim Porter @ 2022-12-30 6:40 UTC (permalink / raw)
To: Eli Zaretskii; +Cc: michael.albinus, 59545-done, milan.zimmermann
On 12/26/2022 11:50 AM, Jim Porter wrote:
> On 12/25/2022 1:49 PM, Jim Porter wrote:
>> I think this should resolve all the issues now, so unless anyone has
>> objections, I'll merge this to the master branch in a few days.
Merged as 073da412a139e317959f56e359ed12de726a0a35 to master. Closing
this again.
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2022-12-30 6:40 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-24 15:49 bug#59545: 29.0.50; Eshell fails to redirect output of sourced eshell file Milan Zimmermann
2022-12-21 0:18 ` Jim Porter
2022-12-21 0:29 ` Jim Porter
2022-12-21 9:54 ` Michael Albinus
2022-12-21 18:48 ` Jim Porter
2022-12-21 19:26 ` Eli Zaretskii
2022-12-22 1:20 ` Jim Porter
2022-12-22 20:02 ` Jim Porter
2022-12-24 7:29 ` Jim Porter
2022-12-25 1:36 ` Jim Porter
2022-12-25 21:49 ` Jim Porter
2022-12-26 19:50 ` Jim Porter
2022-12-30 6:40 ` Jim Porter
2022-12-21 12:01 ` Eli Zaretskii
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.