Contributor

Member

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

Contributor

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

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.

Contributor Author

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

Member

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