bug#28323: Patchset to fix 28323

[Jay Kamat]

Hi Noam,

Noam Postavsky <npostavs@gmail.com> writes:

>> I fixed the blocking bug first in a separate patch, and then added a new
>> parameter for commands like sudo, which would prefer :raw-positional
>> arguments after the first non flag argument. I'm still not sure if this
>> is the best name for this idea, so if anyone has a better name I'd be
>> happy to change it!
>
> :parse-leading-options-only?  Perhaps that's too long though.

This sounds better to me, it's more descriptive even if it's longer, so
I'm going to rename it.

> I think you should be able to just terminate the loop here once you have
> pos-argument-found and (memq :raw-positional options) is true, rather
> than passing an arg to eshell--process-option to make the rest of the
> loop iterations into nops.

This approach is a lot cleaner, thanks for noticing that! :)

>
>> -        (setcdr (nthcdr (1- ai) eshell--args)
>> +            (setcdr (nthcdr (1- ai) eshell--args)
>
> This is just a whitespace change, right?

Whoops, my mistake! Fixing.

Thanks for the review, my updated patches are below.

There's no rush for this, so if you want to hold off on reviewing this
until after the imminent Emacs release, I'm fine with waiting.

-Jay


>From d6fe0506dec8856242f2c8fb4e38d267cd8bcfee Mon Sep 17 00:00:00 2001
From: Jay Kamat <jaygkamat@gmail.com>
Date: Tue, 8 May 2018 12:04:00 -0700
Subject: [PATCH 1/2] esh-opt.el: Fix improper parsing of first argument

* lisp/eshell/esh-opt.el (eshell--process-args): Refactor usage of
  args to eshell--args, as we rely on modifications from
  eshell--process-option and vice versa.  These modifications were not
  being propogated in the (if (= ai 0)) case, since popping the first
  element of a list doesn't destructively modify the underlying list
  object.

Examples of broken behavior:

sudo -u root whoami
Outputs: -u
ls -I '*.txt' /dev/null
Errors with: *.txt: No such file or directory
---
 lisp/eshell/esh-opt.el | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/lisp/eshell/esh-opt.el b/lisp/eshell/esh-opt.el
index b802696306..67b7d05985 100644
--- a/lisp/eshell/esh-opt.el
+++ b/lisp/eshell/esh-opt.el
@@ -246,26 +246,27 @@ eshell--process-args
                                     options)))
          (ai 0) arg
          (eshell--args args))
-    (while (< ai (length args))
-      (setq arg (nth ai args))
+    (while (< ai (length eshell--args))
+      (setq arg (nth ai eshell--args))
       (if (not (and (stringp arg)
                    (string-match "^-\\(-\\)?\\(.*\\)" arg)))
          (setq ai (1+ ai))
        (let* ((dash (match-string 1 arg))
               (switch (match-string 2 arg)))
          (if (= ai 0)
-             (setq args (cdr args))
-           (setcdr (nthcdr (1- ai) args) (nthcdr (1+ ai) args)))
+             (setq eshell--args (cdr eshell--args))
+           (setcdr (nthcdr (1- ai) eshell--args)
+                    (nthcdr (1+ ai) eshell--args)))
          (if dash
              (if (> (length switch) 0)
                  (eshell--process-option name switch 1 ai options opt-vals)
-               (setq ai (length args)))
+               (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))))))))
-    (nconc (mapcar #'cdr opt-vals) args)))
+    (nconc (mapcar #'cdr opt-vals) eshell--args)))
 
 ;;; esh-opt.el ends here
-- 
2.14.2

>From e764cb997f132874e4c58a0c6ef9ae7adb580731 Mon Sep 17 00:00:00 2001
From: Jay Kamat <jaygkamat@gmail.com>
Date: Tue, 8 May 2018 12:36:36 -0700
Subject: [PATCH 2/2] esh-opt.el: Add a :parse-leading-options-only argument

* lisp/eshell/esh-opt.el (eshell-eval-using-options): Add a new
  :parse-leading-options-only argument which ignores dash/switch
  arguments after the first positional argument.
(eshell--process-args): Abort processing of arguments if we see one
positional argument and :parse-leading-options-only is set.
* lisp/eshell/em-tramp.el (eshell/sudo): Use
  :parse-leading-options-only, to avoid parsing subcommand switches as
  switches of sudo itself.
* test/lisp/eshell/esh-opt-tests.el: Add tests for new and old behavior.
---
 lisp/eshell/em-tramp.el           |   1 +
 lisp/eshell/esh-opt.el            |  17 +++++-
 test/lisp/eshell/esh-opt-tests.el | 124 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 139 insertions(+), 3 deletions(-)
 create mode 100644 test/lisp/eshell/esh-opt-tests.el

diff --git a/lisp/eshell/em-tramp.el b/lisp/eshell/em-tramp.el
index 004c495490..9475f4ed94 100644
--- a/lisp/eshell/em-tramp.el
+++ b/lisp/eshell/em-tramp.el
@@ -107,6 +107,7 @@ eshell/sudo
      '((?h "help" nil nil "show this usage screen")
        (?u "user" t user "execute a command as another USER")
        :show-usage
+       :parse-leading-options-only
        :usage "[(-u | --user) USER] COMMAND
 Execute a COMMAND as the superuser or another USER.")
      (throw 'eshell-external
diff --git a/lisp/eshell/esh-opt.el b/lisp/eshell/esh-opt.el
index 67b7d05985..80eb15359a 100644
--- a/lisp/eshell/esh-opt.el
+++ b/lisp/eshell/esh-opt.el
@@ -80,6 +80,10 @@ eshell-eval-using-options
   If present, do not pass MACRO-ARGS through `eshell-flatten-list'
 and `eshell-stringify-list'.
 
+: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.
+
 For example, OPTIONS might look like:
 
    ((?C  nil         nil multi-column    \"multi-column display\")
@@ -245,12 +249,19 @@ eshell--process-args
                                              (list sym)))))
                                     options)))
          (ai 0) arg
-         (eshell--args args))
-    (while (< ai (length eshell--args))
+         (eshell--args args)
+         (pos-argument-found nil))
+    (while (and (< ai (length eshell--args))
+                ;; Abort if we saw the first pos argument and option is set
+                (not (and pos-argument-found
+                          (memq :parse-leading-options-only options))))
       (setq arg (nth ai eshell--args))
       (if (not (and (stringp arg)
                    (string-match "^-\\(-\\)?\\(.*\\)" arg)))
-         (setq ai (1+ ai))
+          ;; Positional argument found, skip
+         (setq ai (1+ ai)
+                pos-argument-found t)
+        ;; dash or switch argument found, parse
        (let* ((dash (match-string 1 arg))
               (switch (match-string 2 arg)))
          (if (= ai 0)
diff --git a/test/lisp/eshell/esh-opt-tests.el 
b/test/lisp/eshell/esh-opt-tests.el
new file mode 100644
index 0000000000..13b522b389
--- /dev/null
+++ b/test/lisp/eshell/esh-opt-tests.el
@@ -0,0 +1,124 @@
+;;; tests/esh-opt-tests.el --- esh-opt test suite
+
+;; Copyright (C) 2018 Free Software Foundation, Inc.
+
+;; This file is part of GNU Emacs.
+
+;; GNU Emacs is free software: you can redistribute it and/or modify
+;; it under the terms of the GNU General Public License as published by
+;; the Free Software Foundation, either version 3 of the License, or
+;; (at your option) any later version.
+
+;; GNU Emacs is distributed in the hope that it will be useful,
+;; but WITHOUT ANY WARRANTY; without even the implied warranty of
+;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+;; GNU General Public License for more details.
+
+;; You should have received a copy of the GNU General Public License
+;; along with GNU Emacs.  If not, see <https://www.gnu.org/licenses/>.
+
+;;; Code:
+
+(require 'ert)
+(require 'esh-opt)
+
+(ert-deftest esh-opt-process-args-test ()
+  "Unit tests which verify correct behavior of `eshell--process-args'."
+  (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 "")))))
+  (should
+   (equal '("root" "world")
+          (eshell--process-args
+           "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")
+             :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")
+             :parse-leading-options-only))))
+  (should
+   (equal '("world" "emerge")
+          (eshell--process-args
+           "sudo"
+           '("-u" "root" "emerge" "-uDN" "world")
+           '((?u "user" t user "execute a command as another USER"))))))
+
+(ert-deftest test-eshell-eval-using-options ()
+  "Tests for `eshell-eval-using-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")))
+  (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")))
+
+  (eshell-eval-using-options
+   "sudo" '("emerge" "-uDN" "world")
+   '((?u "user" t user "execute a command as another USER"))
+   (should (equal user "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)))
+
+  (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")))
+
+  (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)))
+  (eshell-eval-using-options
+   "ls" '("/dev/null")
+   '((?l nil long-listing listing-style
+        "use a long listing format"))
+   (should (eq listing-style nil)))
+
+  (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)))
+  (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)))
+  (eshell-eval-using-options
+   "ls" '("/dev/null")
+   '((?h "human-readable" 1024 human-readable
+        "print sizes in human readable format"))
+   (should (eq human-readable nil))))
+
+(provide 'esh-opt-tests)
+
+;;; esh-opt-tests.el ends here
-- 
2.14.2