Small fix in `shell--unquote&requote-argument' - please review

Small fix in `shell--unquote&requote-argument' - please review

[Filipp Gunbin]

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

Re: Small fix in `shell--unquote&requote-argument' - please review

[Stefan Monnier]

> 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

Re: Small fix in `shell--unquote&requote-argument' - please review

[Filipp Gunbin]

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

Re: Small fix in `shell--unquote&requote-argument' - please review

[Stefan Monnier]

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

Re: Small fix in `shell--unquote&requote-argument' - please review

[Filipp Gunbin]

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

Re: Small fix in `shell--unquote&requote-argument' - please review

[Filipp Gunbin]

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

Re: Small fix in `shell--unquote&requote-argument' - please review

[Stefan Monnier]

> 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

Re: Small fix in `shell--unquote&requote-argument' - please review

[Stefan Monnier]

> 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

Re: Small fix in `shell--unquote&requote-argument' - please review

[Filipp Gunbin]

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