bug#28323: Patchset to fix 28323

[Jay Kamat]

Hi,

This was bugging me for other commands I was running (emerge -uDN
world), so I decided to try to write a patch for this.

In the process, I found another blocking bug, which would have broken
the -u flag entirely. Currently, 'sudo -u root whoami' returns '-u',
because of a bug involved with processing leading positional arguments.

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!

this section of eshell seems very important, so I also added some tests,
contained in the second patch. Please let me know if anything seems
wrong, as I didn't understand the code as well as I would have liked.

Commit 1:

>From 2fd7a43a95ee26b4fab1a0d713dc6d8a994e67bc 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 we cannot
  destructively modify the first element of a list.

Examples of broken behavior:

sudo -u root whoami
ls -I '*.txt' /dev/null
---
 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


Commit 2:

>From 05c1a597ce302144ea7114a3ed3867fdd4dca950 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 :raw-positional argument

* lisp/eshell/esh-opt.el: Add a new :raw-positional argument which
  ignores dash/switch arguments after the first positional
  argument. Useful for eshell/sudo, to avoid parsing subcommand
  switches as switches of the root command.
(eshell--process-option): add a new posarg argument which signals that
we have found a positional argument already.
* 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            |  27 ++++++---
 test/lisp/eshell/esh-opt-tests.el | 124 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 145 insertions(+), 7 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..9057de6da6 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
+       :raw-positional
        :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..2ca4b03f52 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'.
 
+:raw-positional
+  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\")
@@ -203,7 +207,7 @@ eshell--set-option
                       (setq eshell--args (cdr eshell--args)))))
               (or (nth 2 opt) t)))))
 
-(defun eshell--process-option (name switch kind ai options opt-vals)
+(defun eshell--process-option (name switch kind ai posarg 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.
 
@@ -219,7 +223,10 @@ eshell--process-option
     (while opts
       (if (and (listp (car opts))
                (nth kind (car opts))
-               (equal switch (nth kind (car opts))))
+               (equal switch (nth kind (car opts)))
+               ;; If we want to ignore arguments after a pos one, don't find.
+               (not (and posarg
+                         (memq ':raw-positional options))))
          (progn
            (eshell--set-option name ai (car opts) options opt-vals)
            (setq found t opts nil))
@@ -245,27 +252,33 @@ eshell--process-args
                                              (list sym)))))
                                     options)))
          (ai 0) arg
-         (eshell--args args))
+         (eshell--args args)
+         (pos-argument-found nil))
     (while (< ai (length eshell--args))
       (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)
              (setq eshell--args (cdr eshell--args))
-           (setcdr (nthcdr (1- ai) 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)
+                 (eshell--process-option name switch 1 ai pos-argument-found
+                                          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)
+                                        0 ai pos-argument-found
+                                        options opt-vals)
                (setq index (1+ index))))))))
     (nconc (mapcar #'cdr opt-vals) eshell--args)))
 
diff --git a/test/lisp/eshell/esh-opt-tests.el 
b/test/lisp/eshell/esh-opt-tests.el
new file mode 100644
index 0000000000..b0d59b0edd
--- /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" "world")
+          (eshell--process-args
+           "sudo"
+           '("emerge" "-uDN" "world")
+           '((?u "user" t user "execute a command as another USER")
+             :raw-positional))))
+  (should
+   (equal '("root" "emerge" "world")
+          (eshell--process-args
+           "sudo"
+           '("-u" "root" "emerge" "-uDN" "world")
+           '((?u "user" t user "execute a command as another USER")
+             :raw-positional))))
+  (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")
+     :raw-positional)
+   (should (equal user "root")))
+  (eshell-eval-using-options
+   "sudo" '("--user" "root" "whoami")
+   '((?u "user" t user "execute a command as another USER")
+     :raw-positional)
+   (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")
+     :raw-positional)
+   (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


Thanks again for all your amazing work on Emacs!
-Jay