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

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

[Jim Porter]

[-- 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

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

[Eli Zaretskii]

> From: Jim Porter <jporterbugs@gmail.com>
> Date: Sat, 15 Jan 2022 20:03:46 -0800
> 
> 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.

OTOH, with the current behavior shell scripts that use options not
supported by Eshell commands will silently run unaltered, wheres with
your change they will error out.  Do we care about this?

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

[Jim Porter]

On 1/16/2022 1:04 AM, Eli Zaretskii wrote:
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Sat, 15 Jan 2022 20:03:46 -0800
>>
>> 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.
> 
> OTOH, with the current behavior shell scripts that use options not
> supported by Eshell commands will silently run unaltered, wheres with
> your change they will error out.  Do we care about this?

Yeah, that's one of the tricky parts about this. On the one hand, some 
options (boolean flags) can be silently ignored without altering the 
meaning of the command *too* much; on the other hand, some options 
(especially those taking an argument) can't be ignored without 
drastically changing the meaning. The latter case comes up with commands 
like "su -c CMD". In a regular shell, this will run CMD as root, but in 
Eshell, it silently ignores the "-c", so the command looks like "su 
CMD"; thus, it means "switch to the user named CMD (via the TRAMP su 
method)".

Since some options can be (relatively) safely ignored and others can't, 
maybe the best approach would be to audit the list of commands I posted 
in the original message to see if there are any options that users might 
specify that we can silently ignore. Then we can update the option spec 
for those commands to parse those options but not do anything with them.

Here are all the affected Eshell commands compared to their shell 
equivalents:

addpath
   No direct shell equivalent. Erroring out on unrecognized options
   should therefore be safe.

echo
   Eshell echo works quite a bit differently by default. From
   eshell/em-basic.el, it outputs "in a Lisp-friendly fashion (so that
   the value of echo is useful to a Lisp command using the result of
   echo as an argument)". However, with `eshell-plain-echo-behavior' set
   to non-nil, it works more like shell echo. I'm aware of 3 options for
   various implementations of shell echo:

   -n: suppress trailing newline
     This is recognized by Eshell echo, but actually does the opposite:
     it *inserts* a trailing newline. I posted a patch to help improve
     this to bug#27361.
   -e: enable interpretation of escape sequences
     This could probably be accepted by Eshell echo and ignored for now,
     though it would cause some behavioral differences. Warning or
     erroring out when seeing this option would also be reasonable, I
     think.
   -E: disable interpretation of escape sequences
     This is the default behavior for echo, so there's really no reason
     *not* to support this option.
   --version: display the version
     I doubt this option is used much in scripts, but it wouldn't be hard
     to support.

history
   Eshell history has some similar options to Bash history. It supports
   -r, -w, and -a to read, write, and append to the history file,
   respectively. It also supports getting the Nth history item via
   "history N". However, Bash history has some additional options:

   -c: clear history
     This could theoretically be accepted by Eshell history and ignored,
     but that would be pretty surprising. If a script is clearing
     history, there's probably a reason for it, so I think it's best to
     error out when seeing this option for now. However, it shouldn't be
     too hard to implement in Eshell if people want it.
   -d offset / -d start-end: delete history at offset or in range
     For the same reasons as -c, I think it's best to error out when
     seeing this option.
   -p args: perform history substitution on args
     Since this is supposed to show the result of a history substitution,
     I think it should error out so as not to cause any confusion or
     return an invalid result.
   -s: add history-substituted args to history list
     This is just an additional option to control the behavior of -p, so
     the same logic as above applies to this too.

source / .
   It seems Eshell's implementation of these actually has a bug! In a
   regular shell, running "source file.sh -a -b" would pass -a and -b to
   file.sh. However, Eshell's source command currently drops -a and -b
   silently. There are two ways I can think of to fix this:

   a) stop using `eshell-eval-using-options' entirely. This means that
      "source --help" changes from showing the help message to sourcing a
      file named "--help" (this is consistent with source in regular
      shells)
   b) using :parse-leading-options-only, which will stop looking for
      options once it sees a non-option argument like "file.sh".

su
   This (and sudo) both use the TRAMP methods of the same name, so there
   are probably pretty significant differences between the shell meaning
   and the Eshell meaning. I'm not super-familiar with how these TRAMP
   methods work though. Here are the options available:

   -c / --command CMD: run command CMD
   -s / --shell SHELL: run the specified shell
   --session-command CMD: run command CMD with no new session
     All of these take an argument that should affect what gets run by
     su. I think erroring out makes sense for all of them.
   -g / --group GROUP: specify a group
   -G / --supp-group GROUP: specify a supplemental group
   -w / --whitelist-environment LIST
     These are all options that take an argument and which users would
     probably expect to do what they say, since they affect security. I
     think erroring out makes sense for all of them.
   -f / --fast: pass -f to the shell
     Since the manual for su says this "may or may not be useful,
     depending on the shell", it would probably be ok to ignore. However,
     I'm not sure this is a very common option, so maybe erroring out is
     fine too.
   - / -l / --login: provide a login environment
     This is supported by Eshell's su command.
   -m / -p / --preserve-environment: preserve all env vars
     If a user wants to preserve the environment, we shouldn't just
     ignore that. However, since the shell su command ignores this when
     --login is set, we could ignore it then too. That's probably not
     particularly useful though. When would a user explicitly pass both
     --login and --preserve-environment?
   -P / --pty: create a pseudo-terminal
     I'm not sure what to do about this one. It might be reasonable to
     ignore this, but on the other hand, a user requesting a PTY probably
     has a reason for it and might be unpleasantly surprised if we just
     ignore that request.
   -V / --version: display version
     As with echo --version, I doubt this option is used much in scripts,
     but it wouldn't be hard to support.

sudo
   This is similar to the su command above, but sudo has a lot more
   options.

   -E / --preserve-env[=LIST]: preserve all/selected env vars
   -g / --group GROUP: specify a group
     These work like the corresponding su options and so I think erroring
     out is best here too.
   -A / --askpass: use a helper program to ask for the password
   -C / --close-from NUM: close file descriptors >= NUM
   -H / --set-home: set the HOME env var
   -K / --remove-timestamp: remove cached credentials
   -k / --reset-timestamp: invalidate cached credentials
   -P / --preserve-groups: preserve invoking user's group
   -r / --role ROLE: set SELinux role
   -t / --type TYPE: set SELinux type
   -v / --validate: validate cached credentials
     These all directly affect security, so we should be safe and error
     out instead of ignoring them.
   -B / --bell: ring the bell in the password prompt
     Since this is just a beep, it would probably be ok to ignore.
     However, I'm not sure this is a very common option, so maybe
     erroring out is fine too.
   -b / --background: run command in background
     It might be ok to ignore this, but I'm not sure what the
     implications of doing so would be. I'd err on the side of caution
     and report an error for this option.
   -e / --edit: edit a file instead of running command
   -i / --login: run the user's login shell
   -s / --shell: run the shell set in SHELL env var
     These all affect what command is actually run, so we should error
     out instead of silently running the wrong command.
   -h / --host HOST: run command on specified HOST
     This isn't supported, but unfortunately, we use "-h" to mean "help",
     which could result in confusing output. (sudo uses a "-h" without an
     argument for "help" and a -h with one for "host".) However, I don't
     think it will *do* anything invalid (aside from printing the help
     message), so it should be ok for now.
   -l / --list: list allowed/forbidden commands for user
   -u / --user USER: set user for --list
     These both drastically change what sudo does, so erroring out is the
     right thing to do, I think.
   -n / --non-interactive: don't prompt user
   -p / --prompt PROMPT: customize the password prompt
   -S / --stdin: read password from stdin
     I think these are mostly applicable to using sudo from a script, but
     I'm not sure there's an easy analogue for using them in Eshell. I
     don't know what to do about these.
   -V / --version: display version
     As with echo --version, I doubt this option is used much in scripts,
     but it wouldn't be hard to support.

umask
   Eshell's version of this is a bit limited in that it can only set the
   umask if you pass a numeric value. It also only supports the -S
   (symbolic format) option.

   -p: output in a form reusable as input
     I don't think Eshell's umask really supports this properly (in part
     because it can't accept symbol umasks as input), so it's probably
     good to error out in this case.

----------------------------------------

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.

However, I welcome other people's thoughts on this. Hopefully the audit 
above will be enough for people to make an informed opinion about the 
right thing to do.

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

[Jim Porter]

[-- 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

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

[Eli Zaretskii]

> From: Jim Porter <jporterbugs@gmail.com>
> Cc: 53293@debbugs.gnu.org
> Date: Wed, 19 Jan 2022 19:13:33 -0800
> 
> 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).

Thanks, LGTM.

> 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.

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

[Jim Porter]

[-- 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

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

[Lars Ingebrigtsen]

Jim Porter <jporterbugs@gmail.com> writes:

> 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.

Thanks; skimming the patch, it looks OK to me, and I've now pushed it to
Emacs 29.

-- 
(domestic pets only, the antidote for overdose, milk.)
   bloggy blog: http://lars.ingebrigtsen.no