bug#52999: 29.0.50; [PATCH v3] `eshell-eval-using-options' should follow POSIX/GNU argument conventions

[Jim Porter <jporterbugs@gmail.com>]

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

Thanks, I've attached an updated patch.

On 1/4/2022 5:01 AM, Eli Zaretskii wrote:
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Mon, 3 Jan 2022 21:33:28 -0800
>>
>> --- a/etc/NEWS
>> +++ b/etc/NEWS
>> @@ -1060,6 +1060,9 @@ dimensions.
>>   Specifying a cons as the from argument allows to start measuring text
>>   from a specified amount of pixels above or below a position.
>>   
>> +---
>> +** 'eshell-eval-using-options' now follows POSIX/GNU argument syntax conventions.
> 
> This is too terse: we cannot assume that everyone knows what does
> "POSIX/GNU argument syntax conventions" stand for.  Especially since
> you didn't even say "command-line arguments", just "arguments".
> Please make the entry more informative.

Ok, I added a brief description explaining what specifically has been 
changed.

> And why don't people who propose and install changes in Eshell also
> update the Eshell manual?  That the manual in its current shape leaves
> a lot to be desired is not a justification to leave it that way.  We
> will never improve that manual unless we start adding useful stuff to
> it, one small piece at a time.

I just wasn't sure if `eshell-eval-using-options' should be in the 
manual or not. Thinking it over a bit more, it would have helped me if 
it had been in the manual (I encountered this bug while trying to write 
my own Eshell built-in command), so I added some info about it to the 
manual, mostly adapted from the docstring for 
`eshell-eval-using-options'. Hopefully I followed the right conventions 
here; I'm only vaguely familiar with the Texinfo format.

>>   (ert-deftest test-eshell-eval-using-options ()
>>     "Tests for `eshell-eval-using-options'."
>> +  ;; Test short options.
>>     (eshell-eval-using-options
>> -   "sudo" '("-u" "root" "whoami")
>> -   '((?u "user" t user "execute a command as another USER")
>> -     :parse-leading-options-only)
>> -   (should (equal user "root")))
>> +   "ls" '("-a" "/dev/null")
>> +   '((?a "all" nil show-all
>> +         "do not ignore entries starting with ."))
>> +   (should (eq show-all t))
>> +   (should (equal args '("/dev/null"))))
> 
> Can these tests be made less platform-specific?  For example, not all
> the supported platforms have /dev/null, and we have a portable
> abstraction for it.

They should actually work cross-platform, since the tests don't invoke 
the commands at all; they just make sure that 
`eshell-eval-using-options' can parse the switches correctly. To make 
this a bit clearer though, I replaced "/dev/null" with "/some/path". 
Hopefully when people see that, they'll understand that this is a "fake" 
path not corresponding to anything on the actual filesystem.

>> +   "sudo" '("-u" "root" "whoami")
>> +   '((?u "user" t user "execute a command as another USER")
>> +     :parse-leading-options-only)
>> +   (should (equal user "root"))
>> +   (should (equal args '("whoami"))))
[snip]
> 
> And here, sudo and whoami don't necessarily exist, so something should
> be done about that, I think.

The same applies here; the commands aren't actually invoked, so they 
could just as easily be named "foo" and "bar". I think the reason for 
them looking like real commands is just so that a reader can glance at 
them and understand more-readily what the expected result is. Readers 
are likely to be familiar with "sudo" and "whoami", but wouldn't have 
any preconceptions about the semantics of (fake) commands named "foo" 
and "bar". If you still think it's a problem, I can change it though.

(Also, technically, both of those commands should always exist in 
Eshell, since they're defined as built-in commands. "sudo" runs the 
TRAMP sudo method, and "whoami" is a TRAMP-aware Lisp implementation.)

[-- Attachment #2: 0001-Follow-POSIX-GNU-argument-conventions-for-eshell-eva.patch --]
[-- Type: text/plain, Size: 18573 bytes --]

From 85a8e2df855f824c18adc70a4968bd3ae26f558d Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Tue, 4 Jan 2022 12:58:38 -0800
Subject: [PATCH] Follow POSIX/GNU argument conventions for
 'eshell-eval-using-options'

* lisp/eshell/esh-opt.el (eshell--split-switch): New function.
(eshell-set-option): Allow setting a supplied value instead of always
consuming from 'eshell--args'.
(eshell--process-option): Support consuming option values specified as
a single token.
(eshell--process-args): For short options, pass full switch token to
'eshell--process-option'.

* test/lisp/eshell/esh-opt-tests.el (esh-opt-process-args-test): Fix
test.
(test-eshell-eval-using-options): Add tests for various types of
options.

* doc/misc/eshell.texi (Defining new built-in commands): New
subsection, describe how to use 'eshell-eval-using-options'.

* etc/NEWS: Announce the change.
---
 doc/misc/eshell.texi              | 117 +++++++++++++++++++++++
 etc/NEWS                          |   5 +
 lisp/eshell/esh-opt.el            |  90 ++++++++++++------
 test/lisp/eshell/esh-opt-tests.el | 151 ++++++++++++++++++++++--------
 4 files changed, 294 insertions(+), 69 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index a87dd4308c..1bc1fcbe9c 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -694,6 +694,123 @@ Built-ins
 supports Tramp.
 @end table
 
+@subsection Defining new built-in commands
+While Eshell can run Lisp functions directly as commands, it may be
+more convenient to provide a special built-in command for
+Eshell.  Built-in commands are just ordinary Lisp functions designed
+to be called from Eshell.  When defining an Eshell-specific version of
+an existing function, you can give that function a name starting with
+@code{eshell/} so that Eshell knows to use it.
+
+@defmac eshell-eval-using-options name macro-args options body@dots{}
+This macro processes a list of @var{macro-args} for the command
+@var{name} using a set of command line @var{options}.  If the
+arguments are parsed successfully, it will store the resulting values
+in local symbols and execute @var{body}; any remaining arguments will
+be available in the locally let-bound variable @code{args}.  The
+return value is the value of the last form in @var{body}.
+
+If an unknown option was passed in @var{macro-args} and an external
+command was specified (see below), this macro will start a process for
+that command and throw the tag @code{eshell-external} with the new
+process as its value.
+
+@var{options} should be a list beginning with one or more elements of
+the following form, with each element representing a particular
+command-line switch:
+
+@example
+(@var{short} @var{long} @var{value} @var{symbol} @var{help-string})
+@end example
+
+@table @var
+@item short
+This element, if non-nil, should be a character to be used as a short
+switch, like @code{-@var{short}}.  At least one of this element and
+@var{long} must be non-nil.
+
+@item long
+This element, if non-nil, should be a string to be used as a long
+switch, like @code{--@var{long}}.
+
+@item value
+This element is the value associated with the option.  It can be
+either:
+
+@table @asis
+@item @code{t}
+The option needs a value to be specified after the switch.
+
+@item @code{nil}
+The option is given the value @code{t}.
+
+@item anything else
+The option is given the specified value.
+@end table
+
+@item symbol
+This element is the name of the Lisp symbol that will be bound to
+@var{value}.  If @var{symbol} is @code{nil}, specifying this switch
+will instead call @code{eshell-show-usage}, and so is appropriate for
+an option like @code{--help}.
+
+@item help-string
+This element is a documentation string for the option, which will be
+displayed when @code{eshell-show-usage} is invoked.
+@end table
+
+After the list of command-line switch elements, @var{options} can
+include additional keyword arguments to control how
+@code{eshell-eval-using-options} behaves.  Some of these take
+arguments, while others don't.  The recognized keywords are:
+
+@table @code
+@item :external @var{string}
+Specify @var{string} as an external command to run if there are
+unknown switches in @var{macro-args}.
+
+@item :usage @var{string}
+Set @var{string} as the initial part of the command's documentation
+string.  It appears before the options are listed.
+
+@item :post-usage @var{string}
+Set @var{string} to be the (optional) trailing part of the command's
+documentation string.  It appears after the list of options, but
+before the final part of the documentation about the associated
+external command, if there is one.
+
+@item :show-usage
+If present, then show the usage message if the command is called with
+no arguments.
+
+@item :preserve-args
+If present, do not pass @var{macro-args} through @code{flatten-tree}
+and @code{eshell-stringify-list}.
+
+@item :parse-leading-options-only
+If present, do not parse dash or switch arguments after the first
+positional argument.  Instead, treat them as positional arguments
+themselves.
+@end table
+
+For example, you could handle a subset of the options for the
+@code{ls} command like this:
+
+@example
+(eshell-eval-using-options
+ "ls" macro-args
+ '((?a  nil      nil show-all       "show all files")
+   (?I  "ignore" t   ignore-pattern "ignore files matching pattern")
+   (nil "help"   nil nil            "show this help message")
+ :external "ls"
+ :usage "[OPTION]... [FILE]...
+  List information about FILEs (the current directory by default).")
+ ;; List the files in ARGS somehow...
+ )
+@end example
+
+@end defmac
+
 @subsection Built-in variables
 Eshell knows a few built-in variables:
 
diff --git a/etc/NEWS b/etc/NEWS
index ca6a716ccd..cf392ca47e 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1060,6 +1060,11 @@ dimensions.
 Specifying a cons as the from argument allows to start measuring text
 from a specified amount of pixels above or below a position.
 
+---
+** 'eshell-eval-using-options' now follows POSIX/GNU argument syntax conventions.
+This now accepts command-line options with values passed as a single
+token, such as '-oVALUE' or '--option=VALUE'.
+
 ** XDG support
 
 *** New function 'xdg-state-home' returns 'XDG_STATE_HOME' environment variable.
diff --git a/lisp/eshell/esh-opt.el b/lisp/eshell/esh-opt.el
index 7d31845528..fcc35780e9 100644
--- a/lisp/eshell/esh-opt.el
+++ b/lisp/eshell/esh-opt.el
@@ -187,49 +187,82 @@ eshell-show-usage
 will be called instead." extcmd)))))
     (throw 'eshell-usage usage)))
 
-(defun eshell--set-option (name ai opt options opt-vals)
+(defun eshell--split-switch (switch kind)
+  "Split SWITCH into its option name and potential value, if any.
+KIND should be the integer 0 if SWITCH is a short option, or 1 if it's
+a long option."
+  (if (eq kind 0)
+      ;; Short option
+      (cons (aref switch 0)
+            (and (> (length switch) 1) (substring switch 1)))
+    ;; Long option
+    (save-match-data
+      (string-match "\\([^=]*\\)\\(?:=\\(.*\\)\\)?" switch)
+      (cons (match-string 1 switch) (match-string 2 switch)))))
+
+(defun eshell--set-option (name ai opt value options opt-vals)
   "Using NAME's remaining args (index AI), set the OPT within OPTIONS.
-If the option consumes an argument for its value, the argument list
-will be modified."
+VALUE is the potential value of the OPT, coming from args like
+\"-fVALUE\" or \"--foo=VALUE\", or nil if no value was supplied.  If
+OPT doesn't consume a value, return VALUE unchanged so that it can be
+processed later; otherwsie, return nil.
+
+If the OPT consumes an argument for its value and VALUE is nil, the
+argument list will be modified."
   (if (not (nth 3 opt))
       (eshell-show-usage name options)
-    (setcdr (assq (nth 3 opt) opt-vals)
-            (if (eq (nth 2 opt) t)
-                (if (> ai (length eshell--args))
-                    (error "%s: missing option argument" name)
-                  (pop (nthcdr ai eshell--args)))
-              (or (nth 2 opt) t)))))
+    (if (eq (nth 2 opt) t)
+        (progn
+          (setcdr (assq (nth 3 opt) opt-vals)
+                  (or value
+                      (if (> ai (length eshell--args))
+                          (error "%s: missing option argument" name)
+                        (pop (nthcdr ai eshell--args)))))
+          nil)
+      (setcdr (assq (nth 3 opt) opt-vals)
+              (or (nth 2 opt) t))
+      value)))
 
 (defun eshell--process-option (name switch kind ai options opt-vals)
   "For NAME, process SWITCH (of type KIND), from args at index AI.
 The SWITCH will be looked up in the set of OPTIONS.
 
-SWITCH should be either a string or character.  KIND should be the
-integer 0 if it's a character, or 1 if it's a string.
-
-The SWITCH is then be matched against OPTIONS.  If no matching 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."
-  (let* ((opts options)
-	 found)
+SWITCH should be a string starting with the option to process,
+possibly followed by its value, e.g. \"u\" or \"uUSER\".  KIND should
+be the integer 0 if it's a short option, or 1 if it's a long option.
+
+The SWITCH is then be matched against OPTIONS.  If KIND is 0 and the
+SWITCH matches an option that doesn't take a value, return the
+remaining characters in SWITCH to be processed later as further short
+options.
+
+If no matching 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."
+  (let ((switch (eshell--split-switch switch kind))
+        (opts options)
+	found remaining)
     (while opts
       (if (and (listp (car opts))
-               (nth kind (car opts))
-               (equal switch (nth kind (car opts))))
+               (equal (car switch) (nth kind (car opts))))
 	  (progn
-	    (eshell--set-option name ai (car opts) options opt-vals)
+	    (setq remaining (eshell--set-option name ai (car opts)
+                                                (cdr switch) options opt-vals))
+            (when (and remaining (eq kind 1))
+              (error "%s: option --%s doesn't allow an argument"
+                     name (car switch)))
 	    (setq found t opts nil))
 	(setq opts (cdr opts))))
-    (unless found
+    (if found
+        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 switch) "%s: unrecognized option -%c"
+            (error (if (characterp (car switch)) "%s: unrecognized option -%c"
                      "%s: unrecognized option --%s")
-                   name switch)))))))
+                   name (car switch))))))))
 
 (defun eshell--process-args (name args options)
   "Process the given ARGS using OPTIONS."
@@ -262,12 +295,9 @@ eshell--process-args
 	      (if (> (length switch) 0)
 		  (eshell--process-option name switch 1 ai options opt-vals)
 		(setq ai (length eshell--args)))
-	    (let ((len (length switch))
-		  (index 0))
-	      (while (< index len)
-		(eshell--process-option name (aref switch index)
-                                        0 ai options opt-vals)
-		(setq index (1+ index))))))))
+	      (while (> (length switch) 0)
+		(setq switch (eshell--process-option name switch 0
+                                                     ai options opt-vals)))))))
     (nconc (mapcar #'cdr opt-vals) eshell--args)))
 
 (provide 'esh-opt)
diff --git a/test/lisp/eshell/esh-opt-tests.el b/test/lisp/eshell/esh-opt-tests.el
index e2a0ea59d1..c13f5e15b3 100644
--- a/test/lisp/eshell/esh-opt-tests.el
+++ b/test/lisp/eshell/esh-opt-tests.el
@@ -57,7 +57,7 @@ esh-opt-process-args-test
            '((?u "user" t user "execute a command as another USER")
              :parse-leading-options-only))))
   (should
-   (equal '("world" "emerge")
+   (equal '("DN" "emerge" "world")
           (eshell--process-args
            "sudo"
            '("-u" "root" "emerge" "-uDN" "world")
@@ -65,59 +65,132 @@ esh-opt-process-args-test
 
 (ert-deftest test-eshell-eval-using-options ()
   "Tests for `eshell-eval-using-options'."
+  ;; Test short options.
   (eshell-eval-using-options
-   "sudo" '("-u" "root" "whoami")
-   '((?u "user" t user "execute a command as another USER")
-     :parse-leading-options-only)
-   (should (equal user "root")))
+   "ls" '("-a" "/some/path")
+   '((?a "all" nil show-all
+         "do not ignore entries starting with ."))
+   (should (eq show-all t))
+   (should (equal args '("/some/path"))))
   (eshell-eval-using-options
-   "sudo" '("--user" "root" "whoami")
-   '((?u "user" t user "execute a command as another USER")
-     :parse-leading-options-only)
-   (should (equal user "root")))
+   "ls" '("/some/path")
+   '((?a "all" nil show-all
+         "do not ignore entries starting with ."))
+   (should (eq show-all nil))
+   (should (equal args '("/some/path"))))
 
+  ;; Test long options.
   (eshell-eval-using-options
-   "sudo" '("emerge" "-uDN" "world")
-   '((?u "user" t user "execute a command as another USER"))
-   (should (equal user "world")))
+   "ls" '("--all" "/some/path")
+   '((?a "all" nil show-all
+         "do not ignore entries starting with ."))
+   (should (eq show-all t))
+   (should (equal args '("/some/path"))))
+
+  ;; Test options with constant values.
   (eshell-eval-using-options
-   "sudo" '("emerge" "-uDN" "world")
-   '((?u "user" t user "execute a command as another USER")
-     :parse-leading-options-only)
-   (should (eq user nil)))
+   "ls" '("/some/path" "-h")
+   '((?h "human-readable" 1024 human-readable
+         "print sizes in human readable format"))
+   (should (eql human-readable 1024))
+   (should (equal args '("/some/path"))))
+  (eshell-eval-using-options
+   "ls" '("/some/path" "--human-readable")
+   '((?h "human-readable" 1024 human-readable
+         "print sizes in human readable format"))
+   (should (eql human-readable 1024))
+   (should (equal args '("/some/path"))))
+  (eshell-eval-using-options
+   "ls" '("/some/path")
+   '((?h "human-readable" 1024 human-readable
+         "print sizes in human readable format"))
+   (should (eq human-readable nil))
+   (should (equal args '("/some/path"))))
 
+  ;; Test options with user-specified values.
+  (eshell-eval-using-options
+   "ls" '("-I" "*.txt" "/some/path")
+   '((?I "ignore" t ignore-pattern
+         "do not list implied entries matching pattern"))
+   (should (equal ignore-pattern "*.txt"))
+   (should (equal args '("/some/path"))))
+  (eshell-eval-using-options
+   "ls" '("-I*.txt" "/some/path")
+   '((?I "ignore" t ignore-pattern
+         "do not list implied entries matching pattern"))
+   (should (equal ignore-pattern "*.txt"))
+   (should (equal args '("/some/path"))))
   (eshell-eval-using-options
-   "ls" '("-I" "*.txt" "/dev/null")
+   "ls" '("--ignore" "*.txt" "/some/path")
    '((?I "ignore" t ignore-pattern
-	 "do not list implied entries matching pattern"))
-   (should (equal ignore-pattern "*.txt")))
+         "do not list implied entries matching pattern"))
+   (should (equal ignore-pattern "*.txt"))
+   (should (equal args '("/some/path"))))
+  (eshell-eval-using-options
+   "ls" '("--ignore=*.txt" "/some/path")
+   '((?I "ignore" t ignore-pattern
+         "do not list implied entries matching pattern"))
+   (should (equal ignore-pattern "*.txt"))
+   (should (equal args '("/some/path"))))
 
+  ;; Test multiple short options in a single token.
   (eshell-eval-using-options
-   "ls" '("-l" "/dev/null")
-   '((?l nil long-listing listing-style
-	 "use a long listing format"))
-   (should (eql listing-style 'long-listing)))
+   "ls" '("-al" "/some/path")
+   '((?a "all" nil show-all
+         "do not ignore entries starting with .")
+     (?l nil long-listing listing-style
+         "use a long listing format"))
+   (should (eq t show-all))
+   (should (eql listing-style 'long-listing))
+   (should (equal args '("/some/path"))))
   (eshell-eval-using-options
-   "ls" '("/dev/null")
-   '((?l nil long-listing listing-style
-	 "use a long listing format"))
-   (should (eq listing-style nil)))
+   "ls" '("-aI*.txt" "/some/path")
+   '((?a "all" nil show-all
+         "do not ignore entries starting with .")
+     (?I "ignore" t ignore-pattern
+         "do not list implied entries matching pattern"))
+   (should (eq t show-all))
+   (should (equal ignore-pattern "*.txt"))
+   (should (equal args '("/some/path"))))
 
+  ;; Test that "--" terminates options.
   (eshell-eval-using-options
-   "ls" '("/dev/null" "-h")
-   '((?h "human-readable" 1024 human-readable
-	 "print sizes in human readable format"))
-   (should (eql human-readable 1024)))
+   "ls" '("--" "-a")
+   '((?a "all" nil show-all
+         "do not ignore entries starting with ."))
+   (should (eq show-all nil))
+   (should (equal args '("-a"))))
   (eshell-eval-using-options
-   "ls" '("/dev/null" "--human-readable")
-   '((?h "human-readable" 1024 human-readable
-	 "print sizes in human readable format"))
-   (should (eql human-readable 1024)))
+   "ls" '("--" "--all")
+   '((?a "all" nil show-all
+         "do not ignore entries starting with ."))
+   (should (eq show-all nil))
+   (should (equal args '("--all"))))
+
+  ;; Test :parse-leading-options-only.
   (eshell-eval-using-options
-   "ls" '("/dev/null")
-   '((?h "human-readable" 1024 human-readable
-	 "print sizes in human readable format"))
-   (should (eq human-readable nil))))
+   "sudo" '("-u" "root" "whoami")
+   '((?u "user" t user "execute a command as another USER")
+     :parse-leading-options-only)
+   (should (equal user "root"))
+   (should (equal args '("whoami"))))
+  (eshell-eval-using-options
+   "sudo" '("--user" "root" "whoami")
+   '((?u "user" t user "execute a command as another USER")
+     :parse-leading-options-only)
+   (should (equal user "root"))
+   (should (equal args '("whoami"))))
+  (eshell-eval-using-options
+   "sudo" '("emerge" "-uDN" "world")
+   '((?u "user" t user "execute a command as another USER"))
+   (should (equal user "DN"))
+   (should (equal args '("emerge" "world"))))
+  (eshell-eval-using-options
+   "sudo" '("emerge" "-uDN" "world")
+   '((?u "user" t user "execute a command as another USER")
+     :parse-leading-options-only)
+   (should (eq user nil))
+   (should (equal args '("emerge" "-uDN" "world")))))
 
 (provide 'esh-opt-tests)
 
-- 
2.25.1