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

[Jim Porter <jporterbugs@gmail.com>]

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

On 1/3/2022 5:36 PM, Jim Porter wrote:
> I've attached a patch with tests to fix this and use the POSIX/GNU 
> argument conventions, supporting both the above cases.

One small addition here. I just noticed that `eshell-eval-using-options' 
already supports "--" to terminate all the options, so I added a test 
case for that too.

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

From de20c52f7669367479c3c4762cb8326f9a2058b3 Mon Sep 17 00:00:00 2001
From: Jim Porter <jporterbugs@gmail.com>
Date: Mon, 3 Jan 2022 21:30:59 -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.

* etc/NEWS: Announce the change.
---
 etc/NEWS                          |   3 +
 lisp/eshell/esh-opt.el            |  90 ++++++++++++------
 test/lisp/eshell/esh-opt-tests.el | 149 ++++++++++++++++++++++--------
 3 files changed, 174 insertions(+), 68 deletions(-)

diff --git a/etc/NEWS b/etc/NEWS
index ca6a716ccd..b595d99633 100644
--- 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.
+
 ** 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..235e03bced 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" "/dev/null")
+   '((?a "all" nil show-all
+         "do not ignore entries starting with ."))
+   (should (eq show-all t))
+   (should (equal args '("/dev/null"))))
   (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" '("/dev/null")
+   '((?a "all" nil show-all
+         "do not ignore entries starting with ."))
+   (should (eq show-all nil))
+   (should (equal args '("/dev/null"))))
 
+  ;; 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" "/dev/null")
+   '((?a "all" nil show-all
+         "do not ignore entries starting with ."))
+   (should (eq show-all t))
+   (should (equal args '("/dev/null"))))
+
+  ;; 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" '("/dev/null" "-h")
+   '((?h "human-readable" 1024 human-readable
+         "print sizes in human readable format"))
+   (should (eql human-readable 1024))
+   (should (equal args '("/dev/null"))))
+  (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))
+   (should (equal args '("/dev/null"))))
+  (eshell-eval-using-options
+   "ls" '("/dev/null")
+   '((?h "human-readable" 1024 human-readable
+         "print sizes in human readable format"))
+   (should (eq human-readable nil))
+   (should (equal args '("/dev/null"))))
 
+  ;; Test options with user-specified values.
   (eshell-eval-using-options
    "ls" '("-I" "*.txt" "/dev/null")
    '((?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 '("/dev/null"))))
+  (eshell-eval-using-options
+   "ls" '("-I*.txt" "/dev/null")
+   '((?I "ignore" t ignore-pattern
+         "do not list implied entries matching pattern"))
+   (should (equal ignore-pattern "*.txt"))
+   (should (equal args '("/dev/null"))))
+  (eshell-eval-using-options
+   "ls" '("--ignore" "*.txt" "/dev/null")
+   '((?I "ignore" t ignore-pattern
+         "do not list implied entries matching pattern"))
+   (should (equal ignore-pattern "*.txt"))
+   (should (equal args '("/dev/null"))))
+  (eshell-eval-using-options
+   "ls" '("--ignore=*.txt" "/dev/null")
+   '((?I "ignore" t ignore-pattern
+         "do not list implied entries matching pattern"))
+   (should (equal ignore-pattern "*.txt"))
+   (should (equal args '("/dev/null"))))
 
+  ;; 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" "/dev/null")
+   '((?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 '("/dev/null"))))
   (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" "/dev/null")
+   '((?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 '("/dev/null"))))
 
+  ;; 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