bug#28323: Patchset to fix 28323

[Jay Kamat <jaygkamat@gmail.com>]

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

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:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-esh-opt.el-Fix-improper-parsing-of-first-argument.patch --]
[-- Type: text/x-diff, Size: 2090 bytes --]

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


[-- Attachment #3: Type: text/plain, Size: 13 bytes --]



Commit 2:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0002-esh-opt.el-Add-a-raw-positional-argument.patch --]
[-- Type: text/x-diff, Size: 8847 bytes --]

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


[-- Attachment #5: Type: text/plain, Size: 57 bytes --]



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