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/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.

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
+
+    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
+
+        # 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 -h --help --version
-                        __fish_print_help fish_realpath
-                        return 0
+                case -h --help --version
+                    __fish_print_help realpath
+                    return 0
 
-                        # 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
+                # 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 "*"
-                        fish_realpath $arg
-                end
+                case "*"
+                    builtin realpath $arg
             end
         end
     end

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")},

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;
+    // 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);
+    }
 
-    wchar_t *res;
-    wcstring wide_res = str2wcstring(narrow_res);
-    if (resolved_path) {
-        wcslcpy(resolved_path, wide_res.c_str(), PATH_MAX);
-        res = resolved_path;
+    char *narrow_res = realpath(narrow_path.c_str(), NULL);
+    if (narrow_res) {
+        real_path.append(narrow_res);
     } else {
-        res = wcsdup(wide_res.c_str());
+        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

tests/realpath.err

@@ -0,0 +1 @@
+realpath: Invalid path: /this/better/be/an/invalid/path

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

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