cd completion: handle not accessible directories #1135

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants
@Mic92
Contributor

Mic92 commented Nov 22, 2013

No description provided.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Nov 24, 2013

Member

Could you describe the incorrect behaviour that you are seeing that this patch fixes?

Member

zanchey commented Nov 24, 2013

Could you describe the incorrect behaviour that you are seeing that this patch fixes?

@xfix

This comment has been minimized.

Show comment
Hide comment
@xfix

xfix Nov 24, 2013

Member

I would assume that cd would show error messages for directories that aren't available, but nope. Both with this patch, and without, fish doesn't even attempt to complete directories the user doesn't have access to (I checked it with /root directory). Unless I would get some example of what the patch helps with, I'm not going to merge it.

Edit: Actually, it appears to be the case for �CDPATH. Hm.

Member

xfix commented Nov 24, 2013

I would assume that cd would show error messages for directories that aren't available, but nope. Both with this patch, and without, fish doesn't even attempt to complete directories the user doesn't have access to (I checked it with /root directory). Unless I would get some example of what the patch helps with, I'm not going to merge it.

Edit: Actually, it appears to be the case for �CDPATH. Hm.

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Nov 24, 2013

Member

OK, this is a problem when CDPATH is set.

$ stat /root
Access: (0700/drwx------)
$ set -g CDPATH /root .
$ cd <TAB>cd: Permission denied: “/root”
/usr/share/fish/functions/__fish_complete_cd.fish (line 35):                    builtin cd $i
                                                                                     ^
in function “__fish_complete_cd”,
        called on standard input,

in command substitution
        called on standard input,
$ cd /
$ set -g CDPATH etc .
$ cd ~
$ cd <TAB>cd: The directory “etc” does not exist
/usr/share/fish/functions/__fish_complete_cd.fish (line 35):                    builtin cd $i
                                                                                     ^
in function “__fish_complete_cd”,
        called on standard input,

in command substitution
        called on standard input,

I think it would be better do something like test -d $i; and test -x $i; or continue.

Member

zanchey commented Nov 24, 2013

OK, this is a problem when CDPATH is set.

$ stat /root
Access: (0700/drwx------)
$ set -g CDPATH /root .
$ cd <TAB>cd: Permission denied: “/root”
/usr/share/fish/functions/__fish_complete_cd.fish (line 35):                    builtin cd $i
                                                                                     ^
in function “__fish_complete_cd”,
        called on standard input,

in command substitution
        called on standard input,
$ cd /
$ set -g CDPATH etc .
$ cd ~
$ cd <TAB>cd: The directory “etc” does not exist
/usr/share/fish/functions/__fish_complete_cd.fish (line 35):                    builtin cd $i
                                                                                     ^
in function “__fish_complete_cd”,
        called on standard input,

in command substitution
        called on standard input,

I think it would be better do something like test -d $i; and test -x $i; or continue.

@Mic92

This comment has been minimized.

Show comment
Hide comment
@Mic92

Mic92 Nov 24, 2013

Contributor
  • You are in directory, which is deleted after you cd in:
$ cd (mktmp -d)
$ rm (realpath .)
  • The permission is changed
$ cd (mktmp -d)
$ chmod a-rx .
  • Your CDPATH contains entries, which does not exists at all:
    I develop in ruby, which comes with library manager called 'bundler'.
    This program install its dependencies in a subdirectory of the current project path (if configured).
    To change fast to a library in this directory, I use the following:
set -x CDPATH . ./vendor/bundle/gems/ruby/2.0.0/gems

Node.js developer might have similar use cases.

@zanchey
Of course you can test for the existence directory in the first place, but than there would be a race condition between the check and the directory change.
The built-in cd seems do the check anyway, so there is no value in double checking it.
There might be also other cases where changing the directory will not work beside insufficient permission (I am looking at you, NFS).

Contributor

Mic92 commented Nov 24, 2013

  • You are in directory, which is deleted after you cd in:
$ cd (mktmp -d)
$ rm (realpath .)
  • The permission is changed
$ cd (mktmp -d)
$ chmod a-rx .
  • Your CDPATH contains entries, which does not exists at all:
    I develop in ruby, which comes with library manager called 'bundler'.
    This program install its dependencies in a subdirectory of the current project path (if configured).
    To change fast to a library in this directory, I use the following:
set -x CDPATH . ./vendor/bundle/gems/ruby/2.0.0/gems

Node.js developer might have similar use cases.

@zanchey
Of course you can test for the existence directory in the first place, but than there would be a race condition between the check and the directory change.
The built-in cd seems do the check anyway, so there is no value in double checking it.
There might be also other cases where changing the directory will not work beside insufficient permission (I am looking at you, NFS).

@zanchey

This comment has been minimized.

Show comment
Hide comment
@zanchey

zanchey Nov 25, 2013

Member

That is a good line of reasoning. NFS does cause some wonderful problems!

Member

zanchey commented Nov 25, 2013

That is a good line of reasoning. NFS does cause some wonderful problems!

@ridiculousfish

This comment has been minimized.

Show comment
Hide comment
@ridiculousfish

ridiculousfish Nov 26, 2013

Member

Merged as cb86b8f . Thanks!

Member

ridiculousfish commented Nov 26, 2013

Merged as cb86b8f . Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment