unofficial mirror of emacs-devel@gnu.org 
 help / color / mirror / code / Atom feed
* Small fix in `shell--unquote&requote-argument' - please review
@ 2016-08-30 23:24 Filipp Gunbin
  2016-08-31 13:27 ` Stefan Monnier
  0 siblings, 1 reply; 9+ messages in thread
From: Filipp Gunbin @ 2016-08-30 23:24 UTC (permalink / raw)
  To: emacs-devel

Hi, here's what this patch does:

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.

Filipp


diff --git a/lisp/shell.el b/lisp/shell.el
index 1f019f2..1dc4d26 100644
--- a/lisp/shell.el
+++ b/lisp/shell.el
@@ -384,11 +384,13 @@ shell--unquote&requote-argument
        ((eq (aref qstr match) ?\") (setq dquotes (not dquotes)))
        ((eq (aref qstr match) ?\')
         (cond
+         ;; just text if inside double quotes
          (dquotes (funcall push "'" (match-end 0)))
-         ((< match (1+ (length qstr)))
-          (let ((end (string-match "'" qstr (1+ match))))
-            (funcall push (substring qstr (1+ match) end)
-                     (or end (length qstr)))))
+         ((< (1+ match) (length qstr))
+          (let ((end (or (string-match "'" qstr (1+ match))
+                         (error "No matching single quote"))))
+            (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)))



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: Small fix in `shell--unquote&requote-argument' - please review
  2016-08-30 23:24 Small fix in `shell--unquote&requote-argument' - please review Filipp Gunbin
@ 2016-08-31 13:27 ` Stefan Monnier
  2016-09-01 13:15   ` Filipp Gunbin
  0 siblings, 1 reply; 9+ messages in thread
From: Stefan Monnier @ 2016-08-31 13:27 UTC (permalink / raw)
  To: emacs-devel

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


        Stefan




^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Small fix in `shell--unquote&requote-argument' - please review
  2016-08-31 13:27 ` Stefan Monnier
@ 2016-09-01 13:15   ` Filipp Gunbin
  2016-09-01 13:39     ` Stefan Monnier
  0 siblings, 1 reply; 9+ messages in thread
From: Filipp Gunbin @ 2016-09-01 13:15 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

Hi Stefan,

On 31/08/2016 09:27 -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.

Filipp



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Small fix in `shell--unquote&requote-argument' - please review
  2016-09-01 13:15   ` Filipp Gunbin
@ 2016-09-01 13:39     ` Stefan Monnier
  2016-09-01 14:35       ` Filipp Gunbin
  0 siblings, 1 reply; 9+ messages in thread
From: Stefan Monnier @ 2016-09-01 13:39 UTC (permalink / raw)
  To: Filipp Gunbin; +Cc: emacs-devel

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


        Stefan



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Small fix in `shell--unquote&requote-argument' - please review
  2016-09-01 13:39     ` Stefan Monnier
@ 2016-09-01 14:35       ` Filipp Gunbin
  2016-09-08 19:22         ` Filipp Gunbin
  0 siblings, 1 reply; 9+ messages in thread
From: Filipp Gunbin @ 2016-09-01 14:35 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Small fix in `shell--unquote&requote-argument' - please review
  2016-09-01 14:35       ` Filipp Gunbin
@ 2016-09-08 19:22         ` Filipp Gunbin
  2016-09-08 20:23           ` Stefan Monnier
  2016-10-26 18:32           ` Stefan Monnier
  0 siblings, 2 replies; 9+ messages in thread
From: Filipp Gunbin @ 2016-09-08 19:22 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

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)))



^ permalink raw reply related	[flat|nested] 9+ messages in thread

* Re: Small fix in `shell--unquote&requote-argument' - please review
  2016-09-08 19:22         ` Filipp Gunbin
@ 2016-09-08 20:23           ` Stefan Monnier
  2016-10-26 18:32           ` Stefan Monnier
  1 sibling, 0 replies; 9+ messages in thread
From: Stefan Monnier @ 2016-09-08 20:23 UTC (permalink / raw)
  To: Filipp Gunbin; +Cc: emacs-devel

> 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)))'

OK, great, that looks very helpful.
I'll take a look at the patch ASAP,


        Stefan



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Small fix in `shell--unquote&requote-argument' - please review
  2016-09-08 19:22         ` Filipp Gunbin
  2016-09-08 20:23           ` Stefan Monnier
@ 2016-10-26 18:32           ` Stefan Monnier
  2016-10-27 12:41             ` Filipp Gunbin
  1 sibling, 1 reply; 9+ messages in thread
From: Stefan Monnier @ 2016-10-26 18:32 UTC (permalink / raw)
  To: Filipp Gunbin; +Cc: emacs-devel

> But if invoked directly:
> 1. (shell--unquote&requote-argument "te'st" 2)
> This produces `("testst" 2 comint-quote-filename)'.

Actually, that's a great test case, thank you.
The rest makes perfect sense (the (< match (1+ (length qstr))) was
probably just a typo: I meant 1- instead of 1+).

Sorry 'bout the delay.  Installed,


        Stefan



^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: Small fix in `shell--unquote&requote-argument' - please review
  2016-10-26 18:32           ` Stefan Monnier
@ 2016-10-27 12:41             ` Filipp Gunbin
  0 siblings, 0 replies; 9+ messages in thread
From: Filipp Gunbin @ 2016-10-27 12:41 UTC (permalink / raw)
  To: Stefan Monnier; +Cc: emacs-devel

On 26/10/2016 14:32 -0400, Stefan Monnier wrote:

>> But if invoked directly:
>> 1. (shell--unquote&requote-argument "te'st" 2)
>> This produces `("testst" 2 comint-quote-filename)'.
>
> Actually, that's a great test case, thank you.
> The rest makes perfect sense (the (< match (1+ (length qstr))) was
> probably just a typo: I meant 1- instead of 1+).
>
> Sorry 'bout the delay.  Installed,

Thanks!

Filipp



^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2016-10-27 12:41 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2016-08-30 23:24 Small fix in `shell--unquote&requote-argument' - please review Filipp Gunbin
2016-08-31 13:27 ` Stefan Monnier
2016-09-01 13:15   ` Filipp Gunbin
2016-09-01 13:39     ` Stefan Monnier
2016-09-01 14:35       ` Filipp Gunbin
2016-09-08 19:22         ` Filipp Gunbin
2016-09-08 20:23           ` Stefan Monnier
2016-10-26 18:32           ` Stefan Monnier
2016-10-27 12:41             ` Filipp Gunbin

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