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

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

[Jim Porter]

In Eshell, you can use conditionals pretty much like you'd expect from 
other shells. Starting from 'emacs -Q -f eshell':

   ~ $ [ foo = foo ] && echo hi
   hi
   ~ $ [ foo = bar ] && echo hi
   ~ $ [ foo = foo ] || echo hi
   ~ $ [ foo = bar ] || echo hi
   hi
   ~ $ if {[ foo = foo ]} {echo yes} {echo no}
   yes
   ~ $ if {[ foo = bar ]} {echo yes} {echo no}
   no

However, that only works for external commands. If the command is 
implemented in Lisp, it doesn't work:

   ~ $ (zerop 0) && echo hi
   t
   hi
   ~ $ (zerop 1) && echo hi
   hi  ;; Shouldn't say hi.

That's because the exit status is always 0 for Lisp commands. This works 
correctly for external commands:

   ~ $ [ foo = foo ]; echo status $? result $$
   ("status" 0 "result" nil)
   ~ $ [ foo = bar ]; echo status $? result $$
   ("status" 1 "result" nil)

But not for Lisp commands:

   ~ $ (zerop 0); echo status $? result $$
   t
   ("status" 0 "result" t)
   ~ $ (zerop 1); echo status $? result $$
   ("status" 0 "result" nil)
   ~ $ (zerop "foo"); echo status $? result $$
   Wrong type argument: number-or-marker-p, "foo"
   ("status" 0 "result" nil)

The manual says that the status should be 1 when a Lisp command fails, 
but it's 0 no matter what, even if it signaled an error. (Likewise, the 
manual says that the "result" variable should be t for successful 
external commands, but it's always nil.)

Similarly to the above, you can't use variable expansions for conditionals:

   ~ $ (setq foo t)
   t
   ~ $ if $foo {echo yes} {echo no}
   yes
   ~ $ (setq foo nil)
   ~ $ if $foo {echo yes} {echo no}
   yes  ;; Should say no.

Patch forthcoming. Just splitting it into two messages since it seemed 
more readable that way...

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

[Jim Porter]

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

Here are some patches to fix this.

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

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

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

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

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

   cat foo.txt && echo hi

And that this doesn't:

   (zerop 1) && echo hi

For people who don't like this behavior, they can customize the new 
'eshell-lisp-form-nil-is-failure' variable.

[-- Attachment #2: 0001-Only-set-Eshell-execution-result-metavariables-when-.patch --]
[-- Type: text/plain, Size: 20211 bytes --]

From 17d15df2368a2b078e90d39cb8018e274e422602 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 6 Aug 2022 13:37:28 -0700
Subject: [PATCH 1/3] Only set Eshell execution result metavariables when
 non-nil

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

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

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

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

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

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

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


[-- Attachment #3: 0002-Allow-using-dollar-expansions-in-Eshell-conditionals.patch --]
[-- Type: text/plain, Size: 8535 bytes --]

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

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

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

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

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


[-- Attachment #4: 0003-Make-and-variables-more-consistent-in-Eshell.patch --]
[-- Type: text/plain, Size: 20218 bytes --]

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

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

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

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

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

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

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

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

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

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

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

> Here are some patches to fix this.

I haven't tested the patches, but reading them, they make sense to me,
so please go ahead and push.

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

[Jim Porter]

On 8/12/2022 8:22 AM, Lars Ingebrigtsen wrote:
> Jim Porter <jporterbugs@gmail.com> writes:
> 
>> Here are some patches to fix this.
> 
> I haven't tested the patches, but reading them, they make sense to me,
> so please go ahead and push.

Thanks for taking a look. Merged as 
f3408af0a3251a744cb0b55b5e153565bfd57ea3.

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

[Eli Zaretskii]

> Cc: 57129@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Fri, 12 Aug 2022 22:14:02 -0700
> 
> On 8/12/2022 8:22 AM, Lars Ingebrigtsen wrote:
> > Jim Porter <jporterbugs@gmail.com> writes:
> > 
> >> Here are some patches to fix this.
> > 
> > I haven't tested the patches, but reading them, they make sense to me,
> > so please go ahead and push.
> 
> Thanks for taking a look. Merged as 
> f3408af0a3251a744cb0b55b5e153565bfd57ea3.

One of the tests in esh-var-tests.el failed on MS-Windows; I fixed it,
although I'm not sure it's a correct fix, because the Eshell manual
seems to say that a built-in implementation of a command should be
preferred by default?

One of the tests in eshell-tests.el also fails on MS-Windows, but I
have no idea how to fix it, nor even what are the details of the
test.  Are the 'echo' commands in that test supposed to be external
shell commands or internal Eshell commands?  If the former, it could
be a problem on MS-Windows.

(This is a good example of my gripes about the test suite, which I
described in bug#57150 yesterday: debugging a failure is unnecessarily
hard and tedious.  There's no information about the specific intent of
the test and its inner workings and assumptions, except the general
idea described in the doc string.)

Here's the failure I get:

  Test eshell-test/subcommand-reset-in-pipeline backtrace:
    signal(ert-test-failed (((should (equal (eshell-test-command-result
    ert-fail(((should (equal (eshell-test-command-result (format templat
    (if (unwind-protect (setq value-38 (apply fn-36 args-37)) (setq form
    (let (form-description-40) (if (unwind-protect (setq value-38 (apply
    (let ((value-38 'ert-form-evaluation-aborted-39)) (let (form-descrip
    (let* ((fn-36 #'equal) (args-37 (condition-case err (let ((signal-ho
    (let ((template (car --dolist-tail--))) (let* ((fn-26 #'equal) (args
    (while --dolist-tail-- (let ((template (car --dolist-tail--))) (let*
    (let ((--dolist-tail-- '("echo {%s} | *cat" "echo ${%s} | *cat" "*ca
    (closure (t) nil (let* ((fn-21 #'executable-find) (args-22 (conditio
    ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
    ert-run-test(#s(ert-test :name eshell-test/subcommand-reset-in-pipel
    ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
    ert-run-tests((not (or (tag :unstable) (tag :nativecomp))) #f(compil
    ert-run-tests-batch((not (or (tag :unstable) (tag :nativecomp))))
    ert-run-tests-batch-and-exit((not (or (tag :unstable) (tag :nativeco
    eval((ert-run-tests-batch-and-exit '(not (or (tag :unstable) (tag :n
    command-line-1(("-L" ";." "-l" "ert" "-l" "lisp/eshell/eshell-tests.
    command-line()
    normal-top-level()
  Test eshell-test/subcommand-reset-in-pipeline condition:
      (ert-test-failed
       ((should
	 (equal
	  (eshell-test-command-result ...)
	  "first"))
	:form
	(equal nil "first")
	:value nil :explanation
	(different-types nil "first")))
     FAILED  18/18  eshell-test/subcommand-reset-in-pipeline (1.873920 sec) at lisp/eshell/eshell-tests.el:80

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

[Jim Porter]

On 8/13/2022 12:01 AM, Eli Zaretskii wrote:
> One of the tests in esh-var-tests.el failed on MS-Windows; I fixed it,
> although I'm not sure it's a correct fix, because the Eshell manual
> seems to say that a built-in implementation of a command should be
> preferred by default?

Sorry about that. I think that's the right fix for that case. Maybe it 
would make sense to set 'eshell-prefer-lisp-functions' to t for most of 
the Eshell tests to improve reproducibility on various platforms; tests 
that want an external command can just put a "*" in front of the command 
name.

> One of the tests in eshell-tests.el also fails on MS-Windows, but I
> have no idea how to fix it, nor even what are the details of the
> test.  Are the 'echo' commands in that test supposed to be external
> shell commands or internal Eshell commands?  If the former, it could
> be a problem on MS-Windows.

The echo commands should be internal Eshell commands (since there's an 
'eshell/echo' Lisp function, that one should always be preferred by Eshell).

I'm surprised that test fails on MS Windows, since it *should* be 
testing internal Eshell logic that's not platform-specific. Based on the 
failure, it looks like one of the following commands is returning the 
wrong value:

   echo {echo $eshell-in-pipeline-p | echo} | *cat
   echo ${echo $eshell-in-pipeline-p | echo} | *cat
   *cat $<echo $eshell-in-pipeline-p | echo> | *cat

All of these should return 'first'. That test is just checking that, 
when you're in a subcommand ({...}, ${...}, or $<...>), the value of 
'eshell-in-pipeline-p' shouldn't be influenced by the pipeline in the 
"super-command". Some built-in Eshell commands consult 
'eshell-in-pipeline-p', and if it had the wrong value, they might do the 
wrong thing.

If nothing else, it would probably be helpful to set up ERT explainers 
so that the error messages are easier to understand. As it is now, 
they're not very explanatory.

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

[Eli Zaretskii]

> Cc: larsi@gnus.org, 57129@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Sat, 13 Aug 2022 11:56:20 -0700
> 
> On 8/13/2022 12:01 AM, Eli Zaretskii wrote:
> > One of the tests in esh-var-tests.el failed on MS-Windows; I fixed it,
> > although I'm not sure it's a correct fix, because the Eshell manual
> > seems to say that a built-in implementation of a command should be
> > preferred by default?
> 
> Sorry about that. I think that's the right fix for that case. Maybe it 
> would make sense to set 'eshell-prefer-lisp-functions' to t for most of 
> the Eshell tests to improve reproducibility on various platforms; tests 
> that want an external command can just put a "*" in front of the command 
> name.

But then this fragment from the Eshell manual:

     The command can be either an Elisp function or an external command.
  Eshell looks first for an alias (*note Aliases::) with the same name as
  the command, then a built-in (*note Built-ins::) or a function with the
  same name; if there is no match, it then tries to execute it as an
  external command.

seems to be inaccurate?  Since 'format' exists as a built-in command,
why did Eshell in this case invoke the external command instead?

> I'm surprised that test fails on MS Windows, since it *should* be 
> testing internal Eshell logic that's not platform-specific. Based on the 
> failure, it looks like one of the following commands is returning the 
> wrong value:
> 
>    echo {echo $eshell-in-pipeline-p | echo} | *cat
>    echo ${echo $eshell-in-pipeline-p | echo} | *cat
>    *cat $<echo $eshell-in-pipeline-p | echo> | *cat
> 
> All of these should return 'first'.

The first two do; the last one returns nothing.

> That test is just checking that, 
> when you're in a subcommand ({...}, ${...}, or $<...>), the value of 
> 'eshell-in-pipeline-p' shouldn't be influenced by the pipeline in the 
> "super-command". Some built-in Eshell commands consult 
> 'eshell-in-pipeline-p', and if it had the wrong value, they might do the 
> wrong thing.

So how to investigate this failure further?

> If nothing else, it would probably be helpful to set up ERT explainers 
> so that the error messages are easier to understand. As it is now, 
> they're not very explanatory.

Indeed, more explanations in this and other tests will be most
welcome.

Thanks.

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

[Jim Porter]

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

On 8/13/2022 10:07 PM, Eli Zaretskii wrote:
>> Cc: larsi@gnus.org, 57129@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Sat, 13 Aug 2022 11:56:20 -0700
>>
>> Sorry about that. I think that's the right fix for that case. Maybe it
>> would make sense to set 'eshell-prefer-lisp-functions' to t for most of
>> the Eshell tests to improve reproducibility on various platforms; tests
>> that want an external command can just put a "*" in front of the command
>> name.
> 
> But then this fragment from the Eshell manual:
> 
>       The command can be either an Elisp function or an external command.
>    Eshell looks first for an alias (*note Aliases::) with the same name as
>    the command, then a built-in (*note Built-ins::) or a function with the
>    same name; if there is no match, it then tries to execute it as an
>    external command.
> 
> seems to be inaccurate?  Since 'format' exists as a built-in command,
> why did Eshell in this case invoke the external command instead?

"Built-in" in that part of the manual refers to a function named 
'eshell/FOO'. If you run "FOO" as an Eshell command, it will check for 
'eshell/FOO' before an external command on your system. The manual could 
probably stand to be improved here.

>> I'm surprised that test fails on MS Windows, since it *should* be
>> testing internal Eshell logic that's not platform-specific. Based on the
>> failure, it looks like one of the following commands is returning the
>> wrong value:
>>
>>     echo {echo $eshell-in-pipeline-p | echo} | *cat
>>     echo ${echo $eshell-in-pipeline-p | echo} | *cat
>>     *cat $<echo $eshell-in-pipeline-p | echo> | *cat
>>
>> All of these should return 'first'.
> 
> The first two do; the last one returns nothing.
[snip]
> So how to investigate this failure further?

I have access to an MS Windows system (but don't have a build 
environment set up for native MS Windows builds), so I can try to 
reproduce this using the Emacs 29 pretest binaries in the next few days. 
Hopefully that works.

If you'd like to dig into this further yourself, you could try running 
this command in Eshell:

   eshell-parse-command '*cat $<echo $eshell-in-pipeline-p | echo> | *cat'

That will print the Lisp form that the command gets converted to. I've 
attached the result that I get in a recent Emacs 29 build on GNU/Linux. 
The two functions that set 'eshell-in-pipeline-p' in there are:

* 'eshell-as-subcommand', which resets it to nil so that the outer
   command's pipeline state doesn't interfere with the subcommand.

* 'eshell-execute-pipeline', which calls 'eshell-do-pipelines' (except
   on MS-DOS, I think) and should set 'eshell-in-pipeline-p' to 'first'
   in the above case.

>> If nothing else, it would probably be helpful to set up ERT explainers
>> so that the error messages are easier to understand. As it is now,
>> they're not very explanatory.
> 
> Indeed, more explanations in this and other tests will be most
> welcome.

Yeah, this test in particular is high up on the list of tests that needs 
more explanation. It's pretty subtle, and the test doesn't really serve 
as a good example of why someone would care that this behavior works the 
way the test demands it.

[-- Attachment #2: parsed-command.el --]
[-- Type: text/plain, Size: 1563 bytes --]

(eshell-trap-errors
 (eshell-execute-pipeline
  '((eshell-named-command
     (eshell-concat nil "*" "cat")
     (list
      (eshell-escape-arg
       (let
           ((indices
             (eshell-eval-indices 'nil)))
         (let
             ((eshell-current-handles
               (eshell-create-handles "/tmp/QqPFwo" 'overwrite)))
           (progn
             (eshell-as-subcommand
              (eshell-trap-errors
               (eshell-execute-pipeline
                '((eshell-named-command "echo"
                                        (list
                                         (eshell-escape-arg
                                          (let
                                              ((indices
                                                (eshell-eval-indices 'nil)))
                                            (eshell-get-variable
                                             #("eshell-in-pipeline-p" 0 20
                                               (escaped t))
                                             indices nil)))))
                  (progn
                    (ignore
                     (eshell-set-output-handle 1 'overwrite "/tmp/QqPFwo"))
                    (eshell-named-command "echo"))))))
             (ignore
              (nconc eshell-this-command-hook
                     (list
                      #'(lambda nil
                          (delete-file "/tmp/QqPFwo")))))
             (eshell-apply-indices "/tmp/QqPFwo" indices nil)))))))
    (eshell-named-command
     (eshell-concat nil "*" "cat")))))

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

[Eli Zaretskii]

> Cc: larsi@gnus.org, 57129@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Sat, 13 Aug 2022 22:37:12 -0700
> 
> > But then this fragment from the Eshell manual:
> > 
> >       The command can be either an Elisp function or an external command.
> >    Eshell looks first for an alias (*note Aliases::) with the same name as
> >    the command, then a built-in (*note Built-ins::) or a function with the
> >    same name; if there is no match, it then tries to execute it as an
> >    external command.
> > 
> > seems to be inaccurate?  Since 'format' exists as a built-in command,
> > why did Eshell in this case invoke the external command instead?
> 
> "Built-in" in that part of the manual refers to a function named 
> 'eshell/FOO'. If you run "FOO" as an Eshell command, it will check for 
> 'eshell/FOO' before an external command on your system. The manual could 
> probably stand to be improved here.

The manual definitely should be clarified in that matter, because:

  d:/gnu/git/emacs/trunk/src $ which format
  format is a built-in function in ‘C source code’.

To me this says that 'format' is a built-in command, and the manual
says such commands should be invoked in preference to external
commands.  The "eshell/" part is not mentioned anywhere.

> If you'd like to dig into this further yourself, you could try running 
> this command in Eshell:
> 
>    eshell-parse-command '*cat $<echo $eshell-in-pipeline-p | echo> | *cat'
> 
> That will print the Lisp form that the command gets converted to. I've 
> attached the result that I get in a recent Emacs 29 build on GNU/Linux. 

I get the same output, with 2 exceptions:

  . the name of the temporary file is different
  . instead of a simple string here:

                     (eshell-set-output-handle 1 'overwrite "/tmp/QqPFwo"))

    I get a file name split into several parts, which are then
    concatenated by eshell-concat:

		     (eshell-set-output-handle 1 'overwrite
					       (eshell-extended-glob
						(eshell-concat nil "c:/DOCUME" "~" "1/Zaretzky/LOCALS" "~" "1/Temp/OIi8Wd")))

Any chance that the "~" parts here somehow get in the way?

If not, any other thoughts?  My main worry is that there's something
here specific to how we invoke subprocesses, which you lately changed.

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

[Jim Porter]

On 8/14/2022 12:30 AM, Eli Zaretskii wrote:
> The manual definitely should be clarified in that matter, because:
> 
>    d:/gnu/git/emacs/trunk/src $ which format
>    format is a built-in function in ‘C source code’.
> 
> To me this says that 'format' is a built-in command, and the manual
> says such commands should be invoked in preference to external
> commands.  The "eshell/" part is not mentioned anywhere.

I'll work on a fix for that. This is a little more complex than 
immediately apparent though, since Eshell also unfortunately uses the 
term "alias" to refer to two kinds of entities:

1) Command abbreviations created by the user using the 'alias' command,
    e.g. "alias ll 'ls -l'". These are sometimes called "command
    aliases", and are implemented in em-alias.el.

2) Lisp functions with names like 'eshell/FOO'. These are sometimes
    called "alias functions", and are implemented in esh-cmd.el. For
    example, see 'eshell-find-alias-function', which checks if
    'eshell/FOO' exists.

It would probably be good to fix all this terminology for once and for all.

> I get the same output, with 2 exceptions:
> 
>    . the name of the temporary file is different
>    . instead of a simple string here:
> 
>                       (eshell-set-output-handle 1 'overwrite "/tmp/QqPFwo"))
> 
>      I get a file name split into several parts, which are then
>      concatenated by eshell-concat:
> 
> 		     (eshell-set-output-handle 1 'overwrite
> 					       (eshell-extended-glob
> 						(eshell-concat nil "c:/DOCUME" "~" "1/Zaretzky/LOCALS" "~" "1/Temp/OIi8Wd")))
> 
> Any chance that the "~" parts here somehow get in the way?

It's possible. I think Eshell is trying to ensure that the "~" doesn't 
get parsed as "home directory" here. But it could be doing something 
wrong there. I did also touch 'eshell-concat' not too long ago, so I 
might have broken something there.

> If not, any other thoughts?  My main worry is that there's something
> here specific to how we invoke subprocesses, which you lately changed.

You could try some Eshell commands that don't use subprocesses to see if 
that changes anything. For example, these should only use Lisp functions:

   ;; Should print "first" in the Eshell buffer:
   eshell/echo ${eshell/echo $eshell-in-pipeline-p | eshell/echo}
   eshell/echo ${eshell/echo $eshell-in-pipeline-p | eshell/echo} | 
eshell/echo

   ;; Should open a temp file containing "first" in a new buffer:
   find-file $<eshell/echo $eshell-in-pipeline-p | eshell/echo>
   find-file $<eshell/echo $eshell-in-pipeline-p | eshell/echo> | 
eshell/echo

The "eshell/" prefix probably isn't necessary, but I figured it's worth 
being extra-careful here while diagnosing the issue. I'm particularly 
interested in whether the third and fourth commands work. If they work, 
that would definitely point towards a bug in Eshell's subprocess 
handling; if not, then that would point to a bug in the "$<subcommand>" 
handling.

I also filed bug#57216 to add ERT explainers that will print better 
error messages for most of the Eshell tests. That doesn't address the 
sometimes-vague documentation of the tests, but I'll do that separately.

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

[Eli Zaretskii]

> Cc: larsi@gnus.org, 57129@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Sun, 14 Aug 2022 14:40:06 -0700
> 
> On 8/14/2022 12:30 AM, Eli Zaretskii wrote:
> > The manual definitely should be clarified in that matter, because:
> > 
> >    d:/gnu/git/emacs/trunk/src $ which format
> >    format is a built-in function in ‘C source code’.
> > 
> > To me this says that 'format' is a built-in command, and the manual
> > says such commands should be invoked in preference to external
> > commands.  The "eshell/" part is not mentioned anywhere.
> 
> I'll work on a fix for that. This is a little more complex than 
> immediately apparent though, since Eshell also unfortunately uses the 
> term "alias" to refer to two kinds of entities:
> 
> 1) Command abbreviations created by the user using the 'alias' command,
>     e.g. "alias ll 'ls -l'". These are sometimes called "command
>     aliases", and are implemented in em-alias.el.
> 
> 2) Lisp functions with names like 'eshell/FOO'. These are sometimes
>     called "alias functions", and are implemented in esh-cmd.el. For
>     example, see 'eshell-find-alias-function', which checks if
>     'eshell/FOO' exists.
> 
> It would probably be good to fix all this terminology for once and for all.

Yes, that could well be a source of a lot of confusion.

> You could try some Eshell commands that don't use subprocesses to see if 
> that changes anything. For example, these should only use Lisp functions:
> 
>    ;; Should print "first" in the Eshell buffer:
>    eshell/echo ${eshell/echo $eshell-in-pipeline-p | eshell/echo}
>    eshell/echo ${eshell/echo $eshell-in-pipeline-p | eshell/echo} | 
> eshell/echo
> 
>    ;; Should open a temp file containing "first" in a new buffer:
>    find-file $<eshell/echo $eshell-in-pipeline-p | eshell/echo>
>    find-file $<eshell/echo $eshell-in-pipeline-p | eshell/echo> | 
> eshell/echo
> 
> The "eshell/" prefix probably isn't necessary, but I figured it's worth 
> being extra-careful here while diagnosing the issue. I'm particularly 
> interested in whether the third and fourth commands work. If they work, 
> that would definitely point towards a bug in Eshell's subprocess 
> handling; if not, then that would point to a bug in the "$<subcommand>" 
> handling.

All the 4 commands do what you say they should do.

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

[Jim Porter]

On 8/15/2022 5:13 AM, Eli Zaretskii wrote:
>> Cc: larsi@gnus.org, 57129@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Sun, 14 Aug 2022 14:40:06 -0700
>>
>> You could try some Eshell commands that don't use subprocesses to see if
>> that changes anything. For example, these should only use Lisp functions:
>>
>>     ;; Should print "first" in the Eshell buffer:
>>     eshell/echo ${eshell/echo $eshell-in-pipeline-p | eshell/echo}
>>     eshell/echo ${eshell/echo $eshell-in-pipeline-p | eshell/echo} |
>> eshell/echo
>>
>>     ;; Should open a temp file containing "first" in a new buffer:
>>     find-file $<eshell/echo $eshell-in-pipeline-p | eshell/echo>
>>     find-file $<eshell/echo $eshell-in-pipeline-p | eshell/echo> |
>> eshell/echo
>>
>> The "eshell/" prefix probably isn't necessary, but I figured it's worth
>> being extra-careful here while diagnosing the issue. I'm particularly
>> interested in whether the third and fourth commands work. If they work,
>> that would definitely point towards a bug in Eshell's subprocess
>> handling; if not, then that would point to a bug in the "$<subcommand>"
>> handling.
> 
> All the 4 commands do what you say they should do.

Hm, and I can't reproduce this on the prebuilt Emacs 29 snapshot from 
July 18, which is from just before I merged the changes to 
'make-process', so it's definitely possible that this is due to those 
commits.

Does reverting 4e59830bc0ab17cdbd85748b133c97837bed99e3 and 
d7b89ea4077d4fe677ba0577245328819ee79cdc fix the issue? I checked 
locally, and they should revert cleanly at least.

It might also be interesting to revert just the changes from d7b89ea in 
lisp/eshell/esh-proc.el. I don't think any of the changes I made to the 
C sources should impact this, since a) I was careful to keep the logic 
as close to before as I could, and b) it just changes the logic for 
creating a PTY, which doesn't work on MS Windows anyway.

I did notice one weird issue though, and maybe this has something to do 
with the problem: on MS Windows, if I run any command with a 
$<subcommand>, the second and subsequent times, it warns me about 
changing the temp file, e.g.:

   3Zz80A has changed since visited or saved.  Save anyway? (yes or no)

This probably shouldn't do that, and it could definitely cause issues in 
the tests, which can't prompt the user for things like that.

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

[Eli Zaretskii]

> Cc: larsi@gnus.org, 57129@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Mon, 15 Aug 2022 09:14:04 -0700
> 
> > All the 4 commands do what you say they should do.
> 
> Hm, and I can't reproduce this on the prebuilt Emacs 29 snapshot from 
> July 18, which is from just before I merged the changes to 
> 'make-process', so it's definitely possible that this is due to those 
> commits.
> 
> Does reverting 4e59830bc0ab17cdbd85748b133c97837bed99e3 and 
> d7b89ea4077d4fe677ba0577245328819ee79cdc fix the issue? I checked 
> locally, and they should revert cleanly at least.
> 
> It might also be interesting to revert just the changes from d7b89ea in 
> lisp/eshell/esh-proc.el. I don't think any of the changes I made to the 
> C sources should impact this, since a) I was careful to keep the logic 
> as close to before as I could, and b) it just changes the logic for 
> creating a PTY, which doesn't work on MS Windows anyway.

None of these reverts fixes the issue.  I tried both the tests in
eshell-tests.el and the problematic command:

   *cat $<echo $eshell-in-pipeline-p | echo> | *cat

> I did notice one weird issue though, and maybe this has something to do 
> with the problem: on MS Windows, if I run any command with a 
> $<subcommand>, the second and subsequent times, it warns me about 
> changing the temp file, e.g.:
> 
>    3Zz80A has changed since visited or saved.  Save anyway? (yes or no)
> 
> This probably shouldn't do that, and it could definitely cause issues in 
> the tests, which can't prompt the user for things like that.

I guess this means you somehow reuse the same temporary file on
MS-Windows?  Maybe change the test a bit so that the file name
definitely differs?

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

[Jim Porter]

On 8/15/2022 9:49 AM, Eli Zaretskii wrote:
> None of these reverts fixes the issue.  I tried both the tests in
> eshell-tests.el and the problematic command:
> 
>     *cat $<echo $eshell-in-pipeline-p | echo> | *cat

Thanks for testing. I might have to get an MS-Windows dev environment 
set up to dig into this further. I'm a bit worried that this is related 
to bug#55590, where I had to jump through some awkward hoops to fix a 
somewhat-related issue with Eshell subcommands. I think that's due to 
'eshell-do-eval' being written before lexical binding existed, and so 
the variable scoping doesn't work quite as expected. It could be that 
you only see this failure when using external commands due to some 
differences in timing.

If that's the problem here too, it would be a pain to fix (but it would 
likely also mean that this bug has been present for a long time, and my 
tests/fixes have just brought it to the surface). As mentioned in 
bug#55590, I think it'd be nice to rip out 'eshell-do-eval' and replace 
it with the generator.el machinery, since they're conceptually pretty 
similar. That's easier said than done though...

>> I did notice one weird issue though, and maybe this has something to do
>> with the problem: on MS Windows, if I run any command with a
>> $<subcommand>, the second and subsequent times, it warns me about
>> changing the temp file, e.g.:
>>
>>     3Zz80A has changed since visited or saved.  Save anyway? (yes or no)
>>
>> This probably shouldn't do that, and it could definitely cause issues in
>> the tests, which can't prompt the user for things like that.
> 
> I guess this means you somehow reuse the same temporary file on
> MS-Windows?  Maybe change the test a bit so that the file name
> definitely differs?

I need to investigate this a bit further, since on GNU/Linux, I get a 
new temp file every time. I think that's the behavior we want, or 
failing that, we could at least kill the temp file's buffer after we're 
done with it. I don't think there's much value in leaving it around.

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

[Eli Zaretskii]

> Cc: larsi@gnus.org, 57129@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Mon, 15 Aug 2022 11:08:45 -0700
> 
> >> I did notice one weird issue though, and maybe this has something to do
> >> with the problem: on MS Windows, if I run any command with a
> >> $<subcommand>, the second and subsequent times, it warns me about
> >> changing the temp file, e.g.:
> >>
> >>     3Zz80A has changed since visited or saved.  Save anyway? (yes or no)
> >>
> >> This probably shouldn't do that, and it could definitely cause issues in
> >> the tests, which can't prompt the user for things like that.
> > 
> > I guess this means you somehow reuse the same temporary file on
> > MS-Windows?  Maybe change the test a bit so that the file name
> > definitely differs?
> 
> I need to investigate this a bit further, since on GNU/Linux, I get a 
> new temp file every time.

Can you tell how you create these temporary files?  Or point me to the
code which does that?

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

[Jim Porter]

On 8/15/2022 11:21 AM, Eli Zaretskii wrote:
>> Cc: larsi@gnus.org, 57129@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Mon, 15 Aug 2022 11:08:45 -0700
>>
>> I need to investigate this a bit further, since on GNU/Linux, I get a
>> new temp file every time.
> 
> Can you tell how you create these temporary files?  Or point me to the
> code which does that?

The temp files are created by Eshell in lisp/eshell/esh-var.el in the 
function 'eshell-parse-variable-ref', specifically in the part starting 
with:

   (eq (char-after) ?\<)

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

[Eli Zaretskii]

> Cc: larsi@gnus.org, 57129@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Mon, 15 Aug 2022 11:30:07 -0700
> 
> >> I need to investigate this a bit further, since on GNU/Linux, I get a
> >> new temp file every time.
> > 
> > Can you tell how you create these temporary files?  Or point me to the
> > code which does that?
> 
> The temp files are created by Eshell in lisp/eshell/esh-var.el in the 
> function 'eshell-parse-variable-ref', specifically in the part starting 
> with:
> 
>    (eq (char-after) ?\<)

Ah, okay.  It's a (mis)feature of Gnulib's gen_tempname function
(which is the guts of make-temp-file) in its implementation for
MS-Windows (and maybe other platforms?): it always begins from the
same "random" characters in the file name, and only generates other
random characters if there's already a file by that name.  So if you
are careful and delete the temporary file each time after usage, and
never need more than one temporary file at the same time, you will get
the same name every call.

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

[Paul Eggert]

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

On 8/15/22 11:58, Eli Zaretskii wrote:

> Ah, okay.  It's a (mis)feature of Gnulib's gen_tempname function
> (which is the guts of make-temp-file) in its implementation for
> MS-Windows (and maybe other platforms?): it always begins from the
> same "random" characters in the file name, and only generates other
> random characters if there's already a file by that name.

Not sure I'd call it a misfeature, as gen_tempname is generating a 
uniquely-named file that is exclusive to Emacs, which is all it's 
supposed to do.

I do see a comment saying that gen_tempname generates "hard-to-predict" 
names, which as you note is not correct on MS-DOS, nor even strictly 
speaking on all POSIX platforms. I installed the first attached patch 
into Gnulib to fix that comment.

I'm not sure I'm entirely understanding the Emacs problem, but it 
appears to be that Emacs has its own set of filenames that it thinks it 
knows about, and Emacs wants the new temporary file's name to not be a 
member of that set. If I'm right, does the second attached patch (this 
patch is to Emacs) address the problem? I haven't tested or installed it.

[-- Attachment #2: 0001-tempname-remove-incorrect-comment.patch --]
[-- Type: text/x-patch, Size: 1912 bytes --]

From a7ef21bf347da5e3a8b5492b849d633ef3252d62 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 15 Aug 2022 13:04:08 -0700
Subject: [PATCH] tempname: remove incorrect comment

* lib/tempname.c, lib/tempname.h: Remove incorrect comment,
as the names are not necessarily hard to predict (Bug#57129).
---
 ChangeLog      | 6 ++++++
 lib/tempname.c | 2 +-
 lib/tempname.h | 2 +-
 3 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ba16b538f5..1e20db7e37 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,9 @@
+2022-08-15  Paul Eggert  <eggert@cs.ucla.edu>
+
+	tempname: remove incorrect comment
+	* lib/tempname.c, lib/tempname.h: Remove incorrect comment,
+	as the names are not necessarily hard to predict (Bug#57129).
+
 2022-08-14  Simon Josefsson  <simon@josefsson.org>
 
 	bootstrap.conf: Use proper shell marker for Emacs.
diff --git a/lib/tempname.c b/lib/tempname.c
index 5fc5efe031..75a939e571 100644
--- a/lib/tempname.c
+++ b/lib/tempname.c
@@ -213,7 +213,7 @@ static const char letters[] =
                         and return a read-write fd.  The file is mode 0600.
    __GT_DIR:            create a directory, which will be mode 0700.
 
-   We use a clever algorithm to get hard-to-predict names. */
+   */
 #ifdef _LIBC
 static
 #endif
diff --git a/lib/tempname.h b/lib/tempname.h
index c172820f7f..5e3c5e1550 100644
--- a/lib/tempname.h
+++ b/lib/tempname.h
@@ -48,7 +48,7 @@ extern "C" {
                         and return a read-write fd.  The file is mode 0600.
    GT_DIR:              create a directory, which will be mode 0700.
 
-   We use a clever algorithm to get hard-to-predict names. */
+   */
 extern int gen_tempname (char *tmpl, int suffixlen, int flags, int kind);
 /* Similar, except X_SUFFIX_LEN gives the number of Xs.  */
 extern int gen_tempname_len (char *tmpl, int suffixlen, int flags, int kind,
-- 
2.34.1


[-- Attachment #3: 0001-Don-t-create-temp-file-with-same-name-as-visited.patch --]
[-- Type: text/x-patch, Size: 2910 bytes --]

From a0776984bc3630c8c0bd57b3fd1acca1f0b8cb5c Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 15 Aug 2022 12:55:40 -0700
Subject: [PATCH] =?UTF-8?q?Don=E2=80=99t=20create=20temp=20file=20with=20s?=
 =?UTF-8?q?ame=20name=20as=20visited?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Problem reported by Jim Porter (Bug#57129#47).
* src/fileio.c (create_tempname): New function.
(Fmake_temp_file_internal): Use it.
---
 src/fileio.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)

diff --git a/src/fileio.c b/src/fileio.c
index 9697f6c8cf..4f134bbfa9 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -677,6 +677,27 @@ DEFUN ("directory-file-name", Fdirectory_file_name, Sdirectory_file_name,
   return val;
 }
 
+/* Create a temporary file whose encoded name is ENCODED_FILENAME, and
+   whose decoded name is *(Lisp_Object *) FLAGS.  Pretend the file
+   already exists if a live buffer is visiting it.  Return a file
+   descriptor if successful, -1 (setting errno) otherwise.  */
+static int
+create_tempname (char *encoded_filename, void *flags)
+{
+  Lisp_Object *addval = flags;
+  Lisp_Object filename = *addval;
+
+  if (!NILP (Fget_file_buffer (filename)))
+    {
+      errno = EEXIST;
+      return -1;
+    }
+
+  return open (encoded_filename,
+	       O_RDWR | O_BINARY | O_CLOEXEC | O_CREAT | O_EXCL,
+	       S_IRUSR | S_IWUSR);
+}
+
 DEFUN ("make-temp-file-internal", Fmake_temp_file_internal,
        Smake_temp_file_internal, 4, 4, 0,
        doc: /* Generate a new file whose name starts with PREFIX, a string.
@@ -707,18 +728,18 @@ DEFUN ("make-temp-file-internal", Fmake_temp_file_internal,
   memcpy (data, SSDATA (encoded_prefix), prefix_len);
   memset (data + prefix_len, 'X', nX);
   memcpy (data + prefix_len + nX, SSDATA (encoded_suffix), suffix_len);
+  Lisp_Object dval = DECODE_FILE (val), ddval = dval;
   int kind = (NILP (dir_flag) ? GT_FILE
 	      : BASE_EQ (dir_flag, make_fixnum (0)) ? GT_NOCREATE
 	      : GT_DIR);
-  int fd = gen_tempname (data, suffix_len, O_BINARY | O_CLOEXEC, kind);
+  int fd = try_tempname (data, suffix_len, &ddval, create_tempname);
   bool failed = fd < 0;
   if (!failed)
     {
       specpdl_ref count = SPECPDL_INDEX ();
       record_unwind_protect_int (close_file_unwind, fd);
-      val = DECODE_FILE (val);
       if (STRINGP (text) && SBYTES (text) != 0)
-	write_region (text, Qnil, val, Qnil, Qnil, Qnil, Qnil, fd);
+	write_region (text, Qnil, dval, Qnil, Qnil, Qnil, Qnil, fd);
       failed = NILP (dir_flag) && emacs_close (fd) != 0;
       /* Discard the unwind protect.  */
       specpdl_ptr = specpdl_ref_to_ptr (count);
@@ -733,7 +754,7 @@ DEFUN ("make-temp-file-internal", Fmake_temp_file_internal,
 	};
       report_file_error (kind_message[kind], prefix);
     }
-  return val;
+  return dval;
 }
 
 
-- 
2.37.1

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

[Bruno Haible]

Paul Eggert wrote:
> I do see a comment saying that gen_tempname generates "hard-to-predict" 
> names, which as you note is not correct on MS-DOS, nor even strictly 
> speaking on all POSIX platforms. I installed the first attached patch 
> into Gnulib to fix that comment.

Another comment fix is as below. Note that both comment fixes need to be
propagated to glibc.


2022-08-15  Bruno Haible  <bruno@clisp.org>

	tempname: Fix a comment.
	* lib/tempname.c (try_tempname_len): Use of entropy makes the function
	more, not less, secure.

diff --git a/lib/tempname.c b/lib/tempname.c
index 75a939e571..e6520191d7 100644
--- a/lib/tempname.c
+++ b/lib/tempname.c
@@ -273,7 +273,7 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
   /* Whether to consume entropy when acquiring random bits.  On the
      first try it's worth the entropy cost with __GT_NOCREATE, which
      is inherently insecure and can use the entropy to make it a bit
-     less secure.  On the (rare) second and later attempts it might
+     more secure.  On the (rare) second and later attempts it might
      help against DoS attacks.  */
   bool use_getrandom = tryfunc == try_nocreate;
 

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

[Eli Zaretskii]

> Date: Mon, 15 Aug 2022 13:55:35 -0700
> Cc: larsi@gnus.org, 57129@debbugs.gnu.org, Jim Porter
>  <jporterbugs@gmail.com>, Gnulib bugs <bug-gnulib@gnu.org>
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 8/15/22 11:58, Eli Zaretskii wrote:
> 
> > Ah, okay.  It's a (mis)feature of Gnulib's gen_tempname function
> > (which is the guts of make-temp-file) in its implementation for
> > MS-Windows (and maybe other platforms?): it always begins from the
> > same "random" characters in the file name, and only generates other
> > random characters if there's already a file by that name.
> 
> Not sure I'd call it a misfeature

I didn't say "misfeature".  Evidently, for you and perhaps others it's
a feature.

> as gen_tempname is generating a uniquely-named file that is
> exclusive to Emacs, which is all it's supposed to do.

Then perhaps calling it from make-temp-file in Emacs is a mistake,
because that function's doc string says

  Create a temporary file.
  The returned file name (created by appending some random characters at the end
  of PREFIX, and expanding against ‘temporary-file-directory’ if necessary),
  is guaranteed to point to a newly created file.

When you tell someone that the file name will include "some random
characters", they don't expect that they will be the same "random
characters" every call, just like saying that some function generates
random numbers doesn't imply it will be the same number every call.

> I'm not sure I'm entirely understanding the Emacs problem, but it 
> appears to be that Emacs has its own set of filenames that it thinks it 
> knows about, and Emacs wants the new temporary file's name to not be a 
> member of that set. If I'm right, does the second attached patch (this 
> patch is to Emacs) address the problem? I haven't tested or installed it.

I don't think that's the problem, no.  The problem is that callers of
make-temp-file reasonably expect it to return a new name every call.
And evidently, it does that on GNU/Linux (not sure how, given that the
tempname.c code is supposed to be in glibc?).  So if there's no
intention to change gen_tempname on non-glibc platforms so that it
doesn't repeat the same "random characters" upon each call, I think
Emacs should call a different function to generate "random" names of
temporary files, for consistency across platforms if not for other
reasons.

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

[Bruno Haible]

Eli Zaretskii wrote:
> The problem is that callers of
> make-temp-file reasonably expect it to return a new name every call.
> And evidently, it does that on GNU/Linux (not sure how, given that the
> tempname.c code is supposed to be in glibc?).  So if there's no
> intention to change gen_tempname on non-glibc platforms so that it
> doesn't repeat the same "random characters" upon each call, I think
> Emacs should call a different function to generate "random" names of
> temporary files, for consistency across platforms if not for other
> reasons.

You are making incorrect assumptions or hypotheses. I am adding the unit
test below. It proves that (at least) on Linux, FreeBSD 11, NetBSD 7,
OpenBSD 6.0, macOS, AIX 7.1, Solaris 10, Cygwin, and native Windows,
gen_tempname *does* return a new file name on each call, with a very high
probability.

So, gen_tempname does *not* repeat the same "random characters" upon each call.


2022-08-16  Bruno Haible  <bruno@clisp.org>

	tempname: Add tests.
	* tests/test-tempname.c: New file.
	* modules/tempname-tests: New file.

=========================== tests/test-tempname.c ==========================
/* Test of creating a temporary file.
   Copyright (C) 2022 Free Software Foundation, Inc.

   This program is free software: you can redistribute it and/or modify
   it under the terms of the GNU General Public License as published by
   the Free Software Foundation, either version 3 of the License, or
   (at your option) any later version.

   This program is distributed in the hope that it will be useful,
   but WITHOUT ANY WARRANTY; without even the implied warranty of
   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
   GNU General Public License for more details.

   You should have received a copy of the GNU General Public License
   along with this program.  If not, see <https://www.gnu.org/licenses/>.  */

/* Written by Bruno Haible <bruno@clisp.org>, 2022.  */

#include <config.h>

#include "tempname.h"

#include <string.h>
#include <unistd.h>

#include "macros.h"

int
main ()
{
  /* Verify that two consecutive calls to gen_tempname return two different
     file names, with a high probability.  */
  const char *templ18 = "gl-temp-XXXXXX.xyz";
  char filename1[18 + 1];
  char filename2[18 + 1];

  strcpy (filename1, templ18);
  int fd1 = gen_tempname (filename1, strlen (".xyz"), 0, GT_FILE);
  ASSERT (fd1 >= 0);

  strcpy (filename2, templ18);
  int fd2 = gen_tempname (filename2, strlen (".xyz"), 0, GT_FILE);
  ASSERT (fd2 >= 0);

  /* With 6 'X' and a good pseudo-random number generator behind the scenes,
     the probability of getting the same file name twice in a row should be
     1/62^6 < 1/10^10.  */
  ASSERT (strcmp (filename1, filename2) != 0);

  /* Clean up.  */
  close (fd1);
  close (fd2);
  unlink (filename1);
  unlink (filename2);

  return 0;
}
=========================== modules/tempname-tests =========================
Files:
tests/test-tempname.c
tests/macros.h

Depends-on:
unlink

configure.ac:

Makefile.am:
TESTS += test-tempname
check_PROGRAMS += test-tempname
test_tempname_LDADD = $(LDADD) $(LIB_GETRANDOM) $(LIB_CLOCK_GETTIME)
============================================================================

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

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com
> Date: Tue, 16 Aug 2022 15:35:17 +0200
> 
> Eli Zaretskii wrote:
> > The problem is that callers of
> > make-temp-file reasonably expect it to return a new name every call.
> > And evidently, it does that on GNU/Linux (not sure how, given that the
> > tempname.c code is supposed to be in glibc?).  So if there's no
> > intention to change gen_tempname on non-glibc platforms so that it
> > doesn't repeat the same "random characters" upon each call, I think
> > Emacs should call a different function to generate "random" names of
> > temporary files, for consistency across platforms if not for other
> > reasons.
> 
> You are making incorrect assumptions or hypotheses.

They are neither assumptions nor hypotheses.  They are what I actually
saw by stepping with a debugger into the code.

> I am adding the unit test below. It proves that (at least) on Linux,
> FreeBSD 11, NetBSD 7, OpenBSD 6.0, macOS, AIX 7.1, Solaris 10,
> Cygwin, and native Windows, gen_tempname *does* return a new file
> name on each call, with a very high probability.
> 
> So, gen_tempname does *not* repeat the same "random characters" upon each call.

Well, it does here, when called from Emacs in the particular scenario
of the unit test I was looking into.

Looking at your test program, I see that you generate the seconds file
name without deleting the first one.  When a file by the first
generated name already exists, gen_tempname will indeed generate a
different name.  But in the scenario I described, Emacs creates one
temporary file, uses it, then deletes it, and only after that creates
another file.  In terms of your test program, you need to move the
first unlink call to before the second call to gen_tempname.  Then
your test program will faithfully reproduce what Emacs does in the
case in point.

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

[Bruno Haible]

Eli Zaretskii wrote:
> Looking at your test program, I see that you generate the seconds file
> name without deleting the first one.  When a file by the first
> generated name already exists, gen_tempname will indeed generate a
> different name.  But in the scenario I described, Emacs creates one
> temporary file, uses it, then deletes it, and only after that creates
> another file.

Why would it be important for the second temporary file to bear a different
name, once the first one is gone? I mean, process IDs get reused over time;
socket numbers get reused over time; what is wrong with reusing a file name,
once the older file is gone?

Maybe the problem is that the file gets deleted too early, when some parts
of Emacs still reference it?

Bruno

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

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: eggert@cs.ucla.edu, bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com
> Date: Tue, 16 Aug 2022 16:40:16 +0200
> 
> Eli Zaretskii wrote:
> > Looking at your test program, I see that you generate the seconds file
> > name without deleting the first one.  When a file by the first
> > generated name already exists, gen_tempname will indeed generate a
> > different name.  But in the scenario I described, Emacs creates one
> > temporary file, uses it, then deletes it, and only after that creates
> > another file.
> 
> Why would it be important for the second temporary file to bear a different
> name, once the first one is gone? I mean, process IDs get reused over time;
> socket numbers get reused over time; what is wrong with reusing a file name,
> once the older file is gone?

Because the Emacs Lisp program expected that, based on what
make-temp-file in Emacs promises (see the "random characters" part),
and because that's how it behaves on GNU/Linux.  Then the same program
behaved differently on MS-Windows, and the programmer was stumped.

Sure, it's possible to write the same program somewhat differently,
carefully working around this issue, but my point is that such
functions should ideally present the same behavior traits on all
systems, otherwise the portability is hampered.

> Maybe the problem is that the file gets deleted too early, when some parts
> of Emacs still reference it?

The buffer which visited that file still exists, and still records the
name of the file, yes.  And then, when the program writes to another
file created by another call to make-temp-file, it actually writes to
a file that some buffer still visits, and in Emacs that triggers a
prompt to the user to refresh the buffer.  The programmer didn't
expect that because it is natural to expect each call to a function
that creates a temporary file to create a file under a new name, not
reuse the same name.  E.g., similar facilities in the Standard C
library exhibit that behavior.  So the program was written assuming
that the second write was to an entirely different and unrelated file.

And again, my main point is: why does this work differently on
different platforms?  If you consider it OK to return the same file
name each time, provided that no such file exists, then why doesn't
that function do it on GNU/Linux?

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

[Paul Eggert]

On 8/16/22 09:25, Eli Zaretskii wrote:
> The programmer didn't
> expect that because it is natural to expect each call to a function
> that creates a temporary file to create a file under a new name, not
> reuse the same name.

Regardless of whether the programmer expects a random name or a 
predictable-but-unique name, there are only a finite number of names to 
choose from so the programmer must take into account the possibility 
that the chosen name clashes with names that the programmer would prefer 
not to be chosen.

This means an Emacs patch such as [1] is needed regardless of whether 
Gnulib's gen_tempname is changed to be more random than it is. Although 
it's true that the bug fixed by [1] is less likely if gen_tempname 
generates more-random names, the bug can occur no matter what we do 
about gen_tempname.

[1] 
https://debbugs.gnu.org/cgi/bugreport.cgi?filename=0001-Don-t-create-temp-file-with-same-name-as-visited.patch;bug=57129;msg=59;att=2

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

[Eli Zaretskii]

> Date: Tue, 16 Aug 2022 09:54:25 -0700
> Cc: bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org,
>  jporterbugs@gmail.com, Bruno Haible <bruno@clisp.org>
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 8/16/22 09:25, Eli Zaretskii wrote:
> > The programmer didn't
> > expect that because it is natural to expect each call to a function
> > that creates a temporary file to create a file under a new name, not
> > reuse the same name.
> 
> Regardless of whether the programmer expects a random name or a 
> predictable-but-unique name, there are only a finite number of names to 
> choose from so the programmer must take into account the possibility 
> that the chosen name clashes with names that the programmer would prefer 
> not to be chosen.

Then what is this comment and the following assertion in Bruno's code
about?

  /* With 6 'X' and a good pseudo-random number generator behind the scenes,
     the probability of getting the same file name twice in a row should be
     1/62^6 < 1/10^10.  */
  ASSERT (strcmp (filename1, filename2) != 0);

That "finite number" of 62^6 sounds pretty close to infinity to me!

> This means an Emacs patch such as [1] is needed regardless of whether 
> Gnulib's gen_tempname is changed to be more random than it is. Although 
> it's true that the bug fixed by [1] is less likely if gen_tempname 
> generates more-random names, the bug can occur no matter what we do 
> about gen_tempname.

No, sorry.  I object to this patch, because it hides the problem under
the carpet.  That there's a file-visiting buffer that records the name
of a temporary file is just one case where this issue gets in the way.
The general problem is still there.  And it isn't an Emacs problem, so
Emacs is not the place to solve it.

Therefore, if there's no intention to fix this in Gnulib, I'm going to
update the documentation of make-temp-file so that Emacs users and
programmers will be informed about that and will be careful enough to
side-step these issues in all the situations.  (Not that I understand
why won't Gnulib provide consistent behavior on all platforms, but I
guess it's above my pay grade.)

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

[Paul Eggert]

On 8/16/22 10:04, Eli Zaretskii wrote:
> That "finite number" of 62^6 sounds pretty close to infinity to me!

It's only 57 billion or so. That's not infinity, considering all the 
Emacs sessions out there. If Emacs's success grows, almost surely 
someone will run into this unlikely bug. Since we have the patch to 
prevent it, why not install it?

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

[Eli Zaretskii]

> Cc: jporterbugs@gmail.com, larsi@gnus.org, bug-gnulib@gnu.org,
>  57129@debbugs.gnu.org, bruno@clisp.org
> Date: Tue, 16 Aug 2022 20:04:49 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> 
> Therefore, if there's no intention to fix this in Gnulib, I'm going to
> update the documentation of make-temp-file so that Emacs users and
> programmers will be informed about that and will be careful enough to
> side-step these issues in all the situations.  (Not that I understand
> why won't Gnulib provide consistent behavior on all platforms, but I
> guess it's above my pay grade.)

And btw, looking closer, I see that this is a regression in Emacs 28,
which was caused by a change in Gnulib: the versions of tempname.c
that we used up to and including Emacs 27.2 used gettimeofday and the
PID to generate the file name, so it was more random than the code in
current Gnulib, and in particular the sequence of

   generate file-name1
   delete file-name1
   generate file-name2

would produce file-name2 that is different from file-name1.  With the
current code in Gnulib, something similar happens only on glibc
systems.  So I hope the code in Gnulib's tempname.c will be amended to
call clock_gettime or something similar, so that the names on
non-glibc platforms could also be random.  Or, failing that, at least
that gen_tempname in glibc would behave identically to other systems,
i.e. start from a fixed "random" value.

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

[Eli Zaretskii]

> Date: Tue, 16 Aug 2022 10:30:54 -0700
> Cc: bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org,
>  jporterbugs@gmail.com, bruno@clisp.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 8/16/22 10:04, Eli Zaretskii wrote:
> > That "finite number" of 62^6 sounds pretty close to infinity to me!
> 
> It's only 57 billion or so. That's not infinity

Yeah, right.

> Since we have the patch to prevent it, why not install it?

Because it isn't needed, and is a weird thing to do.  It also slows
down make-temp-file, especially when a session has a lot of buffers
and with remote file names.  So thanks, but no, thanks.

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

[Paul Eggert]

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

On 8/16/22 09:25, Eli Zaretskii wrote:
> why does this work differently on
> different platforms?

Platforms that lack clock_gettime and CLOCK_MONOTONIC fall back on a 
deterministic algorithm. Presumably MS-DOS one of the few such 
platforms, which is why the problem is observed only on MS-DOS.

How about something like the attached patch to Gnulib's lib/tempname.c? 
If I understand things correctly this should make the names random 
enough on MS-DOS, though Emacs itself still needs a patch as I mentioned 
a few minutes ago.

If this patch isn't good enough for MS-DOS, what sort of random bits are 
easily available on that platform? We don't need anything 
cryptographically secure; a higher-res clock should suffice.

[-- Attachment #2: rand.diff --]
[-- Type: text/x-patch, Size: 889 bytes --]

diff --git a/lib/tempname.c b/lib/tempname.c
index e6520191d7..e3d09d963d 100644
--- a/lib/tempname.c
+++ b/lib/tempname.c
@@ -71,7 +71,8 @@
 
 /* Use getrandom if it works, falling back on a 64-bit linear
    congruential generator that starts with Var's value
-   mixed in with a clock's low-order bits if available.  */
+   mixed in with a clock's low-order bits (or with some other
+   pseudorandom number) if available.  */
 typedef uint_fast64_t random_value;
 #define RANDOM_VALUE_MAX UINT_FAST64_MAX
 #define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */
@@ -89,6 +90,9 @@ random_bits (random_value var, bool use_getrandom)
   struct __timespec64 tv;
   __clock_gettime64 (CLOCK_MONOTONIC, &tv);
   var ^= tv.tv_nsec;
+#else
+  /* Fall back on pseudorandom number <https://bugs.gnu.org/57129>.  */
+  var ^= rand ();
 #endif
   return 2862933555777941757 * var + 3037000493;
 }

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

[Bruno Haible]

Thanks for the explanations, Eli.

Eli Zaretskii wrote:
> > Maybe the problem is that the file gets deleted too early, when some parts
> > of Emacs still reference it?
> 
> The buffer which visited that file still exists, and still records the
> name of the file, yes.  And then, when the program writes to another
> file created by another call to make-temp-file, it actually writes to
> a file that some buffer still visits, and in Emacs that triggers a
> prompt to the user to refresh the buffer.

That is a reasonable behaviour for a text editor — but only for file
names that are explicitly controlled by some program or by the user,
not for temporary files.

> The programmer didn't
> expect that because it is natural to expect each call to a function
> that creates a temporary file to create a file under a new name, not
> reuse the same name.

This is an incorrect assumption. Just like socket numbers are randomly
generated in some situations, temp file names are random, and you can't
make assumptions about the resulting file name.

How about storing, as an attribute of the buffer, a boolean that tells
whether the underlying file name is randomly generated (i.e. a temp file),
and query this boolean before prompting to the user to refresh the buffer?

Bruno

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

[Eli Zaretskii]

> Date: Tue, 16 Aug 2022 10:25:57 -0700
> Cc: bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org,
>  jporterbugs@gmail.com
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 8/16/22 09:25, Eli Zaretskii wrote:
> > why does this work differently on
> > different platforms?
> 
> Platforms that lack clock_gettime and CLOCK_MONOTONIC fall back on a 
> deterministic algorithm. Presumably MS-DOS one of the few such 
> platforms, which is why the problem is observed only on MS-DOS.

(Why are you talking about MS-DOS?)

MinGW does have clock_gettime, but we don't want to use it, because
one of the MinGW flavors (MinGW64) implements that function in
libwinpthreads, and that makes Emacs depend on libwinpthreads DLL,
which we want to avoid.

> How about something like the attached patch to Gnulib's lib/tempname.c? 

Thanks, but why not use 'random' instead?  Emacs does have it on all
platforms, including MS-Windows.  AFAIU, it's better than 'rand'.

> If I understand things correctly this should make the names random 
> enough on MS-DOS, though Emacs itself still needs a patch as I mentioned 
> a few minutes ago.

Why would Emacs need that patch?

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

[Paul Eggert]

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

On 8/16/22 10:47, Eli Zaretskii wrote:

> (Why are you talking about MS-DOS?)

I mistakenly thought it was an MS-DOS problem because tempname.c 
ordinarily would use clock_gettime on MinGW. I didn't know Emacs 
'configure' deliberately suppresses MinGW's clock_gettime.


> Thanks, but why not use 'random' instead?  Emacs does have it on all
> platforms, including MS-Windows.  AFAIU, it's better than 'rand'.

If the code used 'random' then the Gnulib 'tempname' module would need 
to add a dependency on the Gnulib 'random' module, which would in turn 
add a cascading dependency on Gnulib's 'random_r' module. It's better to 
avoid this dependency if we can easily do so.

Come to think of it, we don't need to use 'rand' either, since 
tempname.c already has a good-enough pseudorandom generator. I installed 
into Gnulib the attached patch, which I hope fixes the Emacs problem 
without changing glibc's generated code (once this gets migrated back 
into glibc).


>> If I understand things correctly this should make the names random
>> enough on MS-DOS, though Emacs itself still needs a patch as I mentioned
>> a few minutes ago.
> 
> Why would Emacs need that patch?

In another part of this thread you rejected that patch, on the grounds 
that fixing the unlikely Emacs bug is more trouble than it's worth. So 
I'll drop that suggestion.

[-- Attachment #2: 0001-tempname-generate-better-names-for-MinGW-Emacs.patch --]
[-- Type: text/x-patch, Size: 3592 bytes --]

From 512e44adaebb3096ddd1bf564e679d06e0301616 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Tue, 16 Aug 2022 12:06:48 -0700
Subject: [PATCH] tempname: generate better names for MinGW Emacs
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

On MinGW, GNU Emacs disables clock_gettime, which reliably breaks
some of gen_tempname’s optimistic callers.  Work around the
problem by making the generated names less predictable.  We don’t
need cryptographic randomness here, just enough unpredictability
to keep Emacs happy most of the time.
* lib/tempname.c (HAS_CLOCK_ENTROPY): New macro.
(random_bits): Use it.
(try_tempname_len): On systems lacking clock entropy, maintain
state so that gen_filename generates less-predictable names on
successive successful calls.
---
 ChangeLog      | 14 ++++++++++++++
 lib/tempname.c | 18 +++++++++++++++++-
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/ChangeLog b/ChangeLog
index b639d1709d..eb96281591 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,17 @@
+2022-08-16  Paul Eggert  <eggert@cs.ucla.edu>
+
+	tempname: generate better names for MinGW Emacs
+	On MinGW, GNU Emacs disables clock_gettime, which reliably breaks
+	some of gen_tempname’s optimistic callers.  Work around the
+	problem by making the generated names less predictable.  We don’t
+	need cryptographic randomness here, just enough unpredictability
+	to keep Emacs happy most of the time.
+	* lib/tempname.c (HAS_CLOCK_ENTROPY): New macro.
+	(random_bits): Use it.
+	(try_tempname_len): On systems lacking clock entropy, maintain
+	state so that gen_filename generates less-predictable names on
+	successive successful calls.
+
 2022-08-16  Simon Josefsson  <simon@josefsson.org>
 
 	maintainer-makefile: Check for incorrect DISTCHECK_CONFIGURE_FLAGS
diff --git a/lib/tempname.c b/lib/tempname.c
index e6520191d7..5adfe629a8 100644
--- a/lib/tempname.c
+++ b/lib/tempname.c
@@ -77,6 +77,12 @@ typedef uint_fast64_t random_value;
 #define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */
 #define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62)
 
+#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME)
+# define HAS_CLOCK_ENTROPY true
+#else
+# define HAS_CLOCK_ENTROPY false
+#endif
+
 static random_value
 random_bits (random_value var, bool use_getrandom)
 {
@@ -84,7 +90,7 @@ random_bits (random_value var, bool use_getrandom)
   /* Without GRND_NONBLOCK it can be blocked for minutes on some systems.  */
   if (use_getrandom && __getrandom (&r, sizeof r, GRND_NONBLOCK) == sizeof r)
     return r;
-#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME)
+#if HAS_CLOCK_ENTROPY
   /* Add entropy if getrandom did not work.  */
   struct __timespec64 tv;
   __clock_gettime64 (CLOCK_MONOTONIC, &tv);
@@ -267,6 +273,13 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
      alignment.  */
   random_value v = ((uintptr_t) &v) / alignof (max_align_t);
 
+#if !HAS_CLOCK_ENTROPY
+  /* Arrange gen_tempname to return less predictable file names on
+     systems lacking clock entropy <https://bugs.gnu.org/57129>.  */
+  static random_value prev_v;
+  v ^= prev_v;
+#endif
+
   /* How many random base-62 digits can currently be extracted from V.  */
   int vdigits = 0;
 
@@ -318,6 +331,9 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
       if (fd >= 0)
         {
           __set_errno (save_errno);
+#if !HAS_CLOCK_ENTROPY
+          prev_v = v;
+#endif
           return fd;
         }
       else if (errno != EEXIST)
-- 
2.34.1

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

[Bruno Haible]

Paul Eggert wrote:
> Emacs 'configure' deliberately suppresses MinGW's clock_gettime.

Ah, this explains why Eli saw the problem in Emacs in a deterministic
manner, whereas I see it only with 0.3% probability in my tests on 64-bit
mingw.

> I installed 
> into Gnulib the attached patch, which I hope fixes the Emacs problem 
> without changing glibc's generated code (once this gets migrated back 
> into glibc).

In 10000 runs on 64-bit mingw, your patch went from 27 test failures
to 11 test failures. So, we're still at ca. 0.1% probability (*), far
above the 62^-6 or 10^-10 probability that one would have hoped for.

(*) I wouldn't consider these measurements reliable. Maybe the probability
did not change, and what I'm seeing is merely a measurement fluke.

Bruno

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

[Eli Zaretskii]

> Date: Tue, 16 Aug 2022 12:11:24 -0700
> Cc: bruno@clisp.org, bug-gnulib@gnu.org, larsi@gnus.org,
>  57129@debbugs.gnu.org, jporterbugs@gmail.com
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> > Thanks, but why not use 'random' instead?  Emacs does have it on all
> > platforms, including MS-Windows.  AFAIU, it's better than 'rand'.
> 
> If the code used 'random' then the Gnulib 'tempname' module would need 
> to add a dependency on the Gnulib 'random' module, which would in turn 
> add a cascading dependency on Gnulib's 'random_r' module. It's better to 
> avoid this dependency if we can easily do so.
> 
> Come to think of it, we don't need to use 'rand' either, since 
> tempname.c already has a good-enough pseudorandom generator. I installed 
> into Gnulib the attached patch, which I hope fixes the Emacs problem 
> without changing glibc's generated code (once this gets migrated back 
> into glibc).

Thanks.

Would it be possible to cherry-pick this to the emacs-28 branch, so
that Emacs 28.2 would have this fixed?

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

[Paul Eggert]

On 8/17/22 04:08, Eli Zaretskii wrote:
> Would it be possible to cherry-pick this to the emacs-28 branch, so
> that Emacs 28.2 would have this fixed?

I installed that.

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

[Eli Zaretskii]

> Date: Wed, 17 Aug 2022 23:05:33 -0700
> Cc: bruno@clisp.org, bug-gnulib@gnu.org, larsi@gnus.org,
>  57129@debbugs.gnu.org, jporterbugs@gmail.com
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 8/17/22 04:08, Eli Zaretskii wrote:
> > Would it be possible to cherry-pick this to the emacs-28 branch, so
> > that Emacs 28.2 would have this fixed?
> 
> I installed that.

Thanks!

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

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: eggert@cs.ucla.edu, bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com
> Date: Tue, 16 Aug 2022 19:29:33 +0200
> 
> > The buffer which visited that file still exists, and still records the
> > name of the file, yes.  And then, when the program writes to another
> > file created by another call to make-temp-file, it actually writes to
> > a file that some buffer still visits, and in Emacs that triggers a
> > prompt to the user to refresh the buffer.
> 
> That is a reasonable behaviour for a text editor — but only for file
> names that are explicitly controlled by some program or by the user,
> not for temporary files.

Why not for temporary files?

Emacs has a with-temp-file macro, which generates a temporary file
name, executes the body of the macro with a variable bound to the file
name, then deletes the file.  Very handy for writing test suites.  We
don't usually bother deleting the buffers where the file was processed
in those tests, because the test suite runs in batch mode, and Emacs
exits after running the tests, thus cleaning up.

Having to carefully make sure no such buffers were left from the
previous test is a nuisance.

> > The programmer didn't
> > expect that because it is natural to expect each call to a function
> > that creates a temporary file to create a file under a new name, not
> > reuse the same name.
> 
> This is an incorrect assumption. Just like socket numbers are randomly
> generated in some situations, temp file names are random, and you can't
> make assumptions about the resulting file name.

People get their ideas from other similar APIs, and functions like
tmpfile in C do generate a different name each call.

> How about storing, as an attribute of the buffer, a boolean that tells
> whether the underlying file name is randomly generated (i.e. a temp file),
> and query this boolean before prompting to the user to refresh the buffer?

That's a terrible complications, especially since the solution is so
close at hand, and it makes gen_tempname behave on Windows as it does
on GNU/Linux, and as it behaved just a year or two ago.

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

[Bruno Haible]

Eli Zaretskii wrote:
> Emacs has a with-temp-file macro, which generates a temporary file
> name, executes the body of the macro with a variable bound to the file
> name, then deletes the file.  Very handy for writing test suites.

Since, even with Paul's newest patch, gen_tempname on 64-bit mingw
produces the same file name as on the previous call with a probability
of about 0.1%, you still need to be prepared to see the original problem
occasionally.

Bruno

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

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: eggert@cs.ucla.edu, bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com
> Date: Tue, 16 Aug 2022 22:06:13 +0200
> 
> Eli Zaretskii wrote:
> > Emacs has a with-temp-file macro, which generates a temporary file
> > name, executes the body of the macro with a variable bound to the file
> > name, then deletes the file.  Very handy for writing test suites.
> 
> Since, even with Paul's newest patch, gen_tempname on 64-bit mingw
> produces the same file name as on the previous call with a probability
> of about 0.1%, you still need to be prepared to see the original problem
> occasionally.

I'll take that risk, thank you.

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

[Bruno Haible]

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

Eli Zaretskii wrote:
> Looking at your test program, I see that you generate the seconds file
> name without deleting the first one.  When a file by the first
> generated name already exists, gen_tempname will indeed generate a
> different name.  But in the scenario I described, Emacs creates one
> temporary file, uses it, then deletes it, and only after that creates
> another file.

I'm adding this scenario to the unit test. Still, the unit test succeeds
when run once on:
  Linux, FreeBSD, NetBSD, OpenBSD, macOS, Solaris, Cygwin, native Windows.

Since this contradicts what you debugged on mingw, I ran the test 10000
times on native Windows. Result:
  - on 32-bit mingw, no failure.
  - on 64-bit mingw, around 30 failures (or around 10 failures with Paul's
    newest patch). That is, a probability of ca. 0.3% of getting the same
    file name as on the previous call.

Bruno

[-- Attachment #2: 0001-tempname-Add-more-tests.patch --]
[-- Type: text/x-patch, Size: 3713 bytes --]

From aa52cadc36fb1af0509dc3a4bce4ce73197ece68 Mon Sep 17 00:00:00 2001
From: Bruno Haible <bruno@clisp.org>
Date: Tue, 16 Aug 2022 21:50:11 +0200
Subject: [PATCH] tempname: Add more tests.

Based on scenario described by Eli Zaretskii in
<https://lists.gnu.org/archive/html/bug-gnulib/2022-08/msg00043.html>.

* tests/test-tempname.c (main): Add another test.
* modules/tempname-tests (Status): Mark the test as unportable.
---
 ChangeLog              |  8 ++++++
 modules/tempname-tests |  3 ++
 tests/test-tempname.c  | 65 ++++++++++++++++++++++++++++++------------
 3 files changed, 58 insertions(+), 18 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index eb96281591..de113ce081 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,11 @@
+2022-08-16  Bruno Haible  <bruno@clisp.org>
+
+	tempname: Add more tests.
+	Based on scenario described by Eli Zaretskii in
+	<https://lists.gnu.org/archive/html/bug-gnulib/2022-08/msg00043.html>.
+	* tests/test-tempname.c (main): Add another test.
+	* modules/tempname-tests (Status): Mark the test as unportable.
+
 2022-08-16  Paul Eggert  <eggert@cs.ucla.edu>
 
 	tempname: generate better names for MinGW Emacs
diff --git a/modules/tempname-tests b/modules/tempname-tests
index 384f98707b..adccd0d8e9 100644
--- a/modules/tempname-tests
+++ b/modules/tempname-tests
@@ -2,6 +2,9 @@ Files:
 tests/test-tempname.c
 tests/macros.h
 
+Status:
+unportable-test
+
 Depends-on:
 unlink
 
diff --git a/tests/test-tempname.c b/tests/test-tempname.c
index d463eec2b4..4a0ca65f2c 100644
--- a/tests/test-tempname.c
+++ b/tests/test-tempname.c
@@ -34,24 +34,53 @@ main ()
   char filename1[18 + 1];
   char filename2[18 + 1];
 
-  strcpy (filename1, templ18);
-  int fd1 = gen_tempname (filename1, strlen (".xyz"), 0, GT_FILE);
-  ASSERT (fd1 >= 0);
-
-  strcpy (filename2, templ18);
-  int fd2 = gen_tempname (filename2, strlen (".xyz"), 0, GT_FILE);
-  ASSERT (fd2 >= 0);
-
-  /* With 6 'X' and a good pseudo-random number generator behind the scenes,
-     the probability of getting the same file name twice in a row should be
-     1/62^6 < 1/10^10.  */
-  ASSERT (strcmp (filename1, filename2) != 0);
-
-  /* Clean up.  */
-  close (fd1);
-  close (fd2);
-  unlink (filename1);
-  unlink (filename2);
+  /* Case 1: The first file still exists while gen_tempname is called a second
+     time.  */
+  {
+    strcpy (filename1, templ18);
+    int fd1 = gen_tempname (filename1, strlen (".xyz"), 0, GT_FILE);
+    ASSERT (fd1 >= 0);
+
+    strcpy (filename2, templ18);
+    int fd2 = gen_tempname (filename2, strlen (".xyz"), 0, GT_FILE);
+    ASSERT (fd2 >= 0);
+
+    /* gen_tempname arranges (via O_EXCL) to not return the name of an existing
+       file.  */
+    ASSERT (strcmp (filename1, filename2) != 0);
+
+    /* Clean up.  */
+    close (fd1);
+    close (fd2);
+    unlink (filename1);
+    unlink (filename2);
+  }
+
+  /* Case 2: The first file is deleted before gen_tempname is called a second
+     time.  */
+  {
+    strcpy (filename1, templ18);
+    int fd1 = gen_tempname (filename1, strlen (".xyz"), 0, GT_FILE);
+    ASSERT (fd1 >= 0);
+
+    /* Clean up.  */
+    close (fd1);
+    unlink (filename1);
+
+    strcpy (filename2, templ18);
+    int fd2 = gen_tempname (filename2, strlen (".xyz"), 0, GT_FILE);
+    ASSERT (fd2 >= 0);
+
+    /* With 6 'X' and a good pseudo-random number generator behind the scenes,
+       the probability of getting the same file name twice in a row should be
+       1/62^6 < 1/10^10.
+       But on 64-bit native Windows, this probability is ca. 0.1% to 0.3%.  */
+    ASSERT (strcmp (filename1, filename2) != 0);
+
+    /* Clean up.  */
+    close (fd2);
+    unlink (filename2);
+  }
 
   return 0;
 }
-- 
2.34.1

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

[Paul Eggert]

On 8/16/22 12:59, Bruno Haible wrote:
> Since this contradicts what you debugged on mingw, I ran the test 10000
> times on native Windows. Result:
>    - on 32-bit mingw, no failure.
>    - on 64-bit mingw, around 30 failures (or around 10 failures with Paul's
>      newest patch). That is, a probability of ca. 0.3% of getting the same
>      file name as on the previous call.

That's odd, for two reasons. First, 64-bit and 32-bit mingw shouldn't 
differ, as they both use uint_fast64_t which should be the same width on 
both platforms. Second, I could not reproduce the problem on 64-bit 
Ubuntu x86-64 (after altering tempname.c to always define 
HAS_CLOCK_ENTROPY to false) test-tempname always succeeded in 10000 tries.

Could you investigate further why mingw 64-bit fails?

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

[Bruno Haible]

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

Paul Eggert wrote:
> > Since this contradicts what you debugged on mingw, I ran the test 10000
> > times on native Windows. Result:
> >    - on 32-bit mingw, no failure.
> >    - on 64-bit mingw, around 30 failures (or around 10 failures with Paul's
> >      newest patch). That is, a probability of ca. 0.3% of getting the same
> >      file name as on the previous call.
> 
> That's odd, for two reasons. First, 64-bit and 32-bit mingw shouldn't 
> differ, as they both use uint_fast64_t which should be the same width on 
> both platforms. Second, I could not reproduce the problem on 64-bit 
> Ubuntu x86-64 (after altering tempname.c to always define 
> HAS_CLOCK_ENTROPY to false) test-tempname always succeeded in 10000 tries.
> 
> Could you investigate further why mingw 64-bit fails?

That's odd indeed, and here are more detailed results.

I changed test-tempname.c to skip the case 1 and execute only case 2, and
added printf statements per the attached tempname.c.diff. Then ran
$ for i in `seq 1000`; do ./test-tempname.exe; done 2>&1 | tee out1000

In 32-bit mingw, the result is fully deterministic: each run behaves the same.
The first file name is always gl-temp-3FXzHa.xyz;
the second file name is always gl-temp-HgtmVy.xyz.

Thus, for a single Emacs process, things will look fine, but as soon as someone
starts to use temporary files in two different Emacs processes, in the way Eli
described, there will be massive collisions.

In 64-bit mingw, the 'tempname 1' value is deterministic. This simply shows
that Windows 10 does not use address space randomization (ASLR).
The 'tempname 2' value is unique 99% of the time:
  $ grep 'tempname 2' out1000-mingw64 | sort | uniq -d | wc -l
  8
The interesting thing is that each of the duplicate values of v is occurring
in the same process:
  $ grep 'tempname 2' out1000-mingw64 | sort | uniq -d
  tempname 2 v=0x00c1efa91fb60900
  tempname 2 v=0x32ccb2946974f2f6
  tempname 2 v=0x5cafcc69e359a147
  tempname 2 v=0x6d0676018e27d771
  tempname 2 v=0x6d95bd6083168079
  tempname 2 v=0x7cb95116ffae8ece
  tempname 2 v=0xe0afc7086808ce33
  tempname 2 v=0xe46a60c28fb0ec7f
  $ grep 'tempname 2' out1000-mingw64 | grep -n 0x00c1efa91fb60900
  560:tempname 2 v=0x00c1efa91fb60900
  561:tempname 2 v=0x00c1efa91fb60900
  $ grep 'tempname 2' out1000-mingw64 | grep -n 0x32ccb2946974f2f6
  1129:tempname 2 v=0x32ccb2946974f2f6
  1130:tempname 2 v=0x32ccb2946974f2f6
  etc.

So, in this environment, different Emacs processes will not conflict. But
within a single Emacs process, with 1% probability, the two resulting file
names are the same.

Bruno

[-- Attachment #2: tempname.c.diff --]
[-- Type: text/x-patch, Size: 817 bytes --]

--- lib/tempname.c	2022-08-16 21:27:10.455608233 +0200
+++ tempname.c	2022-08-17 01:55:47.590241280 +0200
@@ -308,6 +308,7 @@
 
   for (count = 0; count < attempts; ++count)
     {
+fprintf(stderr,"tempname 1 v=0x%016llx\n", v);  fflush(stderr);
       for (size_t i = 0; i < x_suffix_len; i++)
         {
           if (vdigits == 0)
@@ -315,6 +316,7 @@
               do
                 {
                   v = random_bits (v, use_getrandom);
+fprintf(stderr,"tempname 2 v=0x%016llx\n", v);  fflush(stderr);
                   use_getrandom = true;
                 }
               while (unfair_min <= v);
@@ -326,7 +328,7 @@
           v /= 62;
           vdigits--;
         }
-
+fprintf(stderr,"tempname filename=%s\n", tmpl); fflush(stderr);
       fd = tryfunc (tmpl, args);
       if (fd >= 0)
         {

[-- Attachment #3: out1000-mingw32.gz --]
[-- Type: application/gzip, Size: 942 bytes --]

[-- Attachment #4: out1000-mingw64.gz --]
[-- Type: application/gzip, Size: 38377 bytes --]

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

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com
> Date: Sun, 21 Aug 2022 18:20:52 +0200
> 
> I changed test-tempname.c to skip the case 1 and execute only case 2, and
> added printf statements per the attached tempname.c.diff. Then ran
> $ for i in `seq 1000`; do ./test-tempname.exe; done 2>&1 | tee out1000
> 
> In 32-bit mingw, the result is fully deterministic: each run behaves the same.
> The first file name is always gl-temp-3FXzHa.xyz;
> the second file name is always gl-temp-HgtmVy.xyz.
> 
> Thus, for a single Emacs process, things will look fine, but as soon as someone
> starts to use temporary files in two different Emacs processes, in the way Eli
> described, there will be massive collisions.
> 
> In 64-bit mingw, the 'tempname 1' value is deterministic. This simply shows
> that Windows 10 does not use address space randomization (ASLR).

Some of these problems could be alleviated if the initial value of 'v'
depended on something non-deterministic, like the program's PID and/or
the current wallclock time (no need to use anything as fancy as
clock_gettime, a simple call to 'clock' should be enough).

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

[Bruno Haible]

Paul Eggert asked:
> > Could you investigate further why mingw 64-bit fails?

Some words on the "why". You seem to have expectations regarding the
distribution quality of the resulting file names, but these expectations
are not warranted.

Donald E. Knuth wrote in TAOCP vol 2 "Seminumerical algorithms" that
an arbitrary sequence of number manipulating operations usually has a
bad quality as a pseudo-random number generator, and that in order
to get good quality pseudo-random numbers, one needs to use *specific*
pseudo-random number generators and then *prove* mathematical properties
of it. (The same is true, btw, for crypto algorithms.)
Then he started discussing linear congruential generators [1] as the
simplest example for which something could be proved.

The current source code of tempname.c generates the pseudo-random numbers
— assuming no HAS_CLOCK_ENTROPY and no ASLR — using a mix of three
operations:
  (A) A linear congruential generator [2] with m = 2^64,
      a = 2862933555777941757, c = 3037000493.
  (B) A floor operation: v ← floor(v / 62^6)
  (C) A xor operation: v ^= prev_v

There are three different questions:
(Q1) What is the expected quality inside a single gen_tempname call?
(Q2) What is the expected quality of multiple gen_tempname calls in a
     single process?
(Q3) What is the expected quality when considering different processes
     that call gen_tempname?

Answers:
(Q1) For just 6 'X'es, there is essentially a single (A) operation.
     Therefore the quality will be good.
     If someone uses more than 10 'X'es, for example, 50 'X'es, there
     will be 5 (A) and 5 (B), interleaved: (A), (B), (A), (B), ...
     This is *not* a linear congruential generator, therefore the
     expected quality is BAD.
     In order to fix this case, what I would do is to get back to
     a linear congruential generator: (A), (A), ..., (A), (B).
     In other words, feed into (A) exactly the output from the
     previous (A). This means, do the statements
          XXXXXX[i] = letters[v % 62];
          v /= 62;
     not on v itself, but on a copy of v.
     But wait, there is also the desire to have predictability!
     This means to not consume all the possible bits the
     random_bits() call, but only part of it.
     What I would do here, is to reduce BASE_62_DIGITS from 10 to 6.
     So that in each round of the loop, 6 base-62 digits are consumed
     and more than 4 base-62 digits are left in v, for predictability.
     In the usual calls with 6 'X'es the loop will still end after a
     single round.

(Q2) First of all, the multiple gen_tempname calls can occur in
     different threads. Since no locking is involved, it is undefined
     behaviour to access the 'prev_v' variable from different threads.
     On machines with an IA-64 CPU, the 'prev_v' variable's value may
     not be propagated from one thread to the other. [3][4][5]
     The fix is simple, though: Mark 'prev_v' as 'volatile'.

     Then, what the code does, is a mix of (A), (B), (C). Again, this
     is *not* a linear congruential generator, therefore the expected
     quality is BAD.

     To get good quality, I would suggest to use a linear congruential
     generator across *all* gen_tempname calls of the entire thread.
     This means:
       - Move out the (B) invocations out, like explained above in (Q1).
       - Remove the (C) code that you added last week.
       - Store the v value in a per-thread variable. Using '__thread'
         on glibc systems and a TLS variable (#include "glthread/tls.h")
         on the other platforms.

(Q3) Here, to force differences between different processes, I would
     suggest to use a fine-grained clock value. In terms of platforms,
       #if defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME
     is way too restrictive.
     How about
       - using CLOCK_REALTIME when CLOCK_MONOTONIC is not available,
       - using gettimeofday() as fallback, especially on native Windows.

If one does (Q3), then the suggestions for (Q2) (other than the 'volatile')
may not be needed.

Bruno

[1] https://en.wikipedia.org/wiki/Linear_congruential_generator
[2] https://en.wikipedia.org/wiki/Linear_congruential_generator#c_%E2%89%A0_0
[3] https://es.cs.uni-kl.de/publications/datarsg/Geor16.pdf
[4] https://db.in.tum.de/teaching/ws1718/dataprocessing/chapter3.pdf?lang=de
    page 18
[5] https://os.inf.tu-dresden.de/Studium/DOS/SS2014/04-Memory-Consistency.pdf

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

[Paul Eggert]

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

Thanks for the detailed diagnosis, Bruno. To try to fix the problems I 
installed the attached patches into Gnulib. If I understand things 
correctly, these patches should fix the 0.1% failure rate you observed 
on 64-bit mingw. They also fix a minor security leak I discovered: in 
rare cases, ASLR entropy was used to generate publicly visible file 
names, which is a no-no as that might help an attacker infer the 
randomized layout of a victim process.

These fixes follow some but not all the suggestions you made. The basic 
problem I saw was that tempname.c was using too much belt-and-suspenders 
code, so much so that the combination of belts and suspenders 
misbehaved. I simplified it a bit and this removed the need for some of 
the suggestions.

These fixes should be merged into glibc upstream since they fix glibc 
bugs; I plan to follow up on that shortly.

[-- Attachment #2: 0001-tempname-merge-64-bit-time_t-fix-from-glibc.patch --]
[-- Type: text/x-patch, Size: 2463 bytes --]

From 8304617684ba7f71c36fcf49786d3b279dfbefc3 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 22 Aug 2022 12:22:52 -0700
Subject: [PATCH 1/3] tempname: merge 64-bit time_t fix from glibc

This merges glibc commit 52a5fe70a2c77935afe807fb6e904e512ddd894e
"Use 64 bit time_t stat internally".
* lib/tempname.c (struct_stat64) [_LIBC]: Use struct __stat64_t64.
(__lstat64_time64) [!_LIBC]: Rename from __lstat64.
All uses changed.
(direxists): Use __stat64_time64 instead of __stat64.
---
 ChangeLog      | 10 ++++++++++
 lib/tempname.c |  8 ++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index de113ce081..adb445ddcf 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,13 @@
+2022-08-22  Paul Eggert  <eggert@cs.ucla.edu>
+
+	tempname: merge 64-bit time_t fix from glibc
+	This merges glibc commit 52a5fe70a2c77935afe807fb6e904e512ddd894e
+	"Use 64 bit time_t stat internally".
+	* lib/tempname.c (struct_stat64) [_LIBC]: Use struct __stat64_t64.
+	(__lstat64_time64) [!_LIBC]: Rename from __lstat64.
+	All uses changed.
+	(direxists): Use __stat64_time64 instead of __stat64.
+
 2022-08-16  Bruno Haible  <bruno@clisp.org>
 
 	tempname: Add more tests.
diff --git a/lib/tempname.c b/lib/tempname.c
index 5adfe629a8..4f615b90b9 100644
--- a/lib/tempname.c
+++ b/lib/tempname.c
@@ -55,14 +55,14 @@
 #include <time.h>
 
 #if _LIBC
-# define struct_stat64 struct stat64
+# define struct_stat64 struct __stat64_t64
 # define __secure_getenv __libc_secure_getenv
 #else
 # define struct_stat64 struct stat
 # define __gen_tempname gen_tempname
 # define __mkdir mkdir
 # define __open open
-# define __lstat64(file, buf) lstat (file, buf)
+# define __lstat64_time64(file, buf) lstat (file, buf)
 # define __stat64(file, buf) stat (file, buf)
 # define __getrandom getrandom
 # define __clock_gettime64 clock_gettime
@@ -105,7 +105,7 @@ static int
 direxists (const char *dir)
 {
   struct_stat64 buf;
-  return __stat64 (dir, &buf) == 0 && S_ISDIR (buf.st_mode);
+  return __stat64_time64 (dir, &buf) == 0 && S_ISDIR (buf.st_mode);
 }
 
 /* Path search algorithm, for tmpnam, tmpfile, etc.  If DIR is
@@ -197,7 +197,7 @@ try_nocreate (char *tmpl, _GL_UNUSED void *flags)
 {
   struct_stat64 st;
 
-  if (__lstat64 (tmpl, &st) == 0 || errno == EOVERFLOW)
+  if (__lstat64_time64 (tmpl, &st) == 0 || errno == EOVERFLOW)
     __set_errno (EEXIST);
   return errno == ENOENT ? 0 : -1;
 }
-- 
2.37.2


[-- Attachment #3: 0002-tempname-fix-multithreading-ASLR-leak-etc.patch --]
[-- Type: text/x-patch, Size: 8768 bytes --]

From 9ce573cde017182a69881241e8565ec04e5bc728 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 22 Aug 2022 12:07:27 -0700
Subject: [PATCH 2/3] tempname: fix multithreading, ASLR leak etc.

Fix problems with tempname and multithreading, entropy loss,
and missing clock data (this last on non-GNU platforms).
See analysis by Bruno Haible in:
https://bugs.gnu.org/57129#149
While looking into this, I noticed that tempname can leak
info derived from ASLR into publicly-visible file names,
which is a no-no.  Fix that too.
* lib/tempname.c: Don't include stdalign.h.
(HAS_CLOCK_ENTROPY): Remove.
(mix_random_values): New function.
(random_bits): Use it.  Args are now new value address and
old value, and this function now returns a success indicator.
Omit old USE_GETRANDOM argument: always try getrandom now, as
there is no good reason not to now that GRND_NONBLOCK is used.
Caller changed.  Use CLOCK_REALTIME for for ersatz entropy,
as CLOCK_MONOTONIC doesn't work on some platforms.
Also, mix in ersatz entropy from tv_sec and from clock ().
(try_tempname_len): Do not mix in ASLR-based entropy, as
the result is published to the world and ASLR should be private.
Do not try to use a static var as that has issues if multithreaded.
Instead, simply generate new random bits.
Worry about bias only with high-quality random bits.
* modules/tempname (Depends-on): Do not depend on stdalign.
---
 ChangeLog        | 25 +++++++++++++
 lib/tempname.c   | 97 +++++++++++++++++++++++++-----------------------
 modules/tempname |  1 -
 3 files changed, 76 insertions(+), 47 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index adb445ddcf..ed99c845f7 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,30 @@
 2022-08-22  Paul Eggert  <eggert@cs.ucla.edu>
 
+	tempname: fix multithreading, ASLR leak etc.
+	Fix problems with tempname and multithreading, entropy loss,
+	and missing clock data (this last on non-GNU platforms).
+	See analysis by Bruno Haible in:
+	https://bugs.gnu.org/57129#149
+	While looking into this, I noticed that tempname can leak
+	info derived from ASLR into publicly-visible file names,
+	which is a no-no.  Fix that too.
+	* lib/tempname.c: Don't include stdalign.h.
+	(HAS_CLOCK_ENTROPY): Remove.
+	(mix_random_values): New function.
+	(random_bits): Use it.  Args are now new value address and
+	old value, and this function now returns a success indicator.
+	Omit old USE_GETRANDOM argument: always try getrandom now, as
+	there is no good reason not to now that GRND_NONBLOCK is used.
+	Caller changed.  Use CLOCK_REALTIME for for ersatz entropy,
+	as CLOCK_MONOTONIC doesn't work on some platforms.
+	Also, mix in ersatz entropy from tv_sec and from clock ().
+	(try_tempname_len): Do not mix in ASLR-based entropy, as
+	the result is published to the world and ASLR should be private.
+	Do not try to use a static var as that has issues if multithreaded.
+	Instead, simply generate new random bits.
+	Worry about bias only with high-quality random bits.
+	* modules/tempname (Depends-on): Do not depend on stdalign.
+
 	tempname: merge 64-bit time_t fix from glibc
 	This merges glibc commit 52a5fe70a2c77935afe807fb6e904e512ddd894e
 	"Use 64 bit time_t stat internally".
diff --git a/lib/tempname.c b/lib/tempname.c
index 4f615b90b9..bc1f7c14dd 100644
--- a/lib/tempname.c
+++ b/lib/tempname.c
@@ -48,7 +48,6 @@
 #include <string.h>
 
 #include <fcntl.h>
-#include <stdalign.h>
 #include <stdint.h>
 #include <sys/random.h>
 #include <sys/stat.h>
@@ -77,26 +76,55 @@ typedef uint_fast64_t random_value;
 #define BASE_62_DIGITS 10 /* 62**10 < UINT_FAST64_MAX */
 #define BASE_62_POWER (62LL * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62 * 62)
 
-#if _LIBC || (defined CLOCK_MONOTONIC && HAVE_CLOCK_GETTIME)
-# define HAS_CLOCK_ENTROPY true
-#else
-# define HAS_CLOCK_ENTROPY false
-#endif
+/* Return the result of mixing the entropy from R and S.
+   Assume that R and S are not particularly random,
+   and that the result should look randomish to an untrained eye.  */
 
 static random_value
-random_bits (random_value var, bool use_getrandom)
+mix_random_values (random_value r, random_value s)
+{
+  /* As this code is used only when high-quality randomness is neither
+     available nor necessary, there is no need for fancier polynomials
+     such as those in the Linux kernel's 'random' driver.  */
+  return (2862933555777941757 * r + 3037000493) ^ s;
+}
+
+/* Set *R to a random value.
+   Return true if *R is set to high-quality value taken from getrandom.
+   Otherwise return false, falling back to a low-quality *R that might
+   depend on S.
+
+   This function returns false only when getrandom fails.
+   On GNU systems this should happen only early in the boot process,
+   when the fallback should be good enough for programs using tempname
+   because any attacker likely has root privileges already.  */
+
+static bool
+random_bits (random_value *r, random_value s)
 {
-  random_value r;
   /* Without GRND_NONBLOCK it can be blocked for minutes on some systems.  */
-  if (use_getrandom && __getrandom (&r, sizeof r, GRND_NONBLOCK) == sizeof r)
-    return r;
-#if HAS_CLOCK_ENTROPY
-  /* Add entropy if getrandom did not work.  */
+  if (__getrandom (r, sizeof *r, GRND_NONBLOCK) == sizeof *r)
+    return true;
+
+  /* If getrandom did not work, use ersatz entropy based on low-order
+     clock bits.  On GNU systems getrandom should fail only
+     early in booting, when ersatz should be good enough.
+     Do not use ASLR-based entropy, as that would leak ASLR info into
+     the resulting file name which is typically public.
+
+     Of course we are in a state of sin here.  */
+
+  random_value v = 0;
+
+#if _LIBC || (defined CLOCK_REALTIME && HAVE_CLOCK_GETTIME)
   struct __timespec64 tv;
-  __clock_gettime64 (CLOCK_MONOTONIC, &tv);
-  var ^= tv.tv_nsec;
+  __clock_gettime64 (CLOCK_REALTIME, &tv);
+  v = mix_random_values (v, tv.tv_sec);
+  v = mix_random_values (v, tv.tv_nsec);
 #endif
-  return 2862933555777941757 * var + 3037000493;
+
+  *r = mix_random_values (v, clock ());
+  return false;
 }
 
 #if _LIBC
@@ -267,32 +295,15 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
   unsigned int attempts = ATTEMPTS_MIN;
 #endif
 
-  /* A random variable.  The initial value is used only the for fallback path
-     on 'random_bits' on 'getrandom' failure.  Its initial value tries to use
-     some entropy from the ASLR and ignore possible bits from the stack
-     alignment.  */
-  random_value v = ((uintptr_t) &v) / alignof (max_align_t);
-
-#if !HAS_CLOCK_ENTROPY
-  /* Arrange gen_tempname to return less predictable file names on
-     systems lacking clock entropy <https://bugs.gnu.org/57129>.  */
-  static random_value prev_v;
-  v ^= prev_v;
-#endif
+  /* A random variable.  */
+  random_value v = 0;
 
   /* How many random base-62 digits can currently be extracted from V.  */
   int vdigits = 0;
 
-  /* Whether to consume entropy when acquiring random bits.  On the
-     first try it's worth the entropy cost with __GT_NOCREATE, which
-     is inherently insecure and can use the entropy to make it a bit
-     more secure.  On the (rare) second and later attempts it might
-     help against DoS attacks.  */
-  bool use_getrandom = tryfunc == try_nocreate;
-
-  /* Least unfair value for V.  If V is less than this, V can generate
-     BASE_62_DIGITS digits fairly.  Otherwise it might be biased.  */
-  random_value const unfair_min
+  /* Least biased value for V.  If V is less than this, V can generate
+     BASE_62_DIGITS unbiased digits.  Otherwise the digits are biased.  */
+  random_value const biased_min
     = RANDOM_VALUE_MAX - RANDOM_VALUE_MAX % BASE_62_POWER;
 
   len = strlen (tmpl);
@@ -312,12 +323,9 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
         {
           if (vdigits == 0)
             {
-              do
-                {
-                  v = random_bits (v, use_getrandom);
-                  use_getrandom = true;
-                }
-              while (unfair_min <= v);
+              /* Worry about bias only if the bits are high quality.  */
+              while (random_bits (&v, v) && biased_min <= v)
+                continue;
 
               vdigits = BASE_62_DIGITS;
             }
@@ -331,9 +339,6 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
       if (fd >= 0)
         {
           __set_errno (save_errno);
-#if !HAS_CLOCK_ENTROPY
-          prev_v = v;
-#endif
           return fd;
         }
       else if (errno != EEXIST)
diff --git a/modules/tempname b/modules/tempname
index 4779735d9d..f1fb78e8ff 100644
--- a/modules/tempname
+++ b/modules/tempname
@@ -16,7 +16,6 @@ getrandom
 libc-config
 lstat
 mkdir
-stdalign
 stdbool
 stdint
 sys_stat
-- 
2.37.2


[-- Attachment #4: 0003-tempname-don-t-lose-entropy-in-seed.patch --]
[-- Type: text/x-patch, Size: 2663 bytes --]

From d27c820595175ed563b4d4ee5d0f421011891572 Mon Sep 17 00:00:00 2001
From: Paul Eggert <eggert@cs.ucla.edu>
Date: Mon, 22 Aug 2022 15:43:18 -0500
Subject: [PATCH 3/3] tempname: don't lose entropy in seed

* lib/tempname.c (random_bits): Don't lose entropy in S
in the rare case where where the template has more than 10 Xs.
From a suggestion by Bruno Haible in:
https://bugs.gnu.org/57129#149
---
 ChangeLog      |  7 +++++++
 lib/tempname.c | 11 +++++++----
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/ChangeLog b/ChangeLog
index ed99c845f7..4d1c92cd81 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,5 +1,11 @@
 2022-08-22  Paul Eggert  <eggert@cs.ucla.edu>
 
+	tempname: don't lose entropy in seed
+	* lib/tempname.c (random_bits): Don't lose entropy in S
+	in the rare case where where the template has more than 10 Xs.
+	From a suggestion by Bruno Haible in:
+	https://bugs.gnu.org/57129#149
+
 	tempname: fix multithreading, ASLR leak etc.
 	Fix problems with tempname and multithreading, entropy loss,
 	and missing clock data (this last on non-GNU platforms).
@@ -23,6 +29,7 @@
 	Do not try to use a static var as that has issues if multithreaded.
 	Instead, simply generate new random bits.
 	Worry about bias only with high-quality random bits.
+
 	* modules/tempname (Depends-on): Do not depend on stdalign.
 
 	tempname: merge 64-bit time_t fix from glibc
diff --git a/lib/tempname.c b/lib/tempname.c
index bc1f7c14dd..0e2f29f5de 100644
--- a/lib/tempname.c
+++ b/lib/tempname.c
@@ -114,7 +114,7 @@ random_bits (random_value *r, random_value s)
 
      Of course we are in a state of sin here.  */
 
-  random_value v = 0;
+  random_value v = s;
 
 #if _LIBC || (defined CLOCK_REALTIME && HAVE_CLOCK_GETTIME)
   struct __timespec64 tv;
@@ -298,7 +298,9 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
   /* A random variable.  */
   random_value v = 0;
 
-  /* How many random base-62 digits can currently be extracted from V.  */
+  /* A value derived from the random variable, and how many random
+     base-62 digits can currently be extracted from VDIGBUF.  */
+  random_value vdigbuf;
   int vdigits = 0;
 
   /* Least biased value for V.  If V is less than this, V can generate
@@ -327,11 +329,12 @@ try_tempname_len (char *tmpl, int suffixlen, void *args,
               while (random_bits (&v, v) && biased_min <= v)
                 continue;
 
+              vdigbuf = v;
               vdigits = BASE_62_DIGITS;
             }
 
-          XXXXXX[i] = letters[v % 62];
-          v /= 62;
+          XXXXXX[i] = letters[vdigbuf % 62];
+          vdigbuf /= 62;
           vdigits--;
         }
 
-- 
2.37.2

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

[Bruno Haible]

Paul Eggert wrote:
> Thanks for the detailed diagnosis, Bruno. To try to fix the problems I 
> installed the attached patches into Gnulib.

These look all good. Here's again my evaluation of the three scenarios:

(Q1) What is the expected quality inside a single gen_tempname call?

It depends on quality of random_bits(). Usually on the quality of
getrandom(), which is good on most systems. And for the other systems,
the "ersatz" in random_bits() gives reasonable quality.

(Q2) What is the expected quality of multiple gen_tempname calls in a
     single process?
(Q3) What is the expected quality when considering different processes
     that call gen_tempname?

Both have the same answer: The file name essentially depends on the
first call to random_bits(). Two different calls to random_bits()
can be correlated only if
  - getrandom() is not well supported, and
  - CLOCK_REALTIME is not defined, and
  - we're in the same process, less than 0.01 sec apart.
IMO, that's good enough.

> If I understand things 
> correctly, these patches should fix the 0.1% failure rate you observed 
> on 64-bit mingw.

Yes. Running the "case 2" part 1000 times again, among the 2000 generated
file names, there are no duplicates — neither on 32-bit mingw, nor on 64-bit
mingw.

> They also fix a minor security leak I discovered: in 
> rare cases, ASLR entropy was used to generate publicly visible file 
> names, which is a no-no as that might help an attacker infer the 
> randomized layout of a victim process.

Excellent observation :-)

> These fixes follow some but not all the suggestions you made. The basic 
> problem I saw was that tempname.c was using too much belt-and-suspenders 
> code, so much so that the combination of belts and suspenders 
> misbehaved. I simplified it a bit and this removed the need for some of 
> the suggestions.

Thanks. The major quality boost comes from invoking getrandom() always.

Bruno

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

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: Eli Zaretskii <eliz@gnu.org>, bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com
> Date: Tue, 23 Aug 2022 02:13:02 +0200
> 
> > If I understand things 
> > correctly, these patches should fix the 0.1% failure rate you observed 
> > on 64-bit mingw.
> 
> Yes. Running the "case 2" part 1000 times again, among the 2000 generated
> file names, there are no duplicates — neither on 32-bit mingw, nor on 64-bit
> mingw.

Was that with or without using clock_gettime?  If it was with
clock_gettime, what happens without it (i.e. when 'clock' is used
instead)?

Thanks.

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

[Bruno Haible]

Eli Zaretskii asked:
> > Yes. Running the "case 2" part 1000 times again, among the 2000 generated
> > file names, there are no duplicates — neither on 32-bit mingw, nor on 64-bit
> > mingw.
> 
> Was that with or without using clock_gettime?  If it was with
> clock_gettime, what happens without it (i.e. when 'clock' is used
> instead)?

That was with clock_gettime. Without clock_gettime, i.e. without the
dependency to libwinpthread, the result is the same: no duplicates — neither
on 32-bit mingw, nor on 64-bit mingw.

Bruno

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

[Eli Zaretskii]

> From: Bruno Haible <bruno@clisp.org>
> Cc: eggert@cs.ucla.edu, bug-gnulib@gnu.org, larsi@gnus.org, 57129@debbugs.gnu.org, jporterbugs@gmail.com
> Date: Tue, 23 Aug 2022 16:49:36 +0200
> 
> Eli Zaretskii asked:
> > > Yes. Running the "case 2" part 1000 times again, among the 2000 generated
> > > file names, there are no duplicates — neither on 32-bit mingw, nor on 64-bit
> > > mingw.
> > 
> > Was that with or without using clock_gettime?  If it was with
> > clock_gettime, what happens without it (i.e. when 'clock' is used
> > instead)?
> 
> That was with clock_gettime. Without clock_gettime, i.e. without the
> dependency to libwinpthread, the result is the same: no duplicates — neither
> on 32-bit mingw, nor on 64-bit mingw.

Great, thanks.

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

[Jim Porter]

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

On 8/15/2022 11:58 AM, Eli Zaretskii wrote:
>> Cc: larsi@gnus.org, 57129@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Mon, 15 Aug 2022 11:30:07 -0700
>>
>> The temp files are created by Eshell in lisp/eshell/esh-var.el in the
>> function 'eshell-parse-variable-ref', specifically in the part starting
>> with:
>>
>>     (eq (char-after) ?\<)
> 
> Ah, okay.  It's a (mis)feature of Gnulib's gen_tempname function
> (which is the guts of make-temp-file) in its implementation for
> MS-Windows (and maybe other platforms?): it always begins from the
> same "random" characters in the file name, and only generates other
> random characters if there's already a file by that name.  So if you
> are careful and delete the temporary file each time after usage, and
> never need more than one temporary file at the same time, you will get
> the same name every call.

In addition to the changes to temporary file name generation, I think it 
would be useful for Eshell to kill the temporary buffer too. If you use 
this feature in Eshell a lot, the temporary buffers could pile up, 
consuming memory for no real benefit. (A user who wanted the buffer to 
stick around would probably redirect to a non-temporary file, or even 
just to a buffer.) Attached is a patch to do this.

[-- Attachment #2: 0001-Kill-the-buffer-associated-with-a-temp-file-when-usi.patch --]
[-- Type: text/plain, Size: 1403 bytes --]

From 62e4316bad6645ff12ef7587735b9e1c5e74fe21 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Sat, 20 Aug 2022 10:48:32 -0700
Subject: [PATCH] Kill the buffer associated with a temp file when using
 '$<command>' in Eshell

* lisp/eshell/esh-var.el (eshell-parse-variable-ref): Kill the temp
file's buffer when we're done.
---
 lisp/eshell/esh-var.el | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/lisp/eshell/esh-var.el b/lisp/eshell/esh-var.el
index 2f6614b5d7..a9df172e88 100644
--- a/lisp/eshell/esh-var.el
+++ b/lisp/eshell/esh-var.el
@@ -490,8 +490,11 @@ eshell-parse-variable-ref
                            ;; by `eshell-do-eval', which requires very
                            ;; particular forms in order to work
                            ;; properly.  See bug#54190.
-                           (list (function (lambda ()
-                                   (delete-file ,temp))))))
+                           (list (function
+                                  (lambda ()
+                                    (delete-file ,temp)
+                                    (when-let ((buffer (get-file-buffer ,temp)))
+                                      (kill-buffer buffer)))))))
                    (eshell-apply-indices ,temp indices ,eshell-current-quoted)))
             (goto-char (1+ end)))))))
    ((eq (char-after) ?\()
-- 
2.25.1

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

[Eli Zaretskii]

> Cc: larsi@gnus.org, 57129@debbugs.gnu.org
> From: Jim Porter <jporterbugs@gmail.com>
> Date: Sat, 20 Aug 2022 11:03:27 -0700
> 
> In addition to the changes to temporary file name generation, I think it 
> would be useful for Eshell to kill the temporary buffer too. If you use 
> this feature in Eshell a lot, the temporary buffers could pile up, 
> consuming memory for no real benefit. (A user who wanted the buffer to 
> stick around would probably redirect to a non-temporary file, or even 
> just to a buffer.) Attached is a patch to do this.

That makes sense, of course, but why not use with-temp-buffer in the
first place?

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

[Jim Porter]

On 8/20/2022 11:14 AM, Eli Zaretskii wrote:
>> Cc: larsi@gnus.org, 57129@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Sat, 20 Aug 2022 11:03:27 -0700
>>
>> In addition to the changes to temporary file name generation, I think it
>> would be useful for Eshell to kill the temporary buffer too. If you use
>> this feature in Eshell a lot, the temporary buffers could pile up,
>> consuming memory for no real benefit. (A user who wanted the buffer to
>> stick around would probably redirect to a non-temporary file, or even
>> just to a buffer.) Attached is a patch to do this.
> 
> That makes sense, of course, but why not use with-temp-buffer in the
> first place?

I'm not sure that would work in this case. 'with-temp-buffer' (or 
probably 'with-temp-file', since we want a real file) would clean 
everything up once BODY has executed, but we want to perform cleanup in 
a hook, which might execute quite a while later (e.g. if passing this 
temp file to an external process).

If there's a way to use that though, I'm all for it.

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

[Jim Porter]

On 8/20/2022 11:03 AM, Jim Porter wrote:
> In addition to the changes to temporary file name generation, I think it 
> would be useful for Eshell to kill the temporary buffer too.
[snip]
> Attached is a patch to do this.

Assuming there are no remaining objections, I'll merge this in a day or two.

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

[Jim Porter]

On 8/24/2022 2:41 PM, Jim Porter wrote:
> On 8/20/2022 11:03 AM, Jim Porter wrote:
>> In addition to the changes to temporary file name generation, I think 
>> it would be useful for Eshell to kill the temporary buffer too.
> [snip]
>> Attached is a patch to do this.
> 
> Assuming there are no remaining objections, I'll merge this in a day or 
> two.

Merged as a457aa62577284333c7d25d48a49704788b25a04.

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

[Jim Porter]

On 8/25/2022 10:10 PM, Jim Porter wrote:
> On 8/24/2022 2:41 PM, Jim Porter wrote:
>> On 8/20/2022 11:03 AM, Jim Porter wrote:
>>> In addition to the changes to temporary file name generation, I think 
>>> it would be useful for Eshell to kill the temporary buffer too.
>> [snip]
>>> Attached is a patch to do this.
>>
>> Assuming there are no remaining objections, I'll merge this in a day 
>> or two.
> 
> Merged as a457aa62577284333c7d25d48a49704788b25a04.

I know there was a quite-lengthy discussion about the tempname function, 
but I think those patches were merged, along with my patch for the 
original bug topic. Therefore, I'm going to close this.

Of course, if there's still anything to do with the tempname stuff, 
let's do it (though it might help to give it a separate bug for tracking 
purposes).

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

[Sean Whitton]

Hello,

On Fri 12 Aug 2022 at 10:14PM -07, Jim Porter wrote:

> On 8/12/2022 8:22 AM, Lars Ingebrigtsen wrote:
>> Jim Porter <jporterbugs@gmail.com> writes:
>>
>>> Here are some patches to fix this.
>> I haven't tested the patches, but reading them, they make sense to me,
>> so please go ahead and push.
>
> Thanks for taking a look. Merged as f3408af0a3251a744cb0b55b5e153565bfd57ea3.

Thanks for working on this, pretty cool.

-- 
Sean Whitton