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

On 1/5/2022 6:50 AM, Eli Zaretskii wrote:
>> Cc: 52999@debbugs.gnu.org
>> From: Jim Porter <jporterbugs@gmail.com>
>> Date: Tue, 4 Jan 2022 13:09:29 -0800
>>
>> +@item symbol
>> +This element is the name of the Lisp symbol that will be bound to
>> +@var{value}.
> 
> Is it a symbol or its name (a string)?  You say "name", but the
> example:
> 
>>                If @var{symbol} is @code{nil}, specifying this switch
> 
> uses a symbol, not its name.

Good catch. I've fixed this to say that it's the Lisp symbol.

>> +@item :preserve-args
>> +If present, do not pass @var{macro-args} through @code{flatten-tree}
>> +and @code{eshell-stringify-list}.
> 
> I think this should explain the effect of that, or the difference
> between using and not using this keyword.

I had to do a bit of digging to figure out what this keyword is supposed 
to do in practice. It seems that it's used when a built-in Eshell 
command wants to be able to accept arbitrary Lisp objects as arguments, 
instead of working with just a flat list of strings. I've added more 
detail to this paragraph.

>> +---
>> +** 'eshell-eval-using-options' now follows POSIX/GNU argument syntax conventions.
>> +This now accepts command-line options with values passed as a single
>     ^^^^
> "Eshell" instead of "This" will make it more clear what you mean.

Ok, I updated this to refer to "Built-in commands in Eshell".

Thanks for looking over the patch.

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

From 45f48041c64915b4c05024b426c163bb419d083d 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              | 120 ++++++++++++++++++++++++
 etc/NEWS                          |   6 ++
 lisp/eshell/esh-opt.el            |  90 ++++++++++++------
 test/lisp/eshell/esh-opt-tests.el | 151 ++++++++++++++++++++++--------
 4 files changed, 298 insertions(+), 69 deletions(-)

diff --git a/doc/misc/eshell.texi b/doc/misc/eshell.texi
index a87dd4308c..d72ca5afe9 100644
--- a/doc/misc/eshell.texi
+++ b/doc/misc/eshell.texi
@@ -694,6 +694,126 @@ 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 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
+Normally, @code{eshell-eval-using-options} flattens the list of
+arguments in @var{macro-args} and converts each to a string.  If this
+keyword is present, avoid doing that, instead preserving the original
+arguments.  This is useful for commands which want to accept arbitrary
+Lisp objects.
+
+@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..e6bcbf5a86 100644
--- a/etc/NEWS
+++ b/etc/NEWS
@@ -1060,6 +1060,12 @@ 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.
+Built-in commands in Eshell now accept 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