Bug: cd auto-completion of .. fails with parentheses in directory name

Bug: cd auto-completion of .. fails with parentheses in directory name

[Georg Nebehay]

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

Dear maintainers,

there appears to be a bug in cd autocompletion. Please find the details
here: https://github.com/robbyrussell/oh-my-zsh/issues/5424

Regards,
Georg

Re: Bug: cd auto-completion of .. fails with parentheses in directory name

[Daniel Shahaf]

Georg Nebehay wrote on Wed, Sep 21, 2016 at 08:12:06 +0200:
> there appears to be a bug in cd autocompletion. Please find the details
> here: https://github.com/robbyrussell/oh-my-zsh/issues/5424

(For future reference, please restate the issue in the email, don't just
link to an external description.  Among other reasons: because github
may shut down some day.)

I can reproduce this with latest master:
.
    % cd $(mktemp -d)
    % mkdir -p 'A(B)/C'
    % cd $_
    % cd ../<TAB>
.
completes nothing.

This is due to the following bit in _path_files:

   463	    elif [[ "$sopt" = *[/f]* ]]; then
   464	      compfiles -p$cfopt tmp1 accex "$skipped" "$_matcher $matcher[2]" "$sdirs" fake "$pats[@]"
   465	    else
   466	      compfiles -p$cfopt tmp1 accex "$skipped" "$_matcher $matcher[2]" '' fake "$pats[@]"
   467	    fi
   468	    tmp1=( $~tmp1 ) 2> /dev/null

At entry to this code, tmp1=( $PWD ).  compfiles changes that to
tmp1=( '/tmp/tmp.elGZzgGNwi/A(B)/*(-/)' ), and line 468 the parentheses
are interpreted as grouping characters.

Now, bin_compfiles() calls cf_pats() calls cfp_test_exact(), which has
a docstring, which says the values originating from $tmp1 are filenames.
Not patterns.  This means it is up to bin_compfiles() or its callees to
quote the literal parentheses in the input.

So perhaps this? —

[[[
diff --git a/Src/Zle/computil.c b/Src/Zle/computil.c
index 16b681c..27b78cd 100644
--- a/Src/Zle/computil.c
+++ b/Src/Zle/computil.c
@@ -4830,8 +4830,11 @@ cf_remove_other(char **names, char *pre, int *amb)
  *     3. compfiles -P  parnam1 parnam2 skipped matcher sdirs parnam3 
  *
  *     1. Set parnam1 to an array of patterns....
+ *        ${(P)parnam1} is an in/out parameter.
  *     2. Like #1 but without calling cfp_opt_pats().  (This is only used by _approximate.)
  *     3. Like #1 but varargs is implicitly set to  char *varargs[2] = { "*(-/)", NULL };.
+ *
+ *     parnam2 has to do with the accept-exact style (see cfp_test_exact()).
  */
 
 static int
@@ -4866,7 +4869,7 @@ bin_compfiles(char *nam, char **args, UNUSED(Options ops), UNUSED(int func))
 		return 0;
 	    }
 	    for (l = newlinklist(); *tmp; tmp++)
-		addlinknode(l, *tmp);
+		addlinknode(l, quotestring(*tmp, QT_BACKSLASH_PATTERN));
 	    set_list_array(args[1], cf_pats((args[0][1] == 'P'), !!args[0][2],
 					    l, getaparam(args[2]), args[3],
 					    args[4], args[5],
]]]

Questions:

1) Is the lifetime correct?  I'm not sure whether elemnts of 'l' should
be malloc()ed or heap allocated.

2) Should quoting be added in bin_compfiles() or at a later point during
cf_pats()?  Although the docstring of cfp_test_exact() says the elements
of 'l' are filenames, they are then passed to ztat() which ignores
backslashes, so it's not clear to me what quoting is expected where.

(I haven't checked whether the rest of cf_pats() expects the elements of
'l' to be quoted or not.)

Thanks for the bug report.

Cheers,

Daniel

> Regards,
> Georg

Re: Bug: cd auto-completion of .. fails with parentheses in directory name

[Bart Schaefer]

On Sep 22,  2:42pm, Daniel Shahaf wrote:
}
} 1) Is the lifetime correct?  I'm not sure whether elemnts of 'l' should
} be malloc()ed or heap allocated.

I think you have this correct -- completion is designed to put nearly
all of its data in a private zsh heap that is discarded all at once
when control returns to the line editor.  Only specific bits that persist
across calls go into directly-malloc'd memory.

} 2) Should quoting be added in bin_compfiles() or at a later point during
} cf_pats()?  Although the docstring of cfp_test_exact() says the elements
} of 'l' are filenames, they are then passed to ztat() which ignores
} backslashes, so it's not clear to me what quoting is expected where.

I think you have this in the right place, too, but I would be glad to
have someone else confirm.  Or we can just put it in and see if any
other examples break.

Re: Bug: cd auto-completion of .. fails with parentheses in directory name

[Mikael Magnusson]

On Thu, Sep 22, 2016 at 7:30 PM, Bart Schaefer
<schaefer@brasslantern.com> wrote:
> On Sep 22,  2:42pm, Daniel Shahaf wrote:
> }
> } 1) Is the lifetime correct?  I'm not sure whether elemnts of 'l' should
> } be malloc()ed or heap allocated.
>
> I think you have this correct -- completion is designed to put nearly
> all of its data in a private zsh heap that is discarded all at once
> when control returns to the line editor.  Only specific bits that persist
> across calls go into directly-malloc'd memory.
>
> } 2) Should quoting be added in bin_compfiles() or at a later point during
> } cf_pats()?  Although the docstring of cfp_test_exact() says the elements
> } of 'l' are filenames, they are then passed to ztat() which ignores
> } backslashes, so it's not clear to me what quoting is expected where.
>
> I think you have this in the right place, too, but I would be glad to
> have someone else confirm.  Or we can just put it in and see if any
> other examples break.

This patch breaks completion of
% mkdir '[a]'; touch '[a]/foo'
% ls \[a\]/<tab> # gives no results

for me. Reverting it on current master fixes it.

-- 
Mikael Magnusson

Re: Bug: cd auto-completion of .. fails with parentheses in directory name

[Daniel Shahaf]

Mikael Magnusson wrote on Fri, Oct 28, 2016 at 03:56:08 +0200:
> On Thu, Sep 22, 2016 at 7:30 PM, Bart Schaefer
> <schaefer@brasslantern.com> wrote:
> > On Sep 22,  2:42pm, Daniel Shahaf wrote:
> > }
> > } 1) Is the lifetime correct?  I'm not sure whether elemnts of 'l' should
> > } be malloc()ed or heap allocated.
> >
> > I think you have this correct -- completion is designed to put nearly
> > all of its data in a private zsh heap that is discarded all at once
> > when control returns to the line editor.  Only specific bits that persist
> > across calls go into directly-malloc'd memory.
> >
> > } 2) Should quoting be added in bin_compfiles() or at a later point during
> > } cf_pats()?  Although the docstring of cfp_test_exact() says the elements
> > } of 'l' are filenames, they are then passed to ztat() which ignores
> > } backslashes, so it's not clear to me what quoting is expected where.
> >
> > I think you have this in the right place, too, but I would be glad to
> > have someone else confirm.  Or we can just put it in and see if any
> > other examples break.
> 
> This patch breaks completion of
> % mkdir '[a]'; touch '[a]/foo'
> % ls \[a\]/<tab> # gives no results
> 
> for me. Reverting it on current master fixes it.

The new quotestring() is called in the input «\[a\]», (5 bytes), so it
becomes double-escaped.

Added to my list.

Re: Bug: cd auto-completion of .. fails with parentheses in directory name

[Daniel Shahaf]

Daniel Shahaf wrote on Fri, Oct 28, 2016 at 15:10:37 +0000:
> Mikael Magnusson wrote on Fri, Oct 28, 2016 at 03:56:08 +0200:
> > On Thu, Sep 22, 2016 at 7:30 PM, Bart Schaefer
> > <schaefer@brasslantern.com> wrote:
> > > On Sep 22,  2:42pm, Daniel Shahaf wrote:
> > > }
> > > } 1) Is the lifetime correct?  I'm not sure whether elemnts of 'l' should
> > > } be malloc()ed or heap allocated.
> > >
> > > I think you have this correct -- completion is designed to put nearly
> > > all of its data in a private zsh heap that is discarded all at once
> > > when control returns to the line editor.  Only specific bits that persist
> > > across calls go into directly-malloc'd memory.
> > >
> > > } 2) Should quoting be added in bin_compfiles() or at a later point during
> > > } cf_pats()?  Although the docstring of cfp_test_exact() says the elements
> > > } of 'l' are filenames, they are then passed to ztat() which ignores
> > > } backslashes, so it's not clear to me what quoting is expected where.
> > >
> > > I think you have this in the right place, too, but I would be glad to
> > > have someone else confirm.  Or we can just put it in and see if any
> > > other examples break.
> > 
> > This patch breaks completion of
> > % mkdir '[a]'; touch '[a]/foo'
> > % ls \[a\]/<tab> # gives no results
> > 
> > for me. Reverting it on current master fixes it.
> 
> The new quotestring() is called in the input «\[a\]», (5 bytes), so it
> becomes double-escaped.

The following fixes it without breaking the original case; however, the
comment is a Sven original so I'm a bit wary of contravening it.

There are comments throughout the function asking which variables are
shell-quoted, pattern-quoted, or unquoted filenames.  In particular
there's such a comment in relation to $donepath, a variable which
depends on preserve-prefix and affects $tmp1 in the first iteration.
(In Mikael's example its value is "".)

We should get the data flow and data types of _path_files and compfiles
clarified and documented.  Until then, all these fixes will be just
shots in the dark.

As to this patch, however, I don't trust it enough to commit it.  We
could revert the previous patch (workers/39412) to prevent a regression
from 5.2...  but given that that patch prevents «cd 'foo(+pwd)'; cd
../<TAB>» from running the 'pwd' command, I'm not so keen on reverting
it, either.

Suggestions, anyone?

Cheers,

Daniel


diff --git a/Completion/Unix/Type/_path_files b/Completion/Unix/Type/_path_files
index 0d36b54..32942d7 100644
--- a/Completion/Unix/Type/_path_files
+++ b/Completion/Unix/Type/_path_files
@@ -594,7 +594,7 @@ for prepath in "$prepaths[@]"; do
     # There are more components, so skip over the next components and make a
     # slash be added.
 
-    tmp1=( ${tmp1//(#b)([][()|*?^#~<>\\=])/\\${match[1]}} )
+    #tmp1=( ${tmp1//(#b)([][()|*?^#~<>\\=])/\\${match[1]}} )
     tmp2="${(M)tpre##${~skips}}"
     if [[ -n "$tmp2" ]]; then
       skipped="/$tmp2"

Re: Bug: cd auto-completion of .. fails with parentheses in directory name

[Peter Stephenson]

On Sat, 29 Oct 2016 18:06:32 +0000
Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> The following fixes it without breaking the original case; however, the
> comment is a Sven original so I'm a bit wary of contravening it.

That's a "my goodness, is that the time?" if ever I saw it.

The first step to getting this under control is probably tests.  There
are some Y01completion tests involving files.  A few with more exotic
characters plus variables wouldn't hurt.  What else is needed?
Directories and paths, presumably?  We could add a completion multibyte
test in a separate file with some of the more interesting chracters from
D07multibyte.

pws

Re: Bug: cd auto-completion of .. fails with parentheses in directory name

[Bart Schaefer]

On Oct 29,  7:39pm, Peter Stephenson wrote:
} Subject: Re: Bug: cd auto-completion of .. fails with parentheses in direc
}
} On Sat, 29 Oct 2016 18:06:32 +0000
} Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
} > The following fixes it without breaking the original case; however, the
} > comment is a Sven original so I'm a bit wary of contravening it.
} 
} That's a "my goodness, is that the time?" if ever I saw it.

My intuition would be that the line Daniel commented out was there in
the first place because the quoting that was added in the compfiles
internals was not previously present.  The fact that the end result
is double-backslashing of some of the special characters would tend
to support this notion.

I'd lean toward committing Daniel's _path_files one-line change.

} The first step to getting this under control is probably tests.  There
} are some Y01completion tests involving files.  A few with more exotic
} characters plus variables wouldn't hurt.  What else is needed?

I agree this would be a good place to start ... exotic characters are
interesting, but probably less so than characters from the set of
pattern characters and other shell specials.

Re: Bug: cd auto-completion of .. fails with parentheses in directory name

[Daniel Shahaf]

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

Peter Stephenson wrote on Sat, Oct 29, 2016 at 19:39:43 +0100:
> The first step to getting this under control is probably tests.  There
> are some Y01completion tests involving files.  A few with more exotic
> characters plus variables wouldn't hurt.

For starters here's a test for the problem reported at the start of the
thread.  I've verified that it fails when 39412 is reverted.

[-- Attachment #2: 0001-Add-a-regression-test-for-39412.patch --]
[-- Type: text/x-diff, Size: 774 bytes --]

>From e5e7ceb76dd03a4eccad923625530f0d3e658ab1 Mon Sep 17 00:00:00 2001
From: Daniel Shahaf <d.s@daniel.shahaf.name>
Date: Mon, 31 Oct 2016 03:17:20 +0000
Subject: [PATCH] Add a regression test for 39412.

---
 Test/Y01completion.ztst | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Test/Y01completion.ztst b/Test/Y01completion.ztst
index 1568369..b3f8be4 100644
--- a/Test/Y01completion.ztst
+++ b/Test/Y01completion.ztst
@@ -77,6 +77,16 @@ F:regression test workers/32182
 >FI:{file2}
 F:regression test workers/31611
 
+  {
+    mkdir 'A(B)' 'A(B)/C'
+    comptesteval 'cd "A(B)/C"'
+    comptest $'cd ../\t'
+  } always {
+    rmdir 'A(B)/C' 'A(B)'
+  }
+0:directory name is not a glob qualifier
+>line: {cd ../C/}{}
+
 %clean
 
   zmodload -ui zsh/zpty

Re: Bug: cd auto-completion of .. fails with parentheses in directory name

[Daniel Shahaf]

Daniel Shahaf wrote on Sat, Oct 29, 2016 at 18:06:32 +0000:
> diff --git a/Completion/Unix/Type/_path_files b/Completion/Unix/Type/_path_files
> index 0d36b54..32942d7 100644
> --- a/Completion/Unix/Type/_path_files
> +++ b/Completion/Unix/Type/_path_files
> @@ -594,7 +594,7 @@ for prepath in "$prepaths[@]"; do
>      # There are more components, so skip over the next components and make a
>      # slash be added.
>  
> -    tmp1=( ${tmp1//(#b)([][()|*?^#~<>\\=])/\\${match[1]}} )
> +    #tmp1=( ${tmp1//(#b)([][()|*?^#~<>\\=])/\\${match[1]}} )
>      tmp2="${(M)tpre##${~skips}}"
>      if [[ -n "$tmp2" ]]; then
>        skipped="/$tmp2"

I've reviewed the code again.  I believe that at the preceding
'compfiles -i' call, tmp1 is an input parameter containing a list of
unquoted filenames, and is not modified by the call, so this patch is
correct.

Committed in eccb7471b577d55f0b410088fc1125016476b332, with a regression
test.

I'd add some reverse-engineered docs for bin_compfiles()'s -i case but
I can't type right now.

@all, please report breakage if any, with completiosn of oddly-named
directories.

Cheers,

Daniel

Re: Bug: cd auto-completion of .. fails with parentheses in directory name

[Mikael Magnusson]

On Sun, Nov 27, 2016 at 4:17 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> Daniel Shahaf wrote on Sat, Oct 29, 2016 at 18:06:32 +0000:
>> diff --git a/Completion/Unix/Type/_path_files b/Completion/Unix/Type/_path_files
>> index 0d36b54..32942d7 100644
>> --- a/Completion/Unix/Type/_path_files
>> +++ b/Completion/Unix/Type/_path_files
>> @@ -594,7 +594,7 @@ for prepath in "$prepaths[@]"; do
>>      # There are more components, so skip over the next components and make a
>>      # slash be added.
>>
>> -    tmp1=( ${tmp1//(#b)([][()|*?^#~<>\\=])/\\${match[1]}} )
>> +    #tmp1=( ${tmp1//(#b)([][()|*?^#~<>\\=])/\\${match[1]}} )
>>      tmp2="${(M)tpre##${~skips}}"
>>      if [[ -n "$tmp2" ]]; then
>>        skipped="/$tmp2"
>
> I've reviewed the code again.  I believe that at the preceding
> 'compfiles -i' call, tmp1 is an input parameter containing a list of
> unquoted filenames, and is not modified by the call, so this patch is
> correct.
>
> Committed in eccb7471b577d55f0b410088fc1125016476b332, with a regression
> test.
>
> I'd add some reverse-engineered docs for bin_compfiles()'s -i case but
> I can't type right now.
>
> @all, please report breakage if any, with completiosn of oddly-named
> directories.

It still doesn't work for me, as before.

-- 
Mikael Magnusson

Re: Bug: cd auto-completion of .. fails with parentheses in directory name

[Mikael Magnusson]

On Sun, Nov 27, 2016 at 7:30 PM, Mikael Magnusson <mikachu@gmail.com> wrote:
> On Sun, Nov 27, 2016 at 4:17 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>> Daniel Shahaf wrote on Sat, Oct 29, 2016 at 18:06:32 +0000:
>>> diff --git a/Completion/Unix/Type/_path_files b/Completion/Unix/Type/_path_files
>>> index 0d36b54..32942d7 100644
>>> --- a/Completion/Unix/Type/_path_files
>>> +++ b/Completion/Unix/Type/_path_files
>>> @@ -594,7 +594,7 @@ for prepath in "$prepaths[@]"; do
>>>      # There are more components, so skip over the next components and make a
>>>      # slash be added.
>>>
>>> -    tmp1=( ${tmp1//(#b)([][()|*?^#~<>\\=])/\\${match[1]}} )
>>> +    #tmp1=( ${tmp1//(#b)([][()|*?^#~<>\\=])/\\${match[1]}} )
>>>      tmp2="${(M)tpre##${~skips}}"
>>>      if [[ -n "$tmp2" ]]; then
>>>        skipped="/$tmp2"
>>
>> I've reviewed the code again.  I believe that at the preceding
>> 'compfiles -i' call, tmp1 is an input parameter containing a list of
>> unquoted filenames, and is not modified by the call, so this patch is
>> correct.
>>
>> Committed in eccb7471b577d55f0b410088fc1125016476b332, with a regression
>> test.
>>
>> I'd add some reverse-engineered docs for bin_compfiles()'s -i case but
>> I can't type right now.
>>
>> @all, please report breakage if any, with completiosn of oddly-named
>> directories.
>
> It still doesn't work for me, as before.

zstyle ':completion:*' accept-exact-dirs 'yes'
Seems to be the thing that makes it not work.

-- 
Mikael Magnusson

Re: Bug: cd auto-completion of .. fails with parentheses in directory name

[Mikael Magnusson]

On Sun, Nov 27, 2016 at 7:32 PM, Mikael Magnusson <mikachu@gmail.com> wrote:
> On Sun, Nov 27, 2016 at 7:30 PM, Mikael Magnusson <mikachu@gmail.com> wrote:
>> On Sun, Nov 27, 2016 at 4:17 PM, Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>>> Daniel Shahaf wrote on Sat, Oct 29, 2016 at 18:06:32 +0000:
>>>> diff --git a/Completion/Unix/Type/_path_files b/Completion/Unix/Type/_path_files
>>>> index 0d36b54..32942d7 100644
>>>> --- a/Completion/Unix/Type/_path_files
>>>> +++ b/Completion/Unix/Type/_path_files
>>>> @@ -594,7 +594,7 @@ for prepath in "$prepaths[@]"; do
>>>>      # There are more components, so skip over the next components and make a
>>>>      # slash be added.
>>>>
>>>> -    tmp1=( ${tmp1//(#b)([][()|*?^#~<>\\=])/\\${match[1]}} )
>>>> +    #tmp1=( ${tmp1//(#b)([][()|*?^#~<>\\=])/\\${match[1]}} )
>>>>      tmp2="${(M)tpre##${~skips}}"
>>>>      if [[ -n "$tmp2" ]]; then
>>>>        skipped="/$tmp2"
>>>
>>> I've reviewed the code again.  I believe that at the preceding
>>> 'compfiles -i' call, tmp1 is an input parameter containing a list of
>>> unquoted filenames, and is not modified by the call, so this patch is
>>> correct.
>>>
>>> Committed in eccb7471b577d55f0b410088fc1125016476b332, with a regression
>>> test.
>>>
>>> I'd add some reverse-engineered docs for bin_compfiles()'s -i case but
>>> I can't type right now.
>>>
>>> @all, please report breakage if any, with completiosn of oddly-named
>>> directories.
>>
>> It still doesn't work for me, as before.
>
> zstyle ':completion:*' accept-exact-dirs 'yes'
> Seems to be the thing that makes it not work.

This seems to fix it, while also retaining the functionality of
accept-exact-dirs, which is pretty amazing. We probably need PWS to
update the big comment above the code though.

diff --git i/Completion/Unix/Type/_path_files w/Completion/Unix/Type/_path_files
index 32942d7a72..b272f2ce8f 100644
--- i/Completion/Unix/Type/_path_files
+++ w/Completion/Unix/Type/_path_files
@@ -381,7 +381,7 @@ for prepath in "$prepaths[@]"; do
     # however, for tmp2 we unquote everything.
     tmp1=${match[1]}
     tpre=${match[2]}
-    tmp2=${(Q)tmp1}
+    tmp2=$tmp1 #${(Q)tmp1}
     tmp1=${tmp1//(#b)\\(?)/$match[1]}
     tpre=${tpre//(#b)\\([^\\\]\[\^\~\(\)\#\*\?])/$match[1]}
     # Theory: donepath needs the quoting of special characters


-- 
Mikael Magnusson

Re: Bug: cd auto-completion of .. fails with parentheses in directory name

[Bart Schaefer]

On Nov 27,  7:39pm, Mikael Magnusson wrote:
}
} This seems to fix it, while also retaining the functionality of
} accept-exact-dirs, which is pretty amazing. We probably need PWS to
} update the big comment above the code though.

I suspect we've had some changes to the internals that are now handling
some of these quotings, and we're going to find other cases where it
is now redundant to add or remove levels of quote in the script code.