From: Jay Kamat <jaygkamat@gmail.com>
To: 28323@debbugs.gnu.org
Subject: bug#28323: Patchset to fix 28323
Date: Tue, 08 May 2018 13:30:46 -0700 [thread overview]
Message-ID: <871selgaa1.fsf@gmail.com> (raw)
In-Reply-To: <8737865dvh.fsf@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
next prev parent reply other threads:[~2018-05-08 20:30 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-09-01 21:05 bug#28323: 25.2; Eshell/TRAMP's sudo should only parse -u/-h on first position Pierre Neidhardt
2017-09-02 1:08 ` Noam Postavsky
2018-05-08 20:30 ` Jay Kamat [this message]
2018-05-11 1:01 ` bug#28323: Patchset to fix 28323 Noam Postavsky
2018-05-11 20:27 ` Jay Kamat
2018-05-16 0:14 ` Noam Postavsky
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=871selgaa1.fsf@gmail.com \
--to=jaygkamat@gmail.com \
--cc=28323@debbugs.gnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this external index
https://git.savannah.gnu.org/cgit/emacs.git
https://git.savannah.gnu.org/cgit/emacs/org-mode.git
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.