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: 2273 bytes --]

Currently, `eshell-eval-using-options' reports an error if the user 
passes an unrecognized option, but only if 1) the :external keyword has 
been set and 2) the external program can't actually be found. If you'd 
like to see this in action, you can run "echo -Z hi" in Eshell. It just 
silently discards the "-Z". Editing `eshell/echo' to include `:external 
"nonexist"' in its option spec instead results in an error being 
reported to the user.

I think this might be simply an oversight in the implementation. The 
documentation of `eshell--process-option' states:

   If no matching [option] handler is found, and an :external command is
   defined (and available), it will be called; otherwise, an error will
   be triggered to say that the switch is unrecognized.

I would interpret this to mean the following:

   (if (and (not handler-found)
            external-cmd-available)
       (call-external)
     (error "unrecognized option"))

Attached is a patch that ensures unrecognized options report an error 
even when there's no :external command. This *is* a slightly 
incompatible change though. The following Eshell commands use 
`eshell-eval-using-options' with no :external command, so they'll start 
erroring out if you pass unrecognized arguments to them with this patch:

   addpath
   echo
   history
   source / .
   su
   sudo
   umask

Despite this incompatibility, I still think this is the right change to 
make for all these commands. If a user passes unrecognized options to 
any of these, they should be informed of that fact. For example, `su' is 
used in Eshell to invoke TRAMP's su method. It would likely be an 
unpleasant surprise for a user if they tried to pass flags to it that 
only work with /bin/su, only for those options to be silently ignored. 
One of the nastiest parts of the pre-patch behavior is that something 
like "su -c CMD" simply drops the "-c", which results in CMD being 
treated as the USER instead.

I'm not sure if this should be explicitly called out in the manual or 
whether it warrants a NEWS entry. To me, it just seems like a bug, and 
one that was already documented as working the way it does with this 
patch applied. That said, if others think this warrants some more 
documentation, I'm happy to add some.

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

From d99d0f5fc155c450acc93a70798898aa9a13e0d7 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 6 Jan 2022 18:06:06 -0800
Subject: [PATCH] Raise an error from 'eval-eval-using-options' for unknown
 options

* 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/esh-opt.el            | 12 ++---
 test/lisp/eshell/esh-opt-tests.el | 86 +++++++++++++++++++++++--------
 2 files changed, 70 insertions(+), 28 deletions(-)

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