[PATCH] Quote components before using it is pattern

[PATCH] Quote components before using it is pattern

[Jörg Sommer]

A component might contain a character active in patterns, like * or ().
Take for example the path /tmp/foobar). Passing this and /tmp/foo123 as a
completion to _multi_parts results in an error:

_multi_parts:147: bad pattern: (foo123|foobar))*

The characters in the temporary variable tmp1 must be quote, before the
pattern is build with them.
---
 Completion/Base/Utility/_multi_parts |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/Completion/Base/Utility/_multi_parts b/Completion/Base/Utility/_multi_parts
index 615ef79..6fb6cbd 100644
--- a/Completion/Base/Utility/_multi_parts
+++ b/Completion/Base/Utility/_multi_parts
@@ -144,7 +144,7 @@ while true; do
 	SUFFIX="$suf"
       fi
 
-      matches=( "${(@M)matches:#(${(j:|:)~tmp1})*}" )
+      matches=( "${(@M)matches:#(${(j:|:)~${(q)tmp1}})*}" )
 
       if ! zstyle -t ":completion:${curcontext}:" expand suffix ||
          [[ -n "$menu" || -z "$compstate[insert]" ]]; then
-- 
1.6.0.2

Re: [PATCH] Quote components before using it is pattern

[Jörg Sommer]

[-- Attachment #1: Type: text/plain, Size: 607 bytes --]

Hi,

> A component might contain a character active in patterns, like * or ().
> Take for example the path /tmp/foobar). Passing this and /tmp/foo123 as a
> completion to _multi_parts results in an error:
>
> _multi_parts:147: bad pattern: (foo123|foobar))*
>
> The characters in the temporary variable tmp1 must be quote, before the
> pattern is build with them.

Here is a simple test that shows the error in the old version:

% compdef '_multi_parts -- / mpcompletions' mptest
% mpcompletions=( a/foo\) a/f123 )
% mptest a/<TAB>
_multi_parts:147: bad pattern: (foo)|f123)*

Bye, Jörg.

[-- Attachment #2: Digital signature http://en.wikipedia.org/wiki/OpenPGP --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [PATCH] Quote components before using it is pattern

[Mikael Magnusson]

On 14 October 2008 17:01, Jörg Sommer <joerg@alea.gnuu.de> wrote:
> Hi,
>
>> A component might contain a character active in patterns, like * or ().
>> Take for example the path /tmp/foobar). Passing this and /tmp/foo123 as a
>> completion to _multi_parts results in an error:
>>
>> _multi_parts:147: bad pattern: (foo123|foobar))*
>>
>> The characters in the temporary variable tmp1 must be quote, before the
>> pattern is build with them.
>
> Here is a simple test that shows the error in the old version:
>
> % compdef '_multi_parts -- / mpcompletions' mptest
> % mpcompletions=( a/foo\) a/f123 )
> % mptest a/<TAB>
> _multi_parts:147: bad pattern: (foo)|f123)*

This patch sort of breaks completing when the first segment has spaces
for me. They don't appear in the initial listing, but typing the first
char of their name sometimes completes them.
% compdef '_multi_parts -- / mpcompletions' mptest
% mpcompletions=3D( 'foo bar' 'foo baz' bar baz )
% mptest <TAB>ba<TAB>
bar baz

In this particular test, the foo bar and foo baz don't seem to appear
at all in the list, but they _are_ accepted (ie a space is inserted)
when they are typed in full. Removing the added (q) fixes this but of
course breaks the original case again. I have no idea what the line
does or how it does it.

The realworld case is zip- and tar-files that contain files with spaces.

-- 
Mikael Magnusson

Re: [PATCH] Quote components before using it is pattern

[Bart Schaefer]

On Aug 21,  2:46pm, Mikael Magnusson wrote:
} Subject: Re: [PATCH] Quote components before using it is pattern
}
} On 14 October 2008 17:01, Jorg Sommer <joerg@alea.gnuu.de> wrote:

Wow, digging back into history a bit here ...

} >> A component might contain a character active in patterns, like * or ().
} >>
} >> The characters in the temporary variable tmp1 must be quote, before the
} >> pattern is build with them.
} 
} This patch sort of breaks completing when the first segment has spaces
} for me.

As has come up elsewhere, the problem is that ${(q)...} is a bit too
aggressive for the purpose to which it is being put.  We need to quote
pattern characters in tmp1, but not other characters like spaces.

There's a rather ugly hunk of code in _path_files that does something
like this (see the comment "Explanation of substitution: ...") but
unfortunately I don't have time this morning to try to adapt it for
_multi_parts.

Re: [PATCH] Quote components before using it is pattern

[Mikael Magnusson]

On 21 August 2010 18:54, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Aug 21,  2:46pm, Mikael Magnusson wrote:
> } Subject: Re: [PATCH] Quote components before using it is pattern
> }
> } On 14 October 2008 17:01, Jorg Sommer <joerg@alea.gnuu.de> wrote:
>
> Wow, digging back into history a bit here ...

Not that much digging involved, git log **/_multi_parts showed this
was the latest change, I tried undoing and it 'worked'. :)

> } >> A component might contain a character active in patterns, like * or ().
> } >>
> } >> The characters in the temporary variable tmp1 must be quote, before the
> } >> pattern is build with them.
> }
> } This patch sort of breaks completing when the first segment has spaces
> } for me.
>
> As has come up elsewhere, the problem is that ${(q)...} is a bit too
> aggressive for the purpose to which it is being put.  We need to quote
> pattern characters in tmp1, but not other characters like spaces.
>
> There's a rather ugly hunk of code in _path_files that does something
> like this (see the comment "Explanation of substitution: ...") but
> unfortunately I don't have time this morning to try to adapt it for
> _multi_parts.

I merrily thought "Oh, I'll take a look at it then", but of course I
almost immediately exploded. I might try poking at it a bit later,
when I've gathered all my pieces.

-- 
Mikael Magnusson

Re: [PATCH] Quote components before using it is pattern

[Bart Schaefer]

On Aug 21,  9:54am, Bart Schaefer wrote:
}
} As has come up elsewhere, the problem is that ${(q)...} is a bit too
} aggressive for the purpose to which it is being put.  We need to quote
} pattern characters in tmp1, but not other characters like spaces.

Or maybe we're just being too clever trying to construct an (...|...)
pattern and the right thing is just to not quote anything:

Index: Completion/Base/Utility/_multi_parts
===================================================================
diff -c -r1.6 _multi_parts
--- _multi_parts	4 Nov 2008 04:47:52 -0000	1.6
+++ _multi_parts	21 Aug 2010 17:09:09 -0000
@@ -127,7 +127,8 @@
 	return
       fi
     elif (( $#tmp1 )); then
-      local ret=1
+      local ret=1 tt
+      local -a mm
 
       # More than one match. First we get all strings that match the
       # rest from the line.
@@ -144,7 +145,11 @@
 	SUFFIX="$suf"
       fi
 
-      matches=( "${(@M)matches:#(${(j:|:)~${(q)tmp1}})*}" )
+      for tt in $tmp1
+      do
+        mm+=( "${(@M)matches:#$tt*}" )
+      done
+      matches=( $mm )
 
       if ! zstyle -t ":completion:${curcontext}:" expand suffix ||
          [[ -n "$menu" || -z "$compstate[insert]" ]]; then

Re: [PATCH] Quote components before using it is pattern

[Mikael Magnusson]

On 21 August 2010 19:12, Bart Schaefer <schaefer@brasslantern.com> wrote:
> On Aug 21,  9:54am, Bart Schaefer wrote:
> }
> } As has come up elsewhere, the problem is that ${(q)...} is a bit too
> } aggressive for the purpose to which it is being put.  We need to quote
> } pattern characters in tmp1, but not other characters like spaces.
>
> Or maybe we're just being too clever trying to construct an (...|...)
> pattern and the right thing is just to not quote anything:
>
> Index: Completion/Base/Utility/_multi_parts
> ===================================================================
> diff -c -r1.6 _multi_parts
> --- _multi_parts        4 Nov 2008 04:47:52 -0000       1.6
> +++ _multi_parts        21 Aug 2010 17:09:09 -0000
> @@ -127,7 +127,8 @@
>        return
>       fi
>     elif (( $#tmp1 )); then
> -      local ret=1
> +      local ret=1 tt
> +      local -a mm
>
>       # More than one match. First we get all strings that match the
>       # rest from the line.
> @@ -144,7 +145,11 @@
>        SUFFIX="$suf"
>       fi
>
> -      matches=( "${(@M)matches:#(${(j:|:)~${(q)tmp1}})*}" )
> +      for tt in $tmp1
> +      do
> +        mm+=( "${(@M)matches:#$tt*}" )
> +      done
> +      matches=( $mm )
>
>       if ! zstyle -t ":completion:${curcontext}:" expand suffix ||
>          [[ -n "$menu" || -z "$compstate[insert]" ]]; then

This seems to work for me too (assuming it did for you anyway :). I
tried both testcases here, and completing my zip and tar files, and it
all completed correctly.

-- 
Mikael Magnusson