From eae9ee7f35b1d40973b55309b54fbc8a3a4db4bf Mon Sep 17 00:00:00 2001 From: Johannes Altmanninger Date: Sun, 5 Sep 2021 02:11:21 +0200 Subject: [PATCH] builtin cd: print error about broken symlinks When cd is passed a broken symlink, this changes the error message from "no such directory" to "broken symbolic link". This scenario probably won't happen very often since completion won't suggest broken symlinks but it can't hurt to give a good error. Fish used to do this until 7ac5932. This logic used to be in path_get_cdpath, however, that is only used for highlighting, so we don't need error messages there. Changing cd is enough. Reword from "rotten" to "broken" since that's what file(1) uses. Clean-up leftovers from old "rotten" code (nomen est omen). See #8264 --- CHANGELOG.rst | 1 + src/builtin_cd.cpp | 13 ++++++++++--- src/path.h | 8 ++------ src/wutil.cpp | 23 +++++++++++++++++++++++ src/wutil.h | 3 +++ tests/checks/cd.fish | 24 ++++++++++++++++++++++++ 6 files changed, 63 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index af8b4383c..5c4dcb39f 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -66,6 +66,7 @@ Scripting improvements - ``help`` now knows which section is in which document again (:issue:`8245`). - Some error messages occuring after fork, like "text file busy" have been replaced by bespoke error messages for fish. This also restores error messages with current glibc versions that removed sys_errlist (:issue:`8234`, :issue:`4183`). - The ``realpath`` builtin now also squashes leading slashes with the ``--no-symlinks`` option (:issue:`8281`). +- When trying to ``cd`` to a dangling (broken) symbolic link, fish will print an error noting that the target is a broken link (:issue:`8264`). Interactive improvements ------------------------ diff --git a/src/builtin_cd.cpp b/src/builtin_cd.cpp index 9d0fa6e58..9846d8dfd 100644 --- a/src/builtin_cd.cpp +++ b/src/builtin_cd.cpp @@ -68,6 +68,7 @@ maybe_t builtin_cd(parser_t &parser, io_streams_t &streams, const wchar_t * errno = 0; auto best_errno = errno; + wcstring broken_symlink, broken_symlink_target; for (const auto &dir : dirs) { wcstring norm_dir = normalize_path(dir); @@ -83,7 +84,12 @@ maybe_t builtin_cd(parser_t &parser, io_streams_t &streams, const wchar_t * // - if in another directory there was a *file* by the correct name // we prefer *that* error because it's more specific if (errno == ENOENT) { - if (!best_errno) best_errno = errno; + maybe_t tmp; + if (broken_symlink.empty() && (tmp = wreadlink(norm_dir))) { + broken_symlink = norm_dir; + broken_symlink_target = std::move(*tmp); + } else if (!best_errno) + best_errno = errno; continue; } else if (errno == ENOTDIR) { best_errno = errno; @@ -105,11 +111,12 @@ maybe_t builtin_cd(parser_t &parser, io_streams_t &streams, const wchar_t * if (best_errno == ENOTDIR) { streams.err.append_format(_(L"%ls: '%ls' is not a directory\n"), cmd, dir_in.c_str()); + } else if (!broken_symlink.empty()) { + streams.err.append_format(_(L"%ls: '%ls' is a broken symbolic link to '%ls'\n"), cmd, + broken_symlink.c_str(), broken_symlink_target.c_str()); } else if (best_errno == ENOENT) { streams.err.append_format(_(L"%ls: The directory '%ls' does not exist\n"), cmd, dir_in.c_str()); - } else if (best_errno == EROTTEN) { - streams.err.append_format(_(L"%ls: '%ls' is a rotten symlink\n"), cmd, dir_in.c_str()); } else if (best_errno == EACCES) { streams.err.append_format(_(L"%ls: Permission denied: '%ls'\n"), cmd, dir_in.c_str()); } else { diff --git a/src/path.h b/src/path.h index 852714b15..21bcc1dc2 100644 --- a/src/path.h +++ b/src/path.h @@ -9,9 +9,6 @@ #include "common.h" #include "env.h" -/// Return value for path_cdpath_get when locatied a rotten symlink. -#define EROTTEN 1 - /// Returns the user configuration directory for fish. If the directory or one of its parents /// doesn't exist, they are first created. /// @@ -60,9 +57,8 @@ wcstring_list_t path_get_paths(const wcstring &cmd, const environment_t &vars); /// directories for relative paths. /// /// If no valid path is found, false is returned and errno is set to ENOTDIR if at least one such -/// path was found, but it did not point to a directory, EROTTEN if a rotten symbolic link was -/// found, or ENOENT if no file of the specified name was found. If both a rotten symlink and a file -/// are found, it is undefined which error status will be returned. +/// path was found, but it did not point to a directory, or ENOENT if no file of the specified +/// name was found. /// /// \param dir The name of the directory. /// \param wd The working directory. The working directory must end with a slash. diff --git a/src/wutil.cpp b/src/wutil.cpp index f060a6dd9..06eda4cb2 100644 --- a/src/wutil.cpp +++ b/src/wutil.cpp @@ -203,6 +203,29 @@ int make_fd_blocking(int fd) { return err == -1 ? errno : 0; } +maybe_t wreadlink(const wcstring &file_name) { + struct stat buf; + if (lwstat(file_name, &buf) == -1) { + return none(); + } + ssize_t bufsize = buf.st_size + 1; + char target_buf[bufsize]; + const std::string tmp = wcs2string(file_name); + ssize_t nbytes = readlink(tmp.c_str(), target_buf, bufsize); + if (nbytes == -1) { + wperror(L"readlink"); + return none(); + } + // The link might have been modified after our call to lstat. If the link now points to a path + // that's longer than the original one, we can't read everything in our buffer. Simply give + // up. We don't need to report an error since our only caller will already fall back to ENOENT. + if (nbytes == bufsize) { + return none(); + } + + return str2wcstring(target_buf, nbytes); +} + /// Wide character realpath. The last path component does not need to be valid. If an error occurs, /// wrealpath() returns none() and errno is likely set. maybe_t wrealpath(const wcstring &pathname) { diff --git a/src/wutil.h b/src/wutil.h index 8aa3e467a..21f54d860 100644 --- a/src/wutil.h +++ b/src/wutil.h @@ -43,6 +43,9 @@ void wperror(const wchar_t *s); /// Wide character version of getcwd(). wcstring wgetcwd(); +/// Wide character version of readlink(). +maybe_t wreadlink(const wcstring &file_name); + /// Wide character version of realpath function. /// \returns the canonicalized path, or none if the path is invalid. maybe_t wrealpath(const wcstring &pathname); diff --git a/tests/checks/cd.fish b/tests/checks/cd.fish index 6ab7f2598..66cc8d046 100644 --- a/tests/checks/cd.fish +++ b/tests/checks/cd.fish @@ -233,3 +233,27 @@ cd "" # CHECKERR: called on line {{\d+}} of file {{.*}}/cd.fish echo $status # CHECK: 1 + +ln -s no/such/directory broken-symbolic-link +begin + set -lx CDPATH + cd broken-symbolic-link +end +# CHECKERR: cd: '{{.*}}/broken-symbolic-link' is a broken symbolic link to 'no/such/directory' +# CHECKERR: {{.*}}/cd.fish (line {{\d+}}): +# CHECKERR: builtin cd $argv +# CHECKERR: ^ +# CHECKERR: in function 'cd' with arguments 'broken-symbolic-link' +# CHECKERR: called on line {{\d+}} of file {{.*}}/cd.fish + +# Make sure that "broken symlink" is reported over "no such file or directory". +begin + set -lx CDPATH other + cd broken-symbolic-link +end +# CHECKERR: cd: '{{.*}}/broken-symbolic-link' is a broken symbolic link to 'no/such/directory' +# CHECKERR: {{.*}}/cd.fish (line {{\d+}}): +# CHECKERR: builtin cd $argv +# CHECKERR: ^ +# CHECKERR: in function 'cd' with arguments 'broken-symbolic-link' +# CHECKERR: called on line {{\d+}} of file {{.*}}/cd.fish