From mboxrd@z Thu Jan 1 00:00:00 1970 Path: news.gmane.org!.POSTED!not-for-mail From: Filipp Gunbin Newsgroups: gmane.emacs.devel Subject: Re: Small fix in `shell--unquote&requote-argument' - please review Date: Thu, 08 Sep 2016 22:22:21 +0300 Message-ID: References: NNTP-Posting-Host: blaine.gmane.org Mime-Version: 1.0 Content-Type: text/plain X-Trace: blaine.gmane.org 1473362604 14653 195.159.176.226 (8 Sep 2016 19:23:24 GMT) X-Complaints-To: usenet@blaine.gmane.org NNTP-Posting-Date: Thu, 8 Sep 2016 19:23:24 +0000 (UTC) User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/25.1.50 (darwin) Cc: emacs-devel@gnu.org To: Stefan Monnier Original-X-From: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Thu Sep 08 21:23:20 2016 Return-path: Envelope-to: ged-emacs-devel@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 1bi4up-0002on-Bu for ged-emacs-devel@m.gmane.org; Thu, 08 Sep 2016 21:23:15 +0200 Original-Received: from localhost ([::1]:53181 helo=lists.gnu.org) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bi4um-0000B6-W5 for ged-emacs-devel@m.gmane.org; Thu, 08 Sep 2016 15:23:13 -0400 Original-Received: from eggs.gnu.org ([2001:4830:134:3::10]:55703) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bi4ua-0008Pl-CX for emacs-devel@gnu.org; Thu, 08 Sep 2016 15:23:02 -0400 Original-Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bi4uX-0006ra-3g for emacs-devel@gnu.org; Thu, 08 Sep 2016 15:23:00 -0400 Original-Received: from out4-smtp.messagingengine.com ([66.111.4.28]:33346) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bi4uU-0006oo-Nk for emacs-devel@gnu.org; Thu, 08 Sep 2016 15:22:57 -0400 Original-Received: from compute6.internal (compute6.nyi.internal [10.202.2.46]) by mailout.nyi.internal (Postfix) with ESMTP id CF60F207D7; Thu, 8 Sep 2016 15:22:44 -0400 (EDT) Original-Received: from frontend1 ([10.202.2.160]) by compute6.internal (MEProxy); Thu, 08 Sep 2016 15:22:44 -0400 DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=fastmail.fm; h=cc :content-type:date:from:in-reply-to:message-id:mime-version :references:subject:to:x-sasl-enc:x-sasl-enc; s=mesmtp; bh=nXdhH R6yzfPWfYbkGUiXC+m+618=; b=dWkRJXBPA/NAeg+LZN/JMr934hrDIJmVLHqnt 07RdEzLBK6079ETfu3osYdlltJ2LOZTgmGGB90XQ16LY9o2Dc9Towp1KGU+kjcbM 0vuGwgw+PAJutUPFPCWKfpt85ZpAcdWxasSf4+OjkWJx95ziats0Z92+vbtNRavq K15/Jk= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d= messagingengine.com; h=cc:content-type:date:from:in-reply-to :message-id:mime-version:references:subject:to:x-sasl-enc :x-sasl-enc; s=smtpout; bh=nXdhHR6yzfPWfYbkGUiXC+m+618=; b=mdnDS QTTKrvnAjG9zKSo/Fuh3TKDN167i7/znEdlarWQ3x/a6r6SKe8fa8AbpagDZlgXB ZvM+JUljIlbTTkdM4YGymTEMuAvUPhex+VVoSzP21lAn9bJTyI8vzmVpsrmGMW32 pFDUBcLKPGEGPUIdvuOPbuINtyFePrvQAM6kXM= X-Sasl-enc: pSpLX+BpcU5JU2eNx7bz+CwwQkq0ris73Zhoo3Dj2jP3 1473362564 Original-Received: from fgunbin.local (unknown [94.25.218.10]) by mail.messagingengine.com (Postfix) with ESMTPA id 29D0AF2988; Thu, 8 Sep 2016 15:22:44 -0400 (EDT) In-Reply-To: (Filipp Gunbin's message of "Thu, 01 Sep 2016 17:35:18 +0300") X-detected-operating-system: by eggs.gnu.org: GNU/Linux 2.2.x-3.x [generic] X-Received-From: 66.111.4.28 X-BeenThere: emacs-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: "Emacs development discussions." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: emacs-devel-bounces+ged-emacs-devel=m.gmane.org@gnu.org Original-Sender: "Emacs-devel" Xref: news.gmane.org gmane.emacs.devel:207298 Archived-At: Hi Stefan & list, On 01/09/2016 17:35 +0300, Filipp Gunbin wrote: > Stefan, > > On 01/09/2016 09:39 -0400, Stefan Monnier wrote: > >>>>> 1. match is always less than (length str), so I guess they meant >>>>> `((< (1+ match) (length qstr))'. >>>>> 2. If `string-match' searching for ending single quote failed, >>>>> `(match-string 0)' is still called - be careful not to do this. >>>> Do you have corresponding recipes to trigger the corresponding errors >>>> (so we could write tests)? This part of my code is in dire need of >>>> tests, otherwise it's much too easy to introduce regressions. >>> Honestly, no, I found this when studying the code. >> >> Hmm... would you be able to artificially construct or describe a failing >> case, then? Part of the issue is that I don't remember the code well >> enough, so while your description makes some sense, I'm having >> difficulty understanding really what the change does. > > Yep, I'll try. > > Filipp I didn't find a way how to produce an error in an interactive command, because `comint-word' won't eat single/double quote characters (see `comint--match-partial-filename') But if invoked directly: 1. (shell--unquote&requote-argument "te'st" 2) This produces `("testst" 2 comint-quote-filename)'. Having found first `'', `string-match' cannot find closing quote. Then (match-end 0) is used, it's value is "undefined", but happens to be 3, so we duplicate last 2 chars and that's wrong. 2. (shell--unquote&requote-argument "test'" 2) This produces `("test" 2 comint-quote-filename)'. First `'' happens to be at the end of line. The condition `(< match (1+ (length qstr)))' returns t (it always does so), so we are trying to find closing quote when there are no characters left. Again, `(match-end 0)' is called after a failed search. The result is correct, because we are already at the end of string, but the logic is wrong. I've modified my patch to be quite about missing closing quote, it's below. I looked into shell.el/comint.el a bit. There's a FIXME in `comint-word' which points at missing single/double-quote parsing in shell.el. Do we have a planned strategy for how that should work? Something like that: - Parse backwards up to some stop char, like it is done now. We cannot know the context (whether we are in quotes) in this case, so it's reasonable to stop at the first non-filename char and complete what we have. What is and what is not a filename character cannot be known, too, because we don't know whether we are in quotes. or - Determine the position of the current shell pipeline start. Then, from it, we can determine the quoting context and use correct filename chars set. Filipp diff --git a/lisp/shell.el b/lisp/shell.el index 1f019f2..20a5350 100644 --- a/lisp/shell.el +++ b/lisp/shell.el @@ -384,11 +384,15 @@ shell--unquote&requote-argument ((eq (aref qstr match) ?\") (setq dquotes (not dquotes))) ((eq (aref qstr match) ?\') (cond + ;; treat single quote as text if inside double quotes (dquotes (funcall push "'" (match-end 0))) - ((< match (1+ (length qstr))) + ((< (1+ match) (length qstr)) (let ((end (string-match "'" qstr (1+ match)))) - (funcall push (substring qstr (1+ match) end) - (or end (length qstr))))) + (unless end + (setq end (length qstr)) + (set-match-data (list match (length qstr)))) + (funcall push (substring qstr (1+ match) end) end))) + ;; ignore if at the end of string (t nil))) (t (error "Unexpected case in shell--unquote&requote-argument!"))) (setq qpos (match-end 0)))