From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Jay Kamat Newsgroups: gmane.emacs.bugs Subject: bug#28323: Patchset to fix 28323 Date: Fri, 11 May 2018 13:27:45 -0700 Message-ID: <87d0y2545a.fsf@gmail.com> References: <8737865dvh.fsf@gmail.com> <871selgaa1.fsf@gmail.com> <87o9hnkntm.fsf@gmail.com> NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="=-=-=" X-Trace: blaine.gmane.org 1526070367 18366 195.159.176.226 (11 May 2018 20:26:07 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Fri, 11 May 2018 20:26:07 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) Cc: 28323@debbugs.gnu.org To: Noam Postavsky Original-X-From: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Fri May 11 22:26:03 2018 Return-path: Envelope-to: geb-bug-gnu-emacs@m.gmane.org Original-Received: from lists.gnu.org ([208.118.235.17]) by blaine.gmane.org with esmtp (Exim 4.84_2) (envelope-from ) id 1fHEc7-0004ga-1K for geb-bug-gnu-emacs@m.gmane.org; Fri, 11 May 2018 22:26:03 +0200 Original-Received: from localhost ([::1]:42558 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fHEeD-000429-Ve for geb-bug-gnu-emacs@m.gmane.org; Fri, 11 May 2018 16:28:13 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:48447) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fHEe5-0003zT-Tn for bug-gnu-emacs@gnu.org; Fri, 11 May 2018 16:28:07 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fHEe2-0003Ly-ND for bug-gnu-emacs@gnu.org; Fri, 11 May 2018 16:28:05 -0400 Original-Received: from debbugs.gnu.org ([208.118.235.43]:50806) by eggs.gnu.org with esmtps (TLS1.0:RSA_AES_128_CBC_SHA1:16) (Exim 4.71) (envelope-from ) id 1fHEe2-0003Lp-Eg for bug-gnu-emacs@gnu.org; Fri, 11 May 2018 16:28:02 -0400 Original-Received: from Debian-debbugs by debbugs.gnu.org with local (Exim 4.84_2) (envelope-from ) id 1fHEe2-0006Yo-04 for bug-gnu-emacs@gnu.org; Fri, 11 May 2018 16:28:02 -0400 X-Loop: help-debbugs@gnu.org Resent-From: Jay Kamat Original-Sender: "Debbugs-submit" Resent-CC: bug-gnu-emacs@gnu.org Resent-Date: Fri, 11 May 2018 20:28:01 +0000 Resent-Message-ID: Resent-Sender: help-debbugs@gnu.org X-GNU-PR-Message: followup 28323 X-GNU-PR-Package: emacs X-GNU-PR-Keywords: confirmed patch Original-Received: via spool by 28323-submit@debbugs.gnu.org id=B28323.152607047625206 (code B ref 28323); Fri, 11 May 2018 20:28:01 +0000 Original-Received: (at 28323) by debbugs.gnu.org; 11 May 2018 20:27:56 +0000 Original-Received: from localhost ([127.0.0.1]:58703 helo=debbugs.gnu.org) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1fHEdw-0006YU-2t for submit@debbugs.gnu.org; Fri, 11 May 2018 16:27:56 -0400 Original-Received: from mail-pf0-f175.google.com ([209.85.192.175]:39668) by debbugs.gnu.org with esmtp (Exim 4.84_2) (envelope-from ) id 1fHEdu-0006YH-8K for 28323@debbugs.gnu.org; Fri, 11 May 2018 16:27:55 -0400 Original-Received: by mail-pf0-f175.google.com with SMTP id a22-v6so3219108pfn.6 for <28323@debbugs.gnu.org>; Fri, 11 May 2018 13:27:54 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=from:to:cc:subject:references:date:in-reply-to:message-id :user-agent:mime-version; bh=tyPoJA5XhUnqm7aPcAFIufxyL/tlOukYlu0kgetqHxQ=; b=FIWQbCQZFnV/QCUvKqBCs2WpzQUJL5VvNBZJw07ddJlNENx3wAjcNztUDA7q1VtPRo VINr0t/10LrZqMFaBDp2AvkeDkm8a0FfJcjb0aQ06N9snkS7dKG+Vr0rQY6nSs3/bsUQ Lbm8ln3MbBKf7PBFgNRWGsIEBz/lh11QJHIiKqpuQNxm2g8gcygjsJ08E/ucoJNx7kVz wzsIufCTaQT7zJmPN8Xui7sZuOWRHShev9Re9KlbFVSYIXYZtGOEYxt7sMx/e736o5iG bya/hZyU9MbHSxWUHttaTTZhLGWCmZ7oRY4WzWJRd6c93+T5Xuh55dP8N+HVHlcPcX13 FL1w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version; bh=tyPoJA5XhUnqm7aPcAFIufxyL/tlOukYlu0kgetqHxQ=; b=kpn4dFSHv5OGhHov5qhpzTDJnvWMEPlz4f5xucdO/UDbglerXNK7S0qwcdl/vVhyTs pfe8+Eb65d5HI0n3SqzY+3XkCmRmpU15c+mK2XQcr+FxGnl1HlhQOaMkLwcA+nXX4V8d JnYbG2xoAFMJWPh0p0Fy1FIKIAz75w4u3PYkyAk9GaXrrkpBp1A4Ql9+yitkxxPqHakS hJz0RgnqdDqca1oNML9KwZceI5CpD5eWfRk1YX50/wvmHi+OfllbvJSEw1y1KI6K1GEu siy6eSEFQKFnbQIOTRAuPjM14lgEMPiETao4DILVa2nLqcbE8ZJMIJh9oHi1qqfPQFfR wxEw== X-Gm-Message-State: ALKqPwcb5O8YG+pCe5C0Vd8jSvZMSecYdinix/44VoElJFLB0qnW/rnI diKCm+oIwDyIrF514kWeyB7gYFre X-Google-Smtp-Source: AB8JxZpmwM7s97oVrEDQK+tNAvqfbSLynRjO6dKXygwbWTIHThiu44NB3fmyYzfGYBStnqE+QB0AQg== X-Received: by 2002:a63:7f1a:: with SMTP id a26-v6mr377983pgd.371.1526070468205; Fri, 11 May 2018 13:27:48 -0700 (PDT) Original-Received: from laythe (c-24-4-217-5.hsd1.ca.comcast.net. [24.4.217.5]) by smtp.gmail.com with ESMTPSA id b72-v6sm8599532pfm.69.2018.05.11.13.27.46 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 11 May 2018 13:27:46 -0700 (PDT) In-Reply-To: <87o9hnkntm.fsf@gmail.com> (Noam Postavsky's message of "Thu, 10 May 2018 21:01:25 -0400") X-BeenThere: debbugs-submit@debbugs.gnu.org X-Mailman-Version: 2.1.18 Precedence: list X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 208.118.235.43 X-BeenThere: bug-gnu-emacs@gnu.org List-Id: "Bug reports for GNU Emacs, the Swiss army knife of text editors" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: bug-gnu-emacs-bounces+geb-bug-gnu-emacs=m.gmane.org@gnu.org Original-Sender: "bug-gnu-emacs" Xref: news.gmane.org gmane.emacs.bugs:146130 Archived-At: --=-=-= Content-Type: text/plain Hi Noam, Noam Postavsky 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 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0001-esh-opt.el-Fix-improper-parsing-of-first-argument.patch >From d6fe0506dec8856242f2c8fb4e38d267cd8bcfee Mon Sep 17 00:00:00 2001 From: Jay Kamat 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 --=-=-= Content-Type: text/x-diff Content-Disposition: inline; filename=0002-esh-opt.el-Add-a-parse-leading-options-only-argument.patch >From e764cb997f132874e4c58a0c6ef9ae7adb580731 Mon Sep 17 00:00:00 2001 From: Jay Kamat 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 . + +;;; 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 --=-=-=--