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

From: [email-masked] (GNU bug Tracking System)
Date: Wed, 12 Jan 2022 15:01:02 +0000

Your bug report

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

which was filed against the emacs package, has been closed.

The explanation is attached below, along with your original report.
If you require more details, please reply to [email-masked].

-- 
52999: http://debbugs.gnu.org/cgi/bugreport.cgi?bug=52999
GNU Bug Tracking System
Contact [email-masked] with problems

Subject: Re: bug#52999: 29.0.50; [PATCH v3] `eshell-eval-using-options' should follow POSIX/GNU argument conventions
From: Eli Zaretskii <[email-masked]>
To: Jim Porter <[email-masked]>
Cc: [email-masked]
Date: Wed, 12 Jan 2022 17:00:29 +0200
List-Id: <debbugs-submit.debbugs.gnu.org>

> Cc: [email-masked]
> From: Jim Porter <[email-masked]>
> Date: Wed, 5 Jan 2022 16:48:39 -0800
> 
> Ok, I updated this to refer to "Built-in commands in Eshell".
> 
> Thanks for looking over the patch.

Thanks, I installed this, and I'm closing the bug.

Subject: 29.0.50; [PATCH] `eshell-eval-using-options' should follow POSIX/GNU argument conventions
From: Jim Porter <[email-masked]>
To: [email-masked]
Date: Mon, 3 Jan 2022 17:36:06 -0800
List-Id: <debbugs-submit.debbugs.gnu.org>

Currently, `eshell-eval-using-options' doesn't follow POSIX/GNU argument 
conventions[1], resulting in some confusing behavior. To see this in 
action, the easiest way is probably to make a small patch to 
`eshell-do-ls' in lisp/eshell/em-ls.el: just comment out the line that says,

   :external "ls"

and then eval the function again. (This is necessary so that eshell 
doesn't fall back to the system's `ls' command when it gets confused.) 
Then open `eshell' and run:

   ls '-I*.el'

Instead of what you'd expect (a directory listing that ignores Emacs 
Lisp files), instead you get a directory listing of *all* the files in 
the long listing format. That's because `eshell-eval-using-options' 
assumes that all the characters after the "-" are names of short 
options, rather than a single short option followed by its value. You 
can see a similar problem with:

   ls '--ignore=*.el'

In this case, `eshell-eval-using-options' looks for an option named 
"ignore=*.el" instead of an option named "ignore" followed by its value.

I've attached a patch with tests to fix this and use the POSIX/GNU 
argument conventions, supporting both the above cases. However, I should 
mention that this is a slightly incompatible change. A small number of 
existing eshell commands work like `ls -I', and their behavior is now a 
bit different. Previously, you could do the following,

   ls -Ia '*.el'

to list all the files in a directory, excluding Emacs Lisp files. Now, 
you have to spell that as:

   ls -aI '*.el'
   # or...
   ls -aI'*.el'

I think that's ok though, since I can't imagine anyone *wanting* the old 
behavior. It surprised me quite a bit when I stumbled across it, and 
worse, it only crops up sometimes, since eshell transparently falls back 
to the real commands when it gets confused.

For completeness, the following commands+options are affected:

   sudo -u
   du -d
   ls -I

[1] https://www.gnu.org/software/libc/manual/html_node/Argument-Syntax.html
From 9fcb2389cc9828be59c350bf02b578855c867ad3 Mon Sep 17 00:00:00 2001
From: Jim Porter <[email-masked]>
Date: Mon, 3 Jan 2022 17:28:00 -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 | 131 ++++++++++++++++++++++--------
 3 files changed, 158 insertions(+), 66 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..f9203bd92a 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,118 @@ 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")))
+   (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
+   "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 (eql listing-style 'long-listing)))
+   (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 :parse-leading-options-only.
   (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)))
+   "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
-   "ls" '("/dev/null" "--human-readable")
-   '((?h "human-readable" 1024 human-readable
-	 "print sizes in human readable format"))
-   (should (eql human-readable 1024)))
+   "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
-   "ls" '("/dev/null")
-   '((?h "human-readable" 1024 human-readable
-	 "print sizes in human readable format"))
-   (should (eq human-readable nil))))
+   "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