bug#53293: 29.0.50; [PATCH] `eshell-eval-using-options' should report error for unrecognized option, even with no :external

[Jim Porter <jporterbugs@gmail.com>]

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

On 1/16/2022 12:31 PM, Jim Porter wrote:
> In conclusion, I think it would make sense to do the following:
> 
> * Add support for "echo -E" (no-op) and possibly ignore "echo -e".
> * Fix the bug with the source command
> * Consider whether to add support for --version to various Eshell commands.
> * Decide what, if anything, to do with some of the su/sudo options 
> listed above.

While it would probably make sense to wait a bit longer to see if anyone 
has comments on my audit of the existing Eshell built-in commands, I've 
implemented the first two points here (not "echo -e", though).

The changes to `source' and `.' might warrant a NEWS entry, since it's a 
(small) incompatible change; however, I think it's so rare that someone 
would call "source --help" that it might not be worth adding to NEWS. 
I'll do whatever others think is best here, though.

[-- Attachment #2: 0001-Raise-an-error-from-eval-eval-using-options-for-unkn.patch --]
[-- Type: text/plain, Size: 6544 bytes --]

From cbf63ad954002e587a7c674e2f33a37ae99b21b5 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 19 Jan 2022 18:59:23 -0800
Subject: [PATCH 1/2] Raise an error from 'eval-eval-using-options' for unknown
 options

* lisp/eshell/em-basic.el (eshell/echo): Add -E option.

* lisp/eshell/esh-opt.el (eshell--process-option): Raise an error if
an unknown option is encountered, even when :external is nil.

* test/lisp/eshell/esh-opt-tests.el (esh-opt-process-args-test)
(test-eshell-eval-using-options): Add test cases for this.
---
 lisp/eshell/em-basic.el           |  3 +-
 lisp/eshell/esh-opt.el            | 12 ++---
 test/lisp/eshell/esh-opt-tests.el | 86 +++++++++++++++++++++++--------
 3 files changed, 72 insertions(+), 29 deletions(-)

diff --git a/lisp/eshell/em-basic.el b/lisp/eshell/em-basic.el
index 27b343ad39..5b174de420 100644
--- a/lisp/eshell/em-basic.el
+++ b/lisp/eshell/em-basic.el
@@ -110,9 +110,10 @@ eshell/echo
   (eshell-eval-using-options
    "echo" args
    '((?n nil nil output-newline "terminate with a newline")
+     (?E nil nil _disable-escapes "don't interpret backslash escapes (default)")
      (?h "help" nil nil "output this help screen")
      :preserve-args
-     :usage "[-n] [object]")
+     :usage "[OPTION]... [OBJECT]...")
    (eshell-echo args output-newline)))
 
 (defun eshell/printnl (&rest args)
diff --git a/lisp/eshell/esh-opt.el b/lisp/eshell/esh-opt.el
index bba1c4ad25..c802bee3af 100644
--- a/lisp/eshell/esh-opt.el
+++ b/lisp/eshell/esh-opt.el
@@ -257,12 +257,12 @@ eshell--process-option
         remaining
       (let ((extcmd (memq ':external options)))
 	(when extcmd
-	  (setq extcmd (eshell-search-path (cadr extcmd)))
-	  (if extcmd
-	      (throw 'eshell-ext-command extcmd)
-            (error (if (characterp (car switch)) "%s: unrecognized option -%c"
-                     "%s: unrecognized option --%s")
-                   name (car switch))))))))
+	  (setq extcmd (eshell-search-path (cadr extcmd))))
+	(if extcmd
+	    (throw 'eshell-ext-command extcmd)
+          (error (if (characterp (car switch)) "%s: unrecognized option -%c"
+                   "%s: unrecognized option --%s")
+                 name (car switch)))))))
 
 (defun eshell--process-args (name args options)
   "Process the given ARGS using OPTIONS."
diff --git a/test/lisp/eshell/esh-opt-tests.el b/test/lisp/eshell/esh-opt-tests.el
index 255768635b..b76ed8866d 100644
--- a/test/lisp/eshell/esh-opt-tests.el
+++ b/test/lisp/eshell/esh-opt-tests.el
@@ -27,41 +27,63 @@ esh-opt-process-args-test
   (should
    (equal '(t)
           (eshell--process-args
-           "sudo"
-           '("-a")
-           '((?a "all" nil show-all "")))))
-  (should
-   (equal '(nil)
-          (eshell--process-args
-           "sudo"
-           '("-g")
-           '((?a "all" nil show-all "")))))
+           "sudo" '("-a")
+           '((?a "all" nil show-all
+                 "do not ignore entries starting with .")))))
   (should
    (equal '("root" "world")
           (eshell--process-args
-           "sudo"
-           '("-u" "root" "world")
-           '((?u "user" t user "execute a command as another USER")))))
+           "sudo" '("-u" "root" "world")
+           '((?u "user" t user
+                 "execute a command as another USER")))))
   (should
    (equal '(nil "emerge" "-uDN" "world")
           (eshell--process-args
-           "sudo"
-           '("emerge" "-uDN" "world")
-           '((?u "user" t user "execute a command as another USER")
+           "sudo" '("emerge" "-uDN" "world")
+           '((?u "user" t user
+                 "execute a command as another USER")
              :parse-leading-options-only))))
   (should
    (equal '("root" "emerge" "-uDN" "world")
           (eshell--process-args
-           "sudo"
-           '("-u" "root" "emerge" "-uDN" "world")
-           '((?u "user" t user "execute a command as another USER")
+           "sudo" '("-u" "root" "emerge" "-uDN" "world")
+           '((?u "user" t user
+                 "execute a command as another USER")
              :parse-leading-options-only))))
   (should
    (equal '("DN" "emerge" "world")
           (eshell--process-args
-           "sudo"
-           '("-u" "root" "emerge" "-uDN" "world")
-           '((?u "user" t user "execute a command as another USER"))))))
+           "sudo" '("-u" "root" "emerge" "-uDN" "world")
+           '((?u "user" t user
+                 "execute a command as another USER")))))
+
+  ;; Test :external.
+  (cl-letf (((symbol-function 'eshell-search-path) #'ignore))
+    (should
+     (equal '(nil "/some/path")
+            (eshell--process-args
+             "ls" '("/some/path")
+             '((?a "all" nil show-all
+                   "do not ignore entries starting with .")
+               :external "ls")))))
+  (cl-letf (((symbol-function 'eshell-search-path) #'identity))
+    (should
+     (equal '(no-catch eshell-ext-command "ls")
+            (should-error
+             (eshell--process-args
+              "ls" '("-u" "/some/path")
+              '((?a "all" nil show-all
+                    "do not ignore entries starting with .")
+                :external "ls"))
+             :type 'no-catch))))
+  (cl-letf (((symbol-function 'eshell-search-path) #'ignore))
+    (should-error
+     (eshell--process-args
+      "ls" '("-u" "/some/path")
+      '((?a "all" nil show-all
+            "do not ignore entries starting with .")
+        :external "ls"))
+     :type 'error)))
 
 (ert-deftest test-eshell-eval-using-options ()
   "Tests for `eshell-eval-using-options'."
@@ -190,7 +212,27 @@ test-eshell-eval-using-options
    '((?u "user" t user "execute a command as another USER")
      :parse-leading-options-only)
    (should (eq user nil))
-   (should (equal args '("emerge" "-uDN" "world")))))
+   (should (equal args '("emerge" "-uDN" "world"))))
+
+  ;; Test unrecognized options.
+  (should-error
+   (eshell-eval-using-options
+    "ls" '("-u" "/some/path")
+    '((?a "all" nil show-all
+          "do not ignore entries starting with ."))
+    (ignore show-all)))
+  (should-error
+   (eshell-eval-using-options
+    "ls" '("-au" "/some/path")
+    '((?a "all" nil show-all
+          "do not ignore entries starting with ."))
+    (ignore show-all)))
+  (should-error
+   (eshell-eval-using-options
+    "ls" '("--unrecognized" "/some/path")
+    '((?a "all" nil show-all
+          "do not ignore entries starting with ."))
+    (ignore show-all))))
 
 (provide 'esh-opt-tests)
 
-- 
2.25.1


[-- Attachment #3: 0002-Don-t-use-eshell-eval-using-options-for-eshell-sourc.patch --]
[-- Type: text/plain, Size: 1788 bytes --]

From 70c5a88bfdad9da1f2732b82e8a9e3169fe20edd Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Wed, 19 Jan 2022 19:01:42 -0800
Subject: [PATCH 2/2] Don't use 'eshell-eval-using-options' for 'eshell/source'
 or 'eshell/.'

This makes 'source' and '.' in Eshell more compatible with regular
shells, which just treat the first argument as the file to source and
all subsequent arguments as arguments to that file.

* lisp/eshell/em-script.el (eshell/source, eshell/.): Don't use
'eshell-eval-using-options'.
---
 lisp/eshell/em-script.el | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/lisp/eshell/em-script.el b/lisp/eshell/em-script.el
index e8459513f3..e0bcd8b099 100644
--- a/lisp/eshell/em-script.el
+++ b/lisp/eshell/em-script.el
@@ -113,27 +113,13 @@ eshell-source-file
 
 (defun eshell/source (&rest args)
   "Source a file in a subshell environment."
-  (eshell-eval-using-options
-   "source" args
-   '((?h "help" nil nil "show this usage screen")
-     :show-usage
-     :usage "FILE [ARGS]
-Invoke the Eshell commands in FILE in a subshell, binding ARGS to $1,
-$2, etc.")
-   (eshell-source-file (car args) (cdr args) t)))
+  (eshell-source-file (car args) (cdr args) t))
 
 (put 'eshell/source 'eshell-no-numeric-conversions t)
 
 (defun eshell/. (&rest args)
   "Source a file in the current environment."
-  (eshell-eval-using-options
-   "." args
-   '((?h "help" nil nil "show this usage screen")
-     :show-usage
-     :usage "FILE [ARGS]
-Invoke the Eshell commands in FILE within the current shell
-environment, binding ARGS to $1, $2, etc.")
-   (eshell-source-file (car args) (cdr args))))
+  (eshell-source-file (car args) (cdr args)))
 
 (put 'eshell/. 'eshell-no-numeric-conversions t)
 
-- 
2.25.1