From f7f39b8c90cb4b61148fcc3599521009906cd1ee Mon Sep 17 00:00:00 2001 From: Kurtis Rader Date: Mon, 3 Oct 2016 17:51:27 -0700 Subject: [PATCH] make fish's realpath compatible with GNU realpath After implementing `builtin fish_realpath` it was noticed that it did not behave like GNU `realpath` without options. Which is super annoying since that was the whole point of implementing the command. Major failure on my part since I wrote the unit tests to match the behavior of the existing `wrealpath()` function that I simply exposed as a builtin command. Rather than actually verifying it behaved in a manner compatible with GNU realpath. Also, while the decision to call the builtin `fish_realpath` seemed to make sense at the time of the original commit further reflection has shown that to be a silly, idiosyncratic, thing to have done. So rename it to simply `realpath`. Fixes 3400 --- doc_src/fish_realpath.txt | 13 ------ doc_src/realpath.txt | 12 ++++++ share/functions/realpath.fish | 55 ++++++++++++------------- src/builtin.cpp | 8 ++-- src/wutil.cpp | 51 ++++++++++++++++++----- tests/fish_realpath.err | 3 -- tests/fish_realpath.in | 39 ------------------ tests/fish_realpath.out | 5 --- tests/realpath.err | 1 + tests/realpath.in | 76 +++++++++++++++++++++++++++++++++++ tests/realpath.out | 11 +++++ 11 files changed, 171 insertions(+), 103 deletions(-) delete mode 100644 doc_src/fish_realpath.txt create mode 100644 doc_src/realpath.txt delete mode 100644 tests/fish_realpath.err delete mode 100644 tests/fish_realpath.in delete mode 100644 tests/fish_realpath.out create mode 100644 tests/realpath.err create mode 100644 tests/realpath.in create mode 100644 tests/realpath.out diff --git a/doc_src/fish_realpath.txt b/doc_src/fish_realpath.txt deleted file mode 100644 index a2ba0d0f9..000000000 --- a/doc_src/fish_realpath.txt +++ /dev/null @@ -1,13 +0,0 @@ -\section fish_realpath fish_realpath - Convert a path to an absolute path without symlinks - -\subsection fish_realpath-synopsis Synopsis -\fish{synopsis} -fish_realpath path -\endfish - -\subsection fish_realpath-description Description - -This is an implementation of the external realpath command that doesn't support any options. It's meant to be used only by scripts which need to be portable. In general scripts shouldn't invoke this directly. They should just use `realpath` which will fallback to this builtin if an external command cannot be found. - -If the path is invalid no translated path will be written to stdout and an error will be reported. -This implementation behaves like the GNU command being invoked with `--canonicalize-existing`. diff --git a/doc_src/realpath.txt b/doc_src/realpath.txt new file mode 100644 index 000000000..35e6503a3 --- /dev/null +++ b/doc_src/realpath.txt @@ -0,0 +1,12 @@ +\section realpath realpath - Convert a path to an absolute path without symlinks + +\subsection realpath-synopsis Synopsis +\fish{synopsis} +realpath path +\endfish + +\subsection realpath-description Description + +This is implemented as a function and a builtin. The function will attempt to use an external realpath command if one can be found. Otherwise it falls back to the builtin. The builtin does not support any options. It's meant to be used only by scripts which need to be portable. The builtin implementation behaves like GNU realpath when invoked without any options (which is the most common use case). In general scripts should not invoke the builtin directly. They should just use `realpath`. + +If the path is invalid no translated path will be written to stdout and an error will be reported. diff --git a/share/functions/realpath.fish b/share/functions/realpath.fish index df4dcb61b..e18cfd84b 100644 --- a/share/functions/realpath.fish +++ b/share/functions/realpath.fish @@ -9,40 +9,41 @@ # fallback behavion through our builtin. if not command -s realpath >/dev/null - + # If there is a HomeBrew installed version of GNU realpath named grealpath use that. if command -s grealpath >/dev/null function realpath -w grealpath -d "print the resolved path [grealpath]" grealpath $argv end - else - function realpath -w fish_realpath -d "get an absolute path without symlinks [fish_realpath]" - if test -z "$argv" - printf "usage: %s%s%s %sfile%s …\n" (set_color -o) $_ (set_color normal) (set_color -u) (set_color normal) - echo " resolves files as absolute paths without symlinks" - return 1 - end - - # loop over arguments - allow our realpath to work on more than one path per invocatgon like gnu/bsd realpath. - for arg in $argv - switch $arg - # These - no can do our realpath - case -s --strip --no-symlinks -z --zero --relative-base\* --relative-to\* - __fish_print_help fish_realpath - return 2 + exit 0 + end - case -h --help --version - __fish_print_help fish_realpath - return 0 + function realpath -d "return an absolute path without symlinks" + if test -z "$argv" + printf "usage: %s%s%s %sfile%s …\n" (set_color -o) $_ (set_color normal) (set_color -u) (set_color normal) + echo " resolves files as absolute paths without symlinks" + return 1 + end - # Handle commands called with these arguments by - # dropping the arguments to protect fish_realpath from them. - # There are no sure things here. - case -e --canonicalize-existing --physical -P -q --quiet -m --canonicalize-missing - continue + # Loop over arguments - allow our realpath to work on more than one path per invocation + # like gnu/bsd realpath. + for arg in $argv + switch $arg + # These - no can do our realpath + case -s --strip --no-symlinks -z --zero --relative-base\* --relative-to\* + __fish_print_help realpath + return 2 - case "*" - fish_realpath $arg - end + case -h --help --version + __fish_print_help realpath + return 0 + + # Handle commands called with these arguments by dropping the arguments to protect + # realpath from them. There are no sure things here. + case -e --canonicalize-existing --physical -P -q --quiet -m --canonicalize-missing + continue + + case "*" + builtin realpath $arg end end end diff --git a/src/builtin.cpp b/src/builtin.cpp index 44030b9d9..7621cf46d 100644 --- a/src/builtin.cpp +++ b/src/builtin.cpp @@ -3106,7 +3106,7 @@ int builtin_false(parser_t &parser, io_streams_t &streams, wchar_t **argv) { /// directly. They should just use `realpath` which will fallback to this builtin if an external /// command cannot be found. This behaves like the external `realpath --canonicalize-existing`; /// that is, it requires all path components, including the final, to exist. -int builtin_fish_realpath(parser_t &parser, io_streams_t &streams, wchar_t **argv) { +int builtin_realpath(parser_t &parser, io_streams_t &streams, wchar_t **argv) { int argc = builtin_count_args(argv); if (argc != 2) { @@ -3116,11 +3116,10 @@ int builtin_fish_realpath(parser_t &parser, io_streams_t &streams, wchar_t **arg wchar_t *real_path = wrealpath(argv[1], NULL); if (real_path) { - // Yay! We could resolve the path. streams.out.append(real_path); free((void *)real_path); } else { - // We don't actually know why it failed. We should check errno + // We don't actually know why it failed. We should check errno. streams.err.append_format(_(L"%ls: Invalid path: %ls\n"), argv[0], argv[1]); return STATUS_BUILTIN_ERROR; } @@ -3167,8 +3166,6 @@ static const builtin_data_t builtin_datas[] = { {L"exit", &builtin_exit, N_(L"Exit the shell")}, {L"false", &builtin_false, N_(L"Return an unsuccessful result")}, {L"fg", &builtin_fg, N_(L"Send job to foreground")}, - {L"fish_realpath", &builtin_fish_realpath, - N_(L"Convert path to absolute path without symlinks")}, {L"for", &builtin_generic, N_(L"Perform a set of commands multiple times")}, {L"function", &builtin_generic, N_(L"Define a new function")}, {L"functions", &builtin_functions, N_(L"List or remove functions")}, @@ -3181,6 +3178,7 @@ static const builtin_data_t builtin_datas[] = { {L"pwd", &builtin_pwd, N_(L"Print the working directory")}, {L"random", &builtin_random, N_(L"Generate random number")}, {L"read", &builtin_read, N_(L"Read a line of input into variables")}, + {L"realpath", &builtin_realpath, N_(L"Convert path to absolute path without symlinks")}, {L"return", &builtin_return, N_(L"Stop the currently evaluated function")}, {L"set", &builtin_set, N_(L"Handle environment variables")}, {L"set_color", &builtin_set_color, N_(L"Set the terminal color")}, diff --git a/src/wutil.cpp b/src/wutil.cpp index b59de14d3..27bd0c508 100644 --- a/src/wutil.cpp +++ b/src/wutil.cpp @@ -335,20 +335,44 @@ void safe_perror(const char *message) { #ifdef HAVE_REALPATH_NULL wchar_t *wrealpath(const wcstring &pathname, wchar_t *resolved_path) { + if (pathname.size() == 0) return NULL; + + cstring real_path(""); cstring narrow_path = wcs2string(pathname); - char *narrow_res = realpath(narrow_path.c_str(), NULL); - if (!narrow_res) return NULL; - - wchar_t *res; - wcstring wide_res = str2wcstring(narrow_res); - if (resolved_path) { - wcslcpy(resolved_path, wide_res.c_str(), PATH_MAX); - res = resolved_path; - } else { - res = wcsdup(wide_res.c_str()); + // Strip trailing slashes. This is needed to be bug-for-bug compatible with GNU realpath which + // treats "/a//" as equivalent to "/a" whether or not /a exists. + while (narrow_path.size() > 1 && narrow_path.at(narrow_path.size() - 1) == '/') { + narrow_path.erase(narrow_path.size() - 1, 1); } + char *narrow_res = realpath(narrow_path.c_str(), NULL); + if (narrow_res) { + real_path.append(narrow_res); + } else { + int pathsep_idx = narrow_path.rfind('/'); + if (pathsep_idx == 0) { + // If the only pathsep is the first character then it's an absolute path with a + // single path component and thus doesn't need conversion. + real_path = narrow_path; + } else { + if (pathsep_idx == cstring::npos) { + // No pathsep means a single path component relative to pwd. + narrow_res = realpath(".", NULL); + if (!narrow_res) DIE("unexpected realpath(\".\") failure"); + pathsep_idx = 0; + } else { + // Only call realpath() on the portion up to the last component. + narrow_res = realpath(narrow_path.substr(0, pathsep_idx).c_str(), NULL); + if (!narrow_res) return NULL; + pathsep_idx++; + } + real_path.append(narrow_res); + // This test is to deal with pathological cases such as /../../x => //x. + if (real_path.size() > 1) real_path.append("/"); + real_path.append(narrow_path.substr(pathsep_idx, cstring::npos)); + } + } #if __APPLE__ && __DARWIN_C_LEVEL < 200809L // OS X Snow Leopard is broken with respect to the dynamically allocated buffer returned by // realpath(). It's not dynamically allocated so attempting to free that buffer triggers a @@ -357,7 +381,12 @@ wchar_t *wrealpath(const wcstring &pathname, wchar_t *resolved_path) { free(narrow_res); #endif - return res; + wcstring wreal_path = str2wcstring(real_path); + if (resolved_path) { + wcslcpy(resolved_path, wreal_path.c_str(), PATH_MAX); + return resolved_path; + } + return wcsdup(wreal_path.c_str()); } #else diff --git a/tests/fish_realpath.err b/tests/fish_realpath.err deleted file mode 100644 index f12c36186..000000000 --- a/tests/fish_realpath.err +++ /dev/null @@ -1,3 +0,0 @@ -fish_realpath: Invalid path: /this/better/be/an/invalid/path -fish_realpath: Invalid path: nonexistent-file -fish_realpath: Invalid path: ../test/data/fish-symlink/nonexistent-file-relative-to-a-symlink diff --git a/tests/fish_realpath.in b/tests/fish_realpath.in deleted file mode 100644 index f25309c99..000000000 --- a/tests/fish_realpath.in +++ /dev/null @@ -1,39 +0,0 @@ -# $XDG_DATA_HOME can itself be a relative path. So force it to an absolute -# path so we can remove it from any resolved paths below. This is needed -# because the contents of the fish_realpath.out file can't include any $PWD -# data since $PWD isn't under our control. -set -l data_home_realpath (fish_realpath $XDG_DATA_HOME) - -# A bogus absolute path is handled correctly and sets a failure status. -if not fish_realpath /this/better/be/an/invalid/path - echo first invalid path handled okay -end - -# A non-existent file relative to $PWD fails. -fish_realpath nonexistent-file - -# The simplest absolute path should undergo no transformation. -fish_realpath / - -# A single symlink to a directory is correctly resolved. -ln -s fish $XDG_DATA_HOME/fish-symlink -set -l real_path (fish_realpath $XDG_DATA_HOME/fish-symlink) -string replace "$data_home_realpath/" "" $real_path - -# A nonexistent file relative to a valid symlink to a directory fails. -# This depends on the symlink created by the previous test. -set -l real_path (fish_realpath $XDG_DATA_HOME/fish-symlink/nonexistent-file-relative-to-a-symlink) - -# A path with two symlinks, first to a directory, second to a file, is correctly resolved. -ln -s fish $XDG_DATA_HOME/fish-symlink2 -touch $XDG_DATA_HOME/fish/real_file -ln -s real_file $XDG_DATA_HOME/fish/symlink_file -set -l real_path (fish_realpath $XDG_DATA_HOME/fish-symlink/symlink_file) -string replace "$data_home_realpath/" "" $real_path - -# The $PWD should undergo no further transformations because it should already -# be a "realpath". -set -l real_path (fish_realpath $PWD) -string replace $PWD "pwd-resolved-to-itself" $real_path - -exit 0 diff --git a/tests/fish_realpath.out b/tests/fish_realpath.out deleted file mode 100644 index d14bca83b..000000000 --- a/tests/fish_realpath.out +++ /dev/null @@ -1,5 +0,0 @@ -first invalid path handled okay -/ -fish -fish/real_file -pwd-resolved-to-itself diff --git a/tests/realpath.err b/tests/realpath.err new file mode 100644 index 000000000..4a77361fa --- /dev/null +++ b/tests/realpath.err @@ -0,0 +1 @@ +realpath: Invalid path: /this/better/be/an/invalid/path diff --git a/tests/realpath.in b/tests/realpath.in new file mode 100644 index 000000000..fc5680839 --- /dev/null +++ b/tests/realpath.in @@ -0,0 +1,76 @@ +# $XDG_DATA_HOME can itself be a relative path. So force it to an absolute +# path so we can remove it from any resolved paths below. This is needed +# because the contents of the builtin realpath.out file can't include any $PWD +# data since $PWD isn't under our control. +set -l data_home_realpath (builtin realpath $XDG_DATA_HOME) + +# A bogus absolute path is handled correctly and sets a failure status. +if not builtin realpath /this/better/be/an/invalid/path + echo first invalid path handled okay +end + +# A non-existent file relative to $PWD succeeds. +set -l real_path (builtin realpath nonexistent-file) +if test "$real_path" = "$PWD/nonexistent-file" + echo nonexistent-file in PWD correctly converted +end + +# The simplest absolute path should undergo no transformation. +builtin realpath / + +# The second simplest absolute path should undergo no transformation. +builtin realpath /this-better-not-exist + +# Check that a pathological case is handled correctly (i.e., there is only one +# leading slash). +builtin realpath /../../x + +# Another pathological corner case. GNU realpath first strips trailing slashes +# so that "/a//" is converted to "/a" before performing the real path +# conversion. So, despite appearances, it considers "a" to be the last +# component in that case. +builtin realpath /abc/ +builtin realpath /def/// + +# A single symlink to a directory is correctly resolved. +ln -s fish $XDG_DATA_HOME/fish-symlink +set -l real_path (builtin realpath $XDG_DATA_HOME/fish-symlink) +set -l expected_real_path "$data_home_realpath/fish" +if test "$real_path" = "$expected_real_path" + echo "fish-symlink handled correctly" +else + echo "fish-symlink not handled correctly: $real_path != $expected_real_path" >&2 +end + +# A nonexistent file relative to a valid symlink to a directory gets converted. +# This depends on the symlink created by the previous test. +set -l real_path (builtin realpath $XDG_DATA_HOME/fish-symlink/nonexistent-file-relative-to-a-symlink) +set -l expected_real_path "$data_home_realpath/fish/nonexistent-file-relative-to-a-symlink" +if test "$real_path" = "$expected_real_path" + echo "fish-symlink/nonexistent-file-relative-to-a-symlink correctly converted" +else + echo "failure nonexistent-file-relative-to-a-symlink: $real_path != $expected_real_path" >&2 +end + +# A path with two symlinks, first to a directory, second to a file, is correctly resolved. +ln -s fish $XDG_DATA_HOME/fish-symlink2 +touch $XDG_DATA_HOME/fish/real_file +ln -s real_file $XDG_DATA_HOME/fish/symlink_file +set -l real_path (builtin realpath $XDG_DATA_HOME/fish-symlink/symlink_file) +set -l expected_real_path "$data_home_realpath/fish/real_file" +if test "$real_path" = "$expected_real_path" + echo "fish-symlink/symlink_file handled correctly" +else + echo "fish-symlink/symlink_file not handled correctly: $real_path != expected_real_path" >&2 +end + +# The $PWD should undergo no further transformations because it should already +# be a "realpath". +set -l real_path (builtin realpath $PWD) +if test "$real_path" = "$PWD" + echo "pwd-resolved-to-itself" +else + echo "unexpected pwd-resolved-to-itself failure: $real_path != $PWD" >&2 +end + +exit 0 diff --git a/tests/realpath.out b/tests/realpath.out new file mode 100644 index 000000000..5a8ac9014 --- /dev/null +++ b/tests/realpath.out @@ -0,0 +1,11 @@ +first invalid path handled okay +nonexistent-file in PWD correctly converted +/ +/this-better-not-exist +/x +/abc +/def +fish-symlink handled correctly +fish-symlink/nonexistent-file-relative-to-a-symlink correctly converted +fish-symlink/symlink_file handled correctly +pwd-resolved-to-itself