* bug#28323: 25.2; Eshell/TRAMP's sudo should only parse -u/-h on first position
@ 2017-09-01 21:05 Pierre Neidhardt
2017-09-02 1:08 ` Noam Postavsky
2018-05-08 20:30 ` bug#28323: Patchset to fix 28323 Jay Kamat
0 siblings, 2 replies; 6+ messages in thread
From: Pierre Neidhardt @ 2017-09-01 21:05 UTC (permalink / raw)
To: 28323
Load TRAMP's sudo in eshell and run this:
$ sudo pacman -Syu --noconfirm
sudo: unknown user: --noconfirm
$ sudo pacman --noconfirm -Syu
... (OK)
Eshell/TRAMP's sudo parses the subcommands flags while it obviosly
should not.
In GNU Emacs 25.2.1 (x86_64-pc-linux-gnu, GTK+ Version 3.22.15)
of 2017-08-28 built on dhiov23k
Windowing system distributor 'The X.Org Foundation', version 11.0.11903000
System Description: Gentoo Base System release 2.3
Configured using:
'configure --prefix=/usr --build=x86_64-pc-linux-gnu
--host=x86_64-pc-linux-gnu --mandir=/usr/share/man
--infodir=/usr/share/info --datadir=/usr/share --sysconfdir=/etc
--localstatedir=/var/lib --disable-dependency-tracking
--disable-silent-rules --docdir=/usr/share/doc/emacs-25.2
--htmldir=/usr/share/doc/emacs-25.2/html --libdir=/usr/lib64
--program-suffix=-emacs-25 --infodir=/usr/share/info/emacs-25
--localstatedir=/var
--enable-locallisppath=/etc/emacs:/usr/share/emacs/site-lisp
--with-gameuser=:gamestat --without-compress-install
--with-file-notification=inotify --enable-acl --without-dbus
--without-modules --without-gpm --without-hesiod --without-kerberos
--without-kerberos5 --with-xml2 --without-selinux --with-gnutls
--without-wide-int --with-zlib --with-sound=alsa --with-x --without-ns
--without-gconf --without-gsettings --without-toolkit-scroll-bars
--with-gif --with-jpeg --with-png --with-rsvg --with-tiff --with-xpm
--with-imagemagick --with-xft --without-cairo --without-libotf
--without-m17n-flt --with-x-toolkit=gtk3 --without-xwidgets
GENTOO_PACKAGE=app-editors/emacs-25.2 'CFLAGS=-march=ivybridge -O2
-pipe' CPPFLAGS= 'LDFLAGS=-Wl,-O1 -Wl,--as-needed''
Configured features:
XPM JPEG TIFF GIF PNG RSVG IMAGEMAGICK SOUND NOTIFY ACL GNUTLS LIBXML2
FREETYPE XFT ZLIB GTK3 X11
Important settings:
value of $LANG: en_US.utf8
locale-coding-system: utf-8-unix
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#28323: 25.2; Eshell/TRAMP's sudo should only parse -u/-h on first position
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 ` bug#28323: Patchset to fix 28323 Jay Kamat
1 sibling, 0 replies; 6+ messages in thread
From: Noam Postavsky @ 2017-09-02 1:08 UTC (permalink / raw)
To: Pierre Neidhardt; +Cc: 28323
On Fri, Sep 1, 2017 at 5:05 PM, Pierre Neidhardt <ambrevar@gmail.com> wrote:
>
> Load TRAMP's sudo in eshell and run this:
>
> $ sudo pacman -Syu --noconfirm
> sudo: unknown user: --noconfirm
> $ sudo pacman --noconfirm -Syu
> ... (OK)
>
> Eshell/TRAMP's sudo parses the subcommands flags while it obviosly
> should not.
Is this different from #27411?
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#28323: Patchset to fix 28323
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
2018-05-11 1:01 ` Noam Postavsky
1 sibling, 1 reply; 6+ messages in thread
From: Jay Kamat @ 2018-05-08 20:30 UTC (permalink / raw)
To: 28323
[-- 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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#28323: Patchset to fix 28323
2018-05-08 20:30 ` bug#28323: Patchset to fix 28323 Jay Kamat
@ 2018-05-11 1:01 ` Noam Postavsky
2018-05-11 20:27 ` Jay Kamat
0 siblings, 1 reply; 6+ messages in thread
From: Noam Postavsky @ 2018-05-11 1:01 UTC (permalink / raw)
To: Jay Kamat; +Cc: 28323
Jay Kamat <jaygkamat@gmail.com> writes:
> This was bugging me for other commands I was running (emerge -uDN
> world), so I decided to try to write a patch for this.
Thanks!
> 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.
Ah, looks like [1: 170266d096] missed a rename of args into
eshell--args.
[1: 170266d096]: 2013-09-12 01:20:07 -0400
Cleanup Eshell to rely less on dynamic scoping.
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=170266d096bc4d0952bee907532d14503e882bf6
> 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.
> * 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
^
double space
> being propogated in the (if (= ai 0)) case, since we cannot
> destructively modify the first element of a list.
This sentence here is a bit confusing, because of course we can modify
the first element with setcar, but you meant something different.
Something more like
Popping the first element of a list doesn't destructively modify the
underlying list object.
>
> Examples of broken behavior:
>
> sudo -u root whoami
> ls -I '*.txt' /dev/null
I think it would be helpful to mention how these are broken.
> * lisp/eshell/esh-opt.el: Add a new :raw-positional argument which
This entry should start with
* lisp/eshell/esh-opt.el (eshell-eval-using-options):
> ignores dash/switch arguments after the first positional
> argument. Useful for eshell/sudo, to avoid parsing subcommand
^
double space
> switches as switches of the root command.
Though I think it would make more sense to put the second sentence in
its own "* lisp/eshell/em-tramp.el (eshell/sudo)" entry.
> (eshell--process-option): add a new posarg argument which signals that
> we have found a positional argument already.
This entry should mention eshell--process-args as well, but actually I
think it would make sense to change the patch, such that only
eshell--process-args is fixed, see below.
> -(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))))
By the way, you don't need to quote keyword symbols (though it does
still work fine).
> (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
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.
> - (setcdr (nthcdr (1- ai) eshell--args)
> + (setcdr (nthcdr (1- ai) eshell--args)
This is just a whitespace change, right?
^ permalink raw reply [flat|nested] 6+ messages in thread
* bug#28323: Patchset to fix 28323
2018-05-11 1:01 ` Noam Postavsky
@ 2018-05-11 20:27 ` Jay Kamat
2018-05-16 0:14 ` Noam Postavsky
0 siblings, 1 reply; 6+ messages in thread
From: Jay Kamat @ 2018-05-11 20:27 UTC (permalink / raw)
To: Noam Postavsky; +Cc: 28323
[-- Attachment #1: Type: text/plain, Size: 1223 bytes --]
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
[-- 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: 2184 bytes --]
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
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-esh-opt.el-Add-a-parse-leading-options-only-argument.patch --]
[-- Type: text/x-diff, Size: 7489 bytes --]
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* bug#28323: Patchset to fix 28323
2018-05-11 20:27 ` Jay Kamat
@ 2018-05-16 0:14 ` Noam Postavsky
0 siblings, 0 replies; 6+ messages in thread
From: Noam Postavsky @ 2018-05-16 0:14 UTC (permalink / raw)
To: Jay Kamat; +Cc: 28323
tags 28323 fixed
close 28323 27.1
quit
Jay Kamat <jaygkamat@gmail.com> writes:
> Thanks for the review, my updated patches are below.
Pushed to master [1: 92a8230e49] [2: a4c616e27a].
I also simplified the code a bit by using `pop' [3: 2fda57c6fb].
[1: 92a8230e49]: 2018-05-15 19:32:49 -0400
esh-opt.el: Fix improper parsing of first argument (Bug#28323)
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=92a8230e49a65be48442ee95cf50c90514e48f99
[2: a4c616e27a]: 2018-05-15 19:32:49 -0400
esh-opt.el: Add a :parse-leading-options-only argument (Bug#28323)
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=a4c616e27aa48e7d524e0c5cfaf67f17d04989e4
[3: 2fda57c6fb]: 2018-05-15 19:32:49 -0400
Simplify eshell arg processing with (pop (nthcdr ...))
https://git.savannah.gnu.org/cgit/emacs.git/commit/?id=2fda57c6fb29262261911819ec8f5e4cccb3abbb
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-05-16 0:14 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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 ` bug#28323: Patchset to fix 28323 Jay Kamat
2018-05-11 1:01 ` Noam Postavsky
2018-05-11 20:27 ` Jay Kamat
2018-05-16 0:14 ` Noam Postavsky
Code repositories for project(s) associated with this public inbox
https://git.savannah.gnu.org/cgit/emacs.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).