* 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 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.