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

On 1/20/2022 3:48 AM, Eli Zaretskii wrote:
>> From: Jim Porter <jporterbugs@gmail.com>
>> Cc: 53293@debbugs.gnu.org
>> Date: Wed, 19 Jan 2022 19:13:33 -0800
>>
>> 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.
> 
> I think a NEWS entry is called for, since the change in behavior is
> not backward-compatible.

Thanks. Here are updated patches with a NEWS entry to explain the change 
to `source' and `.'. I also rebased the first patch to fix a merge 
conflict with the patch for bug#27361.

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

From af089c16456cd8ec088c166ebb22170a88193417 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           | 13 +++--
 lisp/eshell/esh-opt.el            | 12 ++---
 test/lisp/eshell/esh-opt-tests.el | 86 +++++++++++++++++++++++--------
 3 files changed, 79 insertions(+), 32 deletions(-)

diff --git a/lisp/eshell/em-basic.el b/lisp/eshell/em-basic.el
index d3b15c900b..ba868cee59 100644
--- a/lisp/eshell/em-basic.el
+++ b/lisp/eshell/em-basic.el
@@ -113,11 +113,16 @@ eshell/echo
   "Implementation of `echo'.  See `eshell-plain-echo-behavior'."
   (eshell-eval-using-options
    "echo" args
-   '((?n nil (nil) output-newline "do not output the trailing newline")
-     (?N nil (t)   output-newline "terminate with a newline")
-     (?h "help" nil nil "output this help screen")
+   '((?n nil (nil) output-newline
+         "do not output the trailing newline")
+     (?N nil (t)   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 | -N] [object]")
+     :usage "[OPTION]... [OBJECT]...")
    (if eshell-plain-echo-behavior
        (eshell-echo args (if output-newline (car output-newline) t))
      ;; In Emacs 28.1 and earlier, "-n" was used to add a newline to
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: 2392 bytes --]

From 3631beaddf2ff7869cba5dae6f0e9b4991e7e3d6 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Thu, 20 Jan 2022 19:51:39 -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'.

* etc/NEWS: Announce the change.
---
 etc/NEWS                 |  5 +++++
 lisp/eshell/em-script.el | 18 ++----------------
 2 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index ae4bf7e4d3..b4b66bec0e 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -104,6 +104,11 @@ files that were compiled with an old EIEIO (Emacs<25).
 ** 'C-x 8 .' has been moved to 'C-x 8 . .'.
 This is to open up the 'C-x 8 .' map to bind further characters there.
 
+---
+** 'source' and '.' in Eshell no longer accept the '--help' option.
+This is for compatibility with the shell versions of these commands,
+which don't handle options like '--help' in any special way.
+
 \f
 * Changes in Emacs 29.1
 
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