zsh-workers
 help / color / mirror / code / Atom feed
* Path with spaces in _canonical_paths
@ 2022-11-18 17:41 thomas
  2022-11-21  3:57 ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: thomas @ 2022-11-18 17:41 UTC (permalink / raw)
  To: zsh-workers

Hi all,

I'm trying to use `_canonical_paths` (on zsh 5.9) to let the user autocomplete
a fixed list of files. I hope I'm not using it incorrectly. Otherwise: It
seems to not escape spaces in paths appropriately. Short example:

compdef '_canonical_paths -N files files /tmp/My\ File' cmd
cmd <Tab>

Shows the following completions:

/tmp/My File    (appears twice)
/tmp/My\ File

i.e. I get in total 3 entries for the same path, and notably, the first option
is without escaping the space, i.e. it will result in a command line:

cmd /tmp/My File

which is obviously not as intended.

I tried scanning the code in /usr/share/zsh/functions/Completion/Unix/
_canonical_paths for the problem, but it is quite confusing to me. I was able
to gather that by removing the `-Q` from the line

   _wanted "$tag" expl "$desc" compadd $__gopts -Q -U -a matches && ret=0

it adds escapes to all the entries, i.e. the new completions are:

  /tmp/My\ File   (twice)
  /tmp/My\\\ File

which is still not as desired.

Much appreciated if anyone can help.

Best, Thomas

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

* Re: Path with spaces in _canonical_paths
  2022-11-18 17:41 Path with spaces in _canonical_paths thomas
@ 2022-11-21  3:57 ` Bart Schaefer
  2022-11-21 10:47   ` thomas
  0 siblings, 1 reply; 17+ messages in thread
From: Bart Schaefer @ 2022-11-21  3:57 UTC (permalink / raw)
  To: thomas; +Cc: zsh-workers

On Fri, Nov 18, 2022 at 9:42 AM <thomas@coldfix.de> wrote:
>
> I'm trying to use `_canonical_paths` (on zsh 5.9) to let the user autocomplete
> a fixed list of files. I hope I'm not using it incorrectly.

Tangentially to -workers:  Is _canonical_paths documented in the wrong
manual section?  It's lumped in with functions that are expected to
appear in the "completer" zstyle, which clearly it is not.

> compdef '_canonical_paths -N files files /tmp/My\ File' cmd
> cmd <Tab>
>
> Shows the following completions:
>
> /tmp/My File    (appears twice)
> /tmp/My\ File
>
> i.e. I get in total 3 entries for the same path

I'm not able to reproduce this using nothing but compinit plus the
compdef above.  I get just one completion, "/tmp/My\ File", regardless
of whether that file exists or not.

Try this:

debug_canonical_paths () {
  _canonical_paths -N files files /tmp/My\ File
}
compdef '_complete_debug debug_canonical_paths' cmd

Pressing TAB then drops a file in /tmp with a debug trace of the
execution of _canonical_paths.  In particular look at which "zstyle"
lookups are being done and which of those might be causing the extra
entries.  You can also look at the context around elements being added
to the "matches" array.


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

* Re: Path with spaces in _canonical_paths
  2022-11-21  3:57 ` Bart Schaefer
@ 2022-11-21 10:47   ` thomas
  2022-11-21 16:30     ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: thomas @ 2022-11-21 10:47 UTC (permalink / raw)
  To: zsh-workers


> I'm not able to reproduce this using nothing but compinit plus the
> compdef above.  I get just one completion, "/tmp/My\ File", regardless
> of whether that file exists or not.
> 
> Try this:
> 
> debug_canonical_paths () {
>    _canonical_paths -N files files /tmp/My\ File
> }
> compdef '_complete_debug debug_canonical_paths' cmd

Thanks for your answer! I can't reproduce with the minimal example 
anymore either.

However, I still get similar behaviour by first doing:

     sudo ln -s /tmp /foo

And then, same as before (with or without debug, with or without extra 
function, with or without -N):

     debug_canonical_paths () {
        _canonical_paths -N files files /tmp/My\ File
     }
     compdef '_complete_debug debug_canonical_paths' cmd

     cmd <Tab><Tab>

Results in:

     /foo/My File
     /tmp/My\ File

This is with blank zsh configuration `zsh -f` plus:

     autoload -U compinit; compinit


 > Pressing TAB then drops a file in /tmp with a debug trace of the
 > execution of _canonical_paths.  In particular look at which "zstyle"
 > lookups are being done and which of those might be causing the extra
 > entries.  You can also look at the context around elements being added
 > to the "matches" array.

The trace shows:

        +_canonical_paths_add_paths:19> '(anon)'
        [...]
         +(anon):3> matches+=( '/tmp/My\ File' )

but later:

         +_canonical_paths_add_paths:28> matches+=( '/foo/My File' )

which explains why `compadd -Q` handles those differently. So the 
different unintended behaviour seems to appear only in the "alternative" 
paths.

I originally encountered this behaviour with a file in 
/var/run/media/thomas/ which is symlinked to /media on my system. I get 
the same problem with files in /usr/bin/ which has several symlinks 
/usr/sbin/, /bin, /sbin on archlinux.

I suspect when I previously also got a similar behaviour with my 
original minimal example without the symlink, there might have been 
tmpfs remounts or something(?), not sure what has changed, I did reboot 
in between.

Best, Thomas


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

* Re: Path with spaces in _canonical_paths
  2022-11-21 10:47   ` thomas
@ 2022-11-21 16:30     ` Bart Schaefer
  2022-11-21 17:41       ` Thomas Gläßle
  0 siblings, 1 reply; 17+ messages in thread
From: Bart Schaefer @ 2022-11-21 16:30 UTC (permalink / raw)
  To: thomas; +Cc: zsh-workers

On Mon, Nov 21, 2022 at 2:49 AM <thomas@coldfix.de> wrote:
>
> The trace shows:
>
>         +_canonical_paths_add_paths:19> '(anon)'
>         [...]
>          +(anon):3> matches+=( '/tmp/My\ File' )
>
> but later:
>
>          +_canonical_paths_add_paths:28> matches+=( '/foo/My File' )

I still can't reproduce this, but given your trace output, try

diff --git a/Completion/Unix/Type/_canonical_paths
b/Completion/Unix/Type/_canonical_paths
index a8fbbb524..b2eff84df 100644
--- a/Completion/Unix/Type/_canonical_paths
+++ b/Completion/Unix/Type/_canonical_paths
@@ -42,7 +42,7 @@ _canonical_paths_add_paths () {
     # ### Ideally, this codepath would do what the 'if' above does,
     # ### but telling compadd to pretend the "word on the command line"
     # ### is ${"the word on the command line"/$origpref/$canpref}.
-    matches+=(${${(M)files:#$canpref*}/$canpref/$origpref})
+    matches+=( "${(@)${(@M)files:#$canpref*}/$canpref/$origpref}" )
   fi

   for subdir in $expref?*(@); do


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

* Re: Path with spaces in _canonical_paths
  2022-11-21 16:30     ` Bart Schaefer
@ 2022-11-21 17:41       ` Thomas Gläßle
  2022-11-21 21:32         ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: Thomas Gläßle @ 2022-11-21 17:41 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers


[-- Attachment #1.1.1: Type: text/plain, Size: 2465 bytes --]


On 11/21/22 17:30, Bart Schaefer wrote:
> I still can't reproduce this

Weird.. With the symlink I can now on all my machines (arch, debian 11, 
mint 21), and even the official(?) docker:

    % docker run --rm -it zshusers/zsh:5.9
    # ln -s /tmp /foo
    # autoload -U compinit; compinit
    # compdef '_canonical_paths -N files files /tmp/My\ File' cmd
    # cmd <Tab>
    /foo/My File
    /tmp/My\ File
    foo/My File
    tmp/My File

(output split to several lines, to make it more readable)

> diff --git a/Completion/Unix/Type/_canonical_paths
> b/Completion/Unix/Type/_canonical_paths
> index a8fbbb524..b2eff84df 100644
> --- a/Completion/Unix/Type/_canonical_paths
> +++ b/Completion/Unix/Type/_canonical_paths
> @@ -42,7 +42,7 @@ _canonical_paths_add_paths () {
>       # ### Ideally, this codepath would do what the 'if' above does,
>       # ### but telling compadd to pretend the "word on the command line"
>       # ### is ${"the word on the command line"/$origpref/$canpref}.
> -    matches+=(${${(M)files:#$canpref*}/$canpref/$origpref})
> +    matches+=( "${(@)${(@M)files:#$canpref*}/$canpref/$origpref}" )
>     fi
>
>     for subdir in $expref?*(@); do

This didn't fix the problem. But applying the same escaping "hack"(?) as 
in the if-case above, did:

--- a/Completion/Unix/Type/_canonical_paths
+++ b/Completion/Unix/Type/_canonical_paths
@@ -30,19 +30,17 @@
    canpref+=$rltrim
    [[ $expref == *[^/] && $canpref == */ ]] && origpref+=/

+  local -a tmp_buffer
+  compadd -A tmp_buffer "$__gopts[@]" -a files
    # Append to $matches the subset of $files that matches $canpref.
    if [[ $canpref == $origpref ]]; then
      # This codepath honours any -M matchspec parameters.
-    () {
-      local -a tmp_buffer
-      compadd -A tmp_buffer "$__gopts[@]" -a files
-      matches+=( "${(@)tmp_buffer/$canpref/$origpref}" )
-    }
+    matches+=( "${(@)tmp_buffer/$canpref/$origpref}" )
    else
      # ### Ideally, this codepath would do what the 'if' above does,
      # ### but telling compadd to pretend the "word on the command line"
      # ### is ${"the word on the command line"/$origpref/$canpref}.
-    matches+=(${${(M)files:#$canpref*}/$canpref/$origpref})
+    matches+=(${${(M)tmp_buffer:#$canpref*}/$canpref/$origpref})
    fi

    for subdir in $expref?*(@); do






[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 19183 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

* Re: Path with spaces in _canonical_paths
  2022-11-21 17:41       ` Thomas Gläßle
@ 2022-11-21 21:32         ` Bart Schaefer
  2022-11-23 14:13           ` Daniel Shahaf
  0 siblings, 1 reply; 17+ messages in thread
From: Bart Schaefer @ 2022-11-21 21:32 UTC (permalink / raw)
  To: Thomas Gläßle; +Cc: zsh-workers

On Mon, Nov 21, 2022 at 9:41 AM Thomas Gläßle <thomas@coldfix.de> wrote:
>
> +  local -a tmp_buffer
> +  compadd -A tmp_buffer "$__gopts[@]" -a files
>  [...]
> +    matches+=( "${(@)tmp_buffer/$canpref/$origpref}" )
>     else
>       # ### Ideally, this codepath would do what the 'if' above does,
>       # ### but telling compadd to pretend the "word on the command line"
>       # ### is ${"the word on the command line"/$origpref/$canpref}.
> -    matches+=(${${(M)files:#$canpref*}/$canpref/$origpref})
> +    matches+=(${${(M)tmp_buffer:#$canpref*}/$canpref/$origpref})
>     fi

I'm not confident that's the right fix for other examples, given the
"Ideally" comment there.  Daniel, you were the last editor of that
section of _canonical_paths but the change appears to have been
related to its use in _mount (where it is no longer used now).  Any
comment?


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

* Re: Path with spaces in _canonical_paths
  2022-11-21 21:32         ` Bart Schaefer
@ 2022-11-23 14:13           ` Daniel Shahaf
  2022-11-23 21:36             ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: Daniel Shahaf @ 2022-11-23 14:13 UTC (permalink / raw)
  To: zsh-workers; +Cc: Thomas Gläßle

Bart Schaefer wrote on Mon, Nov 21, 2022 at 13:32:41 -0800:
> On Mon, Nov 21, 2022 at 9:41 AM Thomas Gläßle <thomas@coldfix.de> wrote:
> >
> > +  local -a tmp_buffer
> > +  compadd -A tmp_buffer "$__gopts[@]" -a files
> >  [...]
> > +    matches+=( "${(@)tmp_buffer/$canpref/$origpref}" )
> >     else
> >       # ### Ideally, this codepath would do what the 'if' above does,
> >       # ### but telling compadd to pretend the "word on the command line"
> >       # ### is ${"the word on the command line"/$origpref/$canpref}.
> > -    matches+=(${${(M)files:#$canpref*}/$canpref/$origpref})
> > +    matches+=(${${(M)tmp_buffer:#$canpref*}/$canpref/$origpref})
> >     fi
> 
> I'm not confident that's the right fix for other examples, given the
> "Ideally" comment there.  Daniel, you were the last editor of that
> section of _canonical_paths but the change appears to have been
> related to its use in _mount (where it is no longer used now).

The change in question is workers/39070 (= zsh-5.2-406-gaa041f7a5).

> Any comment?

Not really.

The log message and thread of that change describe what use-case that
change was fixing: namely, «umount /f/b<TAB>» → «umount /foo/bar».

The comment quoted above concerns how the candidate completions are
compared to the word on the command line.  The comment says that,
instead of applying s/foo/bar/ to the word on the command line and
comparing the result against the raw candidate completions, the reverse
is done — s/bar/foo/ is applied to the candidate completions and that's
compared to the raw word on the command line — and implies that that's
not exactly equivalent to the former, and that the former would be
preferred over the latter.

Adding or removing -Q or {(@)} (or ${(b)}, cf. workers/39080) might well
be independent of that issue, though.

HTH.

Daniel

P.S.  Feel free to Cc me directly when there's a question to me
specifically.


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

* Re: Path with spaces in _canonical_paths
  2022-11-23 14:13           ` Daniel Shahaf
@ 2022-11-23 21:36             ` Bart Schaefer
  2022-11-23 22:24               ` Daniel Shahaf
  0 siblings, 1 reply; 17+ messages in thread
From: Bart Schaefer @ 2022-11-23 21:36 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers, Thomas Gläßle

On Wed, Nov 23, 2022 at 6:14 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> Bart Schaefer wrote on Mon, Nov 21, 2022 at 13:32:41 -0800:
> > On Mon, Nov 21, 2022 at 9:41 AM Thomas Gläßle <thomas@coldfix.de> wrote:
> > >
> > > +  local -a tmp_buffer
> > > +  compadd -A tmp_buffer "$__gopts[@]" -a files
> > >  [...]
> > > +    matches+=( "${(@)tmp_buffer/$canpref/$origpref}" )
> > >     else
> > >       # ### Ideally, this codepath would do what the 'if' above does,
> > >       # ### but telling compadd to pretend the "word on the command line"
> > >       # ### is ${"the word on the command line"/$origpref/$canpref}.
> > > -    matches+=(${${(M)files:#$canpref*}/$canpref/$origpref})
> > > +    matches+=(${${(M)tmp_buffer:#$canpref*}/$canpref/$origpref})
> > >     fi
>
> The comment quoted above concerns how the candidate completions are
> compared to the word on the command line.  The comment says that,
> instead of applying s/foo/bar/ to the word on the command line and
> comparing the result against the raw candidate completions, the reverse
> is done — s/bar/foo/ is applied to the candidate completions and that's
> compared to the raw word on the command line

Hrm, but in both cases (even before the diff) the substitution is
s/$canpref/$origpref/ ?  Your explanation here would seem to imply
that in at least the second case the substitution should look like
s/$origpref/$canpref/.  The difference is that in the first case
$tmp_buffer is the result of filtering with "compadd ... files" and in
the second case we're altering the elements of $files directly.

> Adding or removing -Q or {(@)} (or ${(b)}, cf. workers/39080) might well
> be independent of that issue, though.

I believe the difference here is also compadd -- it performs the
quoting changes that -Q then has to account for.  So when we modify
$files directly in the second branch, we have to also emulate the
quote behavior of compadd.  My previous suggested patch perhaps didn't
go far enough in that direction?


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

* Re: Path with spaces in _canonical_paths
  2022-11-23 21:36             ` Bart Schaefer
@ 2022-11-23 22:24               ` Daniel Shahaf
  2022-11-23 22:42                 ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: Daniel Shahaf @ 2022-11-23 22:24 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers, Thomas Gläßle

Bart Schaefer wrote on Wed, 23 Nov 2022 21:36 +00:00:
> On Wed, Nov 23, 2022 at 6:14 AM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>>
>> Bart Schaefer wrote on Mon, Nov 21, 2022 at 13:32:41 -0800:
>> > On Mon, Nov 21, 2022 at 9:41 AM Thomas Gläßle <thomas@coldfix.de> wrote:
>> > >
>> > > +  local -a tmp_buffer
>> > > +  compadd -A tmp_buffer "$__gopts[@]" -a files
>> > >  [...]
>> > > +    matches+=( "${(@)tmp_buffer/$canpref/$origpref}" )
>> > >     else
>> > >       # ### Ideally, this codepath would do what the 'if' above does,
>> > >       # ### but telling compadd to pretend the "word on the command line"
>> > >       # ### is ${"the word on the command line"/$origpref/$canpref}.
>> > > -    matches+=(${${(M)files:#$canpref*}/$canpref/$origpref})
>> > > +    matches+=(${${(M)tmp_buffer:#$canpref*}/$canpref/$origpref})
>> > >     fi
>>
>> The comment quoted above concerns how the candidate completions are
>> compared to the word on the command line.  The comment says that,
>> instead of applying s/foo/bar/ to the word on the command line and
>> comparing the result against the raw candidate completions, the reverse
>> is done — s/bar/foo/ is applied to the candidate completions and that's
>> compared to the raw word on the command line
>
> Hrm, but in both cases (even before the diff) the substitution is
> s/$canpref/$origpref/ ?  Your explanation here would seem to imply
> that in at least the second case the substitution should look like
> s/$origpref/$canpref/.

Yes.  That's why the comment says s/$origpref/$canpref/ rather than
s/$canpref/$origpref/ as the code in HEAD says.

Perhaps an analogy.  Imagine a function that takes an unmetafied string
and an array of metafied strings and has to return 0 or 1 according to
whether or not the string is in the array, using nothing but strcmp()
for comparisons.  The function should either metafy the string or
unmetafy each string in the array in order, then.  (This analogy falls
short of explaining why one approach is better than the other.)

> The difference is that in the first case
> $tmp_buffer is the result of filtering with "compadd ... files" and in
> the second case we're altering the elements of $files directly.
>
>> Adding or removing -Q or {(@)} (or ${(b)}, cf. workers/39080) might well
>> be independent of that issue, though.
>
> I believe the difference here is also compadd -- it performs the
> quoting changes that -Q then has to account for.  So when we modify
> $files directly in the second branch, we have to also emulate the
> quote behavior of compadd.  My previous suggested patch perhaps didn't
> go far enough in that direction?

I'm not sure off the top of my head.  Perhaps that patch needs an extra
${(q)} somewhere?  Or perhaps we should add -Q to the if() codepath?
What do callers expect?

In general, I think the metafy analogy is valid here: we should define
whether $matches contains raw values or quoted values (and perhaps also
which form of quoting is used — presumably ${(q)}) and then ensure that
both branches conform to that internal API, just like for any char*
variable we define whether it's metafied or not.

Cheers,

Daniel


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

* Re: Path with spaces in _canonical_paths
  2022-11-23 22:24               ` Daniel Shahaf
@ 2022-11-23 22:42                 ` Bart Schaefer
  2022-11-23 23:06                   ` Daniel Shahaf
  2022-11-23 23:36                   ` Thomas Gläßle
  0 siblings, 2 replies; 17+ messages in thread
From: Bart Schaefer @ 2022-11-23 22:42 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers, Thomas Gläßle

On Wed, Nov 23, 2022 at 2:24 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> Yes.  That's why the comment says s/$origpref/$canpref/ rather than
> s/$canpref/$origpref/ as the code in HEAD says.

Ah, I see.  The comment is explaining what the else-branch OUGHT to be
accomplishing, rather than what it actually IS doing.  That was not
clear.

> I'm not sure off the top of my head.  Perhaps that patch needs an extra
> ${(q)} somewhere?  Or perhaps we should add -Q to the if() codepath?
> What do callers expect?

Callers expect it in the form from the if-branch (what compadd
produces), so I think the else-branch needs some variant of (q).

Thomas, can you try this?

diff --git a/Completion/Unix/Type/_canonical_paths
b/Completion/Unix/Type/_canonical_paths
index a8fbbb524..1444bc165 100644
--- a/Completion/Unix/Type/_canonical_paths
+++ b/Completion/Unix/Type/_canonical_paths
@@ -42,7 +42,8 @@ _canonical_paths_add_paths () {
     # ### Ideally, this codepath would do what the 'if' above does,
     # ### but telling compadd to pretend the "word on the command line"
     # ### is ${"the word on the command line"/$origpref/$canpref}.
-    matches+=(${${(M)files:#$canpref*}/$canpref/$origpref})
+    # ### The following approximates that.
+    matches+=(${(q)${(M)files:#$canpref*}/$canpref/$origpref})
   fi

   for subdir in $expref?*(@); do


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

* Re: Path with spaces in _canonical_paths
  2022-11-23 22:42                 ` Bart Schaefer
@ 2022-11-23 23:06                   ` Daniel Shahaf
  2022-11-23 23:12                     ` Bart Schaefer
  2022-11-23 23:36                   ` Thomas Gläßle
  1 sibling, 1 reply; 17+ messages in thread
From: Daniel Shahaf @ 2022-11-23 23:06 UTC (permalink / raw)
  To: Bart Schaefer; +Cc: zsh-workers, Thomas Gläßle

Bart Schaefer wrote on Wed, 23 Nov 2022 22:42 +00:00:
> On Wed, Nov 23, 2022 at 2:24 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>>
>> Yes.  That's why the comment says s/$origpref/$canpref/ rather than
>> s/$canpref/$origpref/ as the code in HEAD says.
>
> Ah, I see.  The comment is explaining what the else-branch OUGHT to be
> accomplishing, rather than what it actually IS doing.

Right.

> That was not clear.
>

Thanks for the feedback.

That comment is using "###" comments (a conventional "TODO" marker), the
word "ideally", a conditional ("would"), and the order of pattern and
replacement is the opposite of what it is in the code.  I had thought it
was clear.

Anyway, I see you've extended it to clarify.  Thanks for this.

>> I'm not sure off the top of my head.  Perhaps that patch needs an extra
>> ${(q)} somewhere?  Or perhaps we should add -Q to the if() codepath?
>> What do callers expect?
>
> Callers expect it in the form from the if-branch (what compadd
> produces), so I think the else-branch needs some variant of (q).
>
> Thomas, can you try this?
>
> diff --git a/Completion/Unix/Type/_canonical_paths
> b/Completion/Unix/Type/_canonical_paths
> index a8fbbb524..1444bc165 100644
> --- a/Completion/Unix/Type/_canonical_paths
> +++ b/Completion/Unix/Type/_canonical_paths
> @@ -42,7 +42,8 @@ _canonical_paths_add_paths () {
>      # ### Ideally, this codepath would do what the 'if' above does,
>      # ### but telling compadd to pretend the "word on the command line"
>      # ### is ${"the word on the command line"/$origpref/$canpref}.
> -    matches+=(${${(M)files:#$canpref*}/$canpref/$origpref})
> +    # ### The following approximates that.
> +    matches+=(${(q)${(M)files:#$canpref*}/$canpref/$origpref})
>    fi
>
>    for subdir in $expref?*(@); do


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

* Re: Path with spaces in _canonical_paths
  2022-11-23 23:06                   ` Daniel Shahaf
@ 2022-11-23 23:12                     ` Bart Schaefer
  2022-11-24  0:12                       ` Daniel Shahaf
  0 siblings, 1 reply; 17+ messages in thread
From: Bart Schaefer @ 2022-11-23 23:12 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers, Thomas Gläßle

On Wed, Nov 23, 2022 at 3:06 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
>
> That comment is using "###" comments (a conventional "TODO" marker)

Also something I was not familiar with.

> Anyway, I see you've extended it to clarify.  Thanks for this.

Remains to be seen if it works ... I still can't figure out how to
cause that code path ("else") to be followed when I try to reproduce
Thomas's error.


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

* Re: Path with spaces in _canonical_paths
  2022-11-23 22:42                 ` Bart Schaefer
  2022-11-23 23:06                   ` Daniel Shahaf
@ 2022-11-23 23:36                   ` Thomas Gläßle
  2022-11-23 23:40                     ` Thomas Gläßle
  2022-11-24 18:51                     ` Bart Schaefer
  1 sibling, 2 replies; 17+ messages in thread
From: Thomas Gläßle @ 2022-11-23 23:36 UTC (permalink / raw)
  To: Bart Schaefer, Daniel Shahaf; +Cc: zsh-workers


[-- Attachment #1.1.1: Type: text/plain, Size: 1104 bytes --]


On 11/23/22 23:42, Bart Schaefer wrote:
> +    # ### The following approximates that.
> +    matches+=(${(q)${(M)files:#$canpref*}/$canpref/$origpref})

Seems to resolve the issue!

However, there is another weird behaviour that I just now noticed (but 
it's unrelated to this patch). When resolving relative paths from within 
a symlinked directory, it seems to assume the resolved path of the cwd 
as basepath. Maybe best explained by another example:

     # ln -s /usr/local/bin /mnt
     # ln -s /tmp /foo
     # cd /mnt
     # compdef '_canonical_paths -N files files /tmp/My\ File' cmd
     # cmd <Tab>
     ../../../foo/My\ File
     ../../../tmp/My\ File

Notice the amount of ../

> Remains to be seen if it works ... I still can't figure out how to
> cause that code path ("else") to be followed when I try to reproduce
> Thomas's error.

Are you still not able to reproduce, even on the docker? How is that 
possible? Can it be different due to terminal? Doesn't really make sense 
to me.. Are you able to reproduce @Daniel?

Best, Thomas

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 19183 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

* Re: Path with spaces in _canonical_paths
  2022-11-23 23:36                   ` Thomas Gläßle
@ 2022-11-23 23:40                     ` Thomas Gläßle
  2022-11-24 18:51                     ` Bart Schaefer
  1 sibling, 0 replies; 17+ messages in thread
From: Thomas Gläßle @ 2022-11-23 23:40 UTC (permalink / raw)
  To: Bart Schaefer, Daniel Shahaf; +Cc: zsh-workers


[-- Attachment #1.1.1: Type: text/plain, Size: 190 bytes --]


On 11/24/22 00:36, Thomas Gläßle wrote:
>
>     # ln -s /usr/local/bin /mnt
>     # cd /mnt

Of course, /mnt was a bad choice because it already exists. Take /bar 
instead.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 19183 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

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

* Re: Path with spaces in _canonical_paths
  2022-11-23 23:12                     ` Bart Schaefer
@ 2022-11-24  0:12                       ` Daniel Shahaf
  2022-11-24 18:42                         ` Bart Schaefer
  0 siblings, 1 reply; 17+ messages in thread
From: Daniel Shahaf @ 2022-11-24  0:12 UTC (permalink / raw)
  To: zsh-workers; +Cc: Thomas Gläßle

Bart Schaefer wrote on Wed, Nov 23, 2022 at 15:12:40 -0800:
> On Wed, Nov 23, 2022 at 3:06 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> > Anyway, I see you've extended it to clarify.  Thanks for this.
> 
> Remains to be seen if it works ... I still can't figure out how to
> cause that code path ("else") to be followed when I try to reproduce
> Thomas's error.

In the zsh tree,
.
    _f() { _canonical_paths tag desc $PWD/README }
    compdef _f f
    f <TAB>
.
is enough for me to get the else path to be taken.  For the variant with
-N and quoting,
.
    touch /tmp/My\ File
    ln -s /tmp slashtmp 
    compdef '_canonical_paths -N files files /tmp/My\ File' cmd
    cmd <TAB>
.
gives
.
     1	>>> files
     2	/tmp/My\ File     slashtmp/My File

That's with current HEAD and my nearly-minimal test setup (sets
little besides 'format', 'group-name', $PS1, $PS4, and $fpath).

Cheers,

Daniel



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

* Re: Path with spaces in _canonical_paths
  2022-11-24  0:12                       ` Daniel Shahaf
@ 2022-11-24 18:42                         ` Bart Schaefer
  0 siblings, 0 replies; 17+ messages in thread
From: Bart Schaefer @ 2022-11-24 18:42 UTC (permalink / raw)
  To: Daniel Shahaf; +Cc: zsh-workers, Thomas Gläßle

On Wed, Nov 23, 2022 at 4:13 PM Daniel Shahaf <d.s@daniel.shahaf.name> wrote:
> .
>     _f() { _canonical_paths tag desc $PWD/README }
>     compdef _f f
>     f <TAB>
> .
> is enough for me to get the else path to be taken.

For that one I do get

% f <TAB>
/Users/schaefer/Public/zsh/README  README

> .
>     touch /tmp/My\ File
>     ln -s /tmp slashtmp
>     compdef '_canonical_paths -N files files /tmp/My\ File' cmd
>     cmd <TAB>
> .

But here I just get

% cmd <TAB>
% cmd /tmp/My\ File

I haven't actually tried the docker image.  Does the above have
something to do with being on a Mac, where /tmp itself is already a
symlink to /private/tmp?


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

* Re: Path with spaces in _canonical_paths
  2022-11-23 23:36                   ` Thomas Gläßle
  2022-11-23 23:40                     ` Thomas Gläßle
@ 2022-11-24 18:51                     ` Bart Schaefer
  1 sibling, 0 replies; 17+ messages in thread
From: Bart Schaefer @ 2022-11-24 18:51 UTC (permalink / raw)
  To: Thomas Gläßle; +Cc: Daniel Shahaf, zsh-workers

On Wed, Nov 23, 2022 at 3:36 PM Thomas Gläßle <thomas@coldfix.de> wrote:
>
> On 11/23/22 23:42, Bart Schaefer wrote:
> > +    # ### The following approximates that.
> > +    matches+=(${(q)${(M)files:#$canpref*}/$canpref/$origpref})
>
> Seems to resolve the issue!

Good!

> However, there is another weird behaviour that I just now noticed>
>
>      # cmd <Tab>
>      ../../../foo/My\ File
>      ../../../tmp/My\ File

> When resolving relative paths from within
> a symlinked directory, it seems to assume the resolved path of the cwd
> as basepath.

Yes, there's a loop that walks up the tree if there's a path prefix on
the file being completed.  How far up the tree it will look is
controlled by the canonical-paths-back-limit zstyle (default 8
levels).  This is being exacerbated by the -N option because without
-N the :P modifier on the input paths would already have resolved the
symlinks.

Independently, I wonder if we should switch from :P to :A in the
not-dash-N case.


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

end of thread, other threads:[~2022-11-24 18:52 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-18 17:41 Path with spaces in _canonical_paths thomas
2022-11-21  3:57 ` Bart Schaefer
2022-11-21 10:47   ` thomas
2022-11-21 16:30     ` Bart Schaefer
2022-11-21 17:41       ` Thomas Gläßle
2022-11-21 21:32         ` Bart Schaefer
2022-11-23 14:13           ` Daniel Shahaf
2022-11-23 21:36             ` Bart Schaefer
2022-11-23 22:24               ` Daniel Shahaf
2022-11-23 22:42                 ` Bart Schaefer
2022-11-23 23:06                   ` Daniel Shahaf
2022-11-23 23:12                     ` Bart Schaefer
2022-11-24  0:12                       ` Daniel Shahaf
2022-11-24 18:42                         ` Bart Schaefer
2022-11-23 23:36                   ` Thomas Gläßle
2022-11-23 23:40                     ` Thomas Gläßle
2022-11-24 18:51                     ` Bart Schaefer

Code repositories for project(s) associated with this public inbox

	https://git.vuxu.org/mirror/zsh/

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 NNTP newsgroup(s).