Merge branch 'virtualpwd'

This merges a switch to  a 'virtual PWD' model where it no longer resolves
symlinks by default.

CHANGELOG.md

@@ -21,6 +21,7 @@ fish 3.0 is a major release which brings with it both improvements in functional
 - Range expansion (`$foo[1..5]`) will now always go forward if only the end is negative, and in reverse if just the start is. This is to enable clamping to the last valid index without changing direction if the list has fewer elements than expected.
 - Background jobs not first `disown`'d will be reaped upon `exec`, bringing the behavior in line with that of `exit`.
 - `read` now uses `-s` as short for `--silent` (à la `bash`); `--shell`'s abbreviation (formerly `-s`) is now `-S` instead (#4490).
+- `cd` no longer resolves symlinks. fish now maintains a virtual path, matching other shells. (#3350).
 
 ## Notable fixes and improvements
 ### Syntax/semantic changes and new builtins

doc_src/faq.hdr

@@ -17,7 +17,6 @@
 - <a href='#faq-exported-uvar'>Why doesn't `set -Ux` (exported universal variables) seem to work?</a>
 - <a href='#faq-customize-colors'>How do I customize my syntax highlighting colors?</a>
 - <a href='#faq-update-manpage-completions'>How do I update man page completions?</a>
-- <a href='#faq-cwd-symlink'>Why does cd, pwd and other fish commands always resolve symlinked directories to their canonical path?</a>
 - <a href='#faq-cd-implicit'>I accidentally entered a directory path and fish changed directory. What happened?</a>
 - <a href='#faq-open'>The open command doesn't work.</a>
 - <a href='#faq-default'>How do I make fish my default shell?</a>
@@ -154,17 +153,6 @@ Use the web configuration tool, <a href="commands.html#fish_config">`fish_config
 
 Use the <a href="commands.html#fish_update_completions">`fish_update_completions`</a> command.
 
-<hr>
-\section faq-cwd-symlink Why does cd, $PWD and and various fish commands always resolve symlinked directories to their canonical path?
-
-<i>For example if `~/images` is a symlink to `~/Documents/Images`, if I write '`cd images`', my prompt will say `~/Documents/Images`, not `~/images`.</i>
-
-Because it is impossible to consistently keep symlinked directories unresolved. It is indeed possible to do this partially, and many other shells do so. But it was felt there are enough serious corner cases that this is a bad idea. Most such issues have to do with how '..' is handled, and are variations of the following example:
-
-Writing `cd images; ls ..` given the above directory structure would list the contents of `~/Documents`, not of `~`, even though using `cd ..` changes the current directory to `~`, and the prompt, the `pwd` builtin and many other directory information sources suggest that the current directory is `~/images` and its parent is `~`. This issue is not possible to fix without either making every single command into a builtin, breaking Unix semantics or implementing kludges in every single command. This issue can also be seen when doing IO redirection.
-
-Another related issue is that many programs that operate on recursive directory trees, like the find command, silently ignore symlinked directories. For example, ```find $PWD -name '*.txt'``` silently fails in shells that don't resolve symlinked paths.
-
 <hr>
 \section faq-cd-implicit I accidentally entered a directory path and fish changed directory. What happened?
 

doc_src/pwd.txt

@@ -9,4 +9,8 @@ pwd
 
 `pwd` outputs (prints) the current working directory.
 
-Note that `fish` always resolves symbolic links in the current directory path.
+The following options are available:
+
+- `-L`, Output the logical working directory, without resolving symlinks (default behavior).
+
+- `-P`, Output the physical working directory, with symlinks resolved.

share/functions/__fish_hg_prompt.fish

@@ -30,7 +30,7 @@ function __fish_hg_prompt --description 'Write out the hg prompt'
     # Find an hg directory above $PWD
     # without calling `hg root` because that's too slow
     set -l root
-    set -l dir $PWD
+    set -l dir (pwd -P)
     while test $dir != "/"
         if test -f $dir'/.hg/dirstate'
             set root $dir"/.hg"

src/builtin_cd.cpp

@@ -65,7 +65,11 @@ int builtin_cd(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         return STATUS_CMD_ERROR;
     }
 
-    if (wchdir(dir) != 0) {
+    // Prepend the PWD if we don't start with a slash, and then normalize the directory.
+    wcstring norm_dir =
+        normalize_path(string_prefixes_string(L"/", dir) ? dir : env_get_pwd_slash() + dir);
+
+    if (wchdir(norm_dir) != 0) {
         struct stat buffer;
         int status;
 
@@ -84,10 +88,6 @@ int builtin_cd(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
         return STATUS_CMD_ERROR;
     }
 
-    if (!env_set_pwd()) {
-        streams.err.append_format(_(L"%ls: Could not set PWD variable\n"), cmd);
-        return STATUS_CMD_ERROR;
-    }
-
+    env_set_one(L"PWD", ENV_EXPORT | ENV_GLOBAL, std::move(norm_dir));
     return STATUS_CMD_OK;
 }

src/builtin_pwd.cpp

@@ -6,35 +6,64 @@
 #include "common.h"
 #include "fallback.h"  // IWYU pragma: keep
 #include "io.h"
+#include "wgetopt.h"
 #include "wutil.h"  // IWYU pragma: keep
 
-/// The pwd builtin. We don't respect -P to resolve symbolic links because we
-/// try to always resolve them.
+/// The pwd builtin. Respect -P to resolve symbolic links. Respect -L to not do that (the default).
+static const wchar_t *short_options = L"LPh";
+static const struct woption long_options[] = {{L"help", no_argument, NULL, 'h'},
+                                              {NULL, 0, NULL, 0}};
 int builtin_pwd(parser_t &parser, io_streams_t &streams, wchar_t **argv) {
     UNUSED(parser);
     const wchar_t *cmd = argv[0];
     int argc = builtin_count_args(argv);
-    help_only_cmd_opts_t opts;
-
-    int optind;
-    int retval = parse_help_only_cmd_opts(opts, &optind, argc, argv, parser, streams);
-    if (retval != STATUS_CMD_OK) return retval;
-
-    if (opts.print_help) {
-        builtin_print_help(parser, streams, cmd, streams.out);
-        return STATUS_CMD_OK;
+    bool resolve_symlinks = false;
+    wgetopter_t w;
+    int opt;
+    while ((opt = w.wgetopt_long(argc, argv, short_options, long_options, NULL)) != -1) {
+        switch (opt) {
+            case 'L':
+                resolve_symlinks = false;
+                break;
+            case 'P':
+                resolve_symlinks = true;
+                break;
+            case 'h':
+                builtin_print_help(parser, streams, cmd, streams.out);
+                return STATUS_CMD_OK;
+            case '?': {
+                builtin_unknown_option(parser, streams, cmd, argv[w.woptind - 1]);
+                return STATUS_INVALID_ARGS;
+            }
+            default: {
+                DIE("unexpected retval from wgetopt_long");
+                break;
+            }
+        }
     }
 
-    if (optind != argc) {
+    if (w.woptind != argc) {
         streams.err.append_format(BUILTIN_ERR_ARG_COUNT1, cmd, 0, argc - 1);
         return STATUS_INVALID_ARGS;
     }
 
-    wcstring res = wgetcwd();
-    if (res.empty()) {
+    wcstring pwd;
+    if (auto tmp = env_get(L"PWD")) {
+        pwd = tmp->as_string();
+    }
+    if (resolve_symlinks) {
+        if (auto real_pwd = wrealpath(pwd)) {
+            pwd = std::move(*real_pwd);
+        } else {
+            const char *error = strerror(errno);
+            streams.err.append_format(L"%ls: realpath failed:", cmd, error);
+            return STATUS_CMD_ERROR;
+        }
+    }
+    if (pwd.empty()) {
         return STATUS_CMD_ERROR;
     }
-    streams.out.append(res);
+    streams.out.append(pwd);
     streams.out.push_back(L'\n');
     return STATUS_CMD_OK;
 }

src/env.cpp

@@ -663,15 +663,15 @@ static void env_set_termsize() {
     if (rows.missing_or_empty()) env_set_one(L"LINES", ENV_GLOBAL, DFLT_TERM_ROW_STR);
 }
 
-bool env_set_pwd() {
+/// Update the PWD variable directory from the result of getcwd().
+void env_set_pwd_from_getcwd() {
     wcstring cwd = wgetcwd();
     if (cwd.empty()) {
         debug(0,
               _(L"Could not determine current working directory. Is your locale set correctly?"));
-        return false;
+        return;
     }
-    env_set_one(L"PWD", ENV_EXPORT | ENV_GLOBAL, cwd);
-    return true;
+    env_set_one(L"PWD", ENV_EXPORT | ENV_GLOBAL, std::move(cwd));
 }
 
 /// Allow the user to override the limit on how much data the `read` command will process.
@@ -982,7 +982,11 @@ void env_init(const struct config_paths_t *paths /* or NULL */) {
         }
     }
 
-    env_set_pwd();         // initialize the PWD variable
+    // initialize the PWD variable if necessary
+    // Note we may inherit a virtual PWD that doesn't match what getcwd would return; respect that.
+    if (env_get(L"PWD").missing_or_empty()) {
+        env_set_pwd_from_getcwd();
+    }
     env_set_termsize();    // initialize the terminal size variables
     env_set_read_limit();  // initialize the read_byte_limit
 

src/env.h

@@ -156,8 +156,8 @@ void env_set_argv(const wchar_t *const *argv);
 /// Returns all variable names.
 wcstring_list_t env_get_names(int flags);
 
-/// Update the PWD variable directory.
-bool env_set_pwd();
+/// Update the PWD variable based on the result of getcwd.
+void env_set_pwd_from_getcwd();
 
 /// Returns the PWD with a terminating slash.
 wcstring env_get_pwd_slash();

src/expand.cpp

@@ -772,7 +772,9 @@ static void expand_home_directory(wcstring &input) {
             }
         }
 
-        maybe_t<wcstring> realhome = (home ? wrealpath(*home) : none());
+        maybe_t<wcstring> realhome;
+        if (home) realhome = normalize_path(*home);
+
         if (realhome) {
             input.replace(input.begin(), input.begin() + tail_idx, *realhome);
         } else {

src/fish_tests.cpp

@@ -172,7 +172,7 @@ static bool pushd(const char *path) {
         return false;
     }
 
-    env_set_pwd();
+    env_set_pwd_from_getcwd();
     return true;
 }
 
@@ -183,7 +183,7 @@ static void popd() {
     }
     free((void *)old_cwd);
     pushed_dirs.pop_back();
-    env_set_pwd();
+    env_set_pwd_from_getcwd();
 }
 
 // The odd formulation of these macros is to avoid "multiple unary operator" warnings from oclint
@@ -4664,6 +4664,22 @@ void test_layout_cache() {
     do_test(seqs.find_prompt_layout(L"whatever")->line_count == 100);
 }
 
+void test_normalize_path() {
+    say(L"Testing path normalization");
+    do_test(normalize_path(L"") == L".");
+    do_test(normalize_path(L"..") == L"..");
+    do_test(normalize_path(L"./") == L".");
+    do_test(normalize_path(L"////abc") == L"//abc");
+    do_test(normalize_path(L"/abc") == L"/abc");
+    do_test(normalize_path(L"/abc/") == L"/abc");
+    do_test(normalize_path(L"/abc/..def/") == L"/abc/..def");
+    do_test(normalize_path(L"//abc/../def/") == L"//def");
+    do_test(normalize_path(L"abc/../abc/../abc/../abc") == L"abc");
+    do_test(normalize_path(L"../../") == L"../..");
+    do_test(normalize_path(L"foo/./bar") == L"foo/bar");
+    do_test(normalize_path(L"foo/././bar/.././baz") == L"foo/baz");
+}
+
 /// Main test.
 int main(int argc, char **argv) {
     UNUSED(argc);
@@ -4762,6 +4778,7 @@ int main(int argc, char **argv) {
     if (should_test_function("illegal_command_exit_code")) test_illegal_command_exit_code();
     if (should_test_function("maybe")) test_maybe();
     if (should_test_function("layout_cache")) test_layout_cache();
+    if (should_test_function("normalize")) test_normalize_path();
     // history_tests_t::test_history_speed();
 
     say(L"Encountered %d errors in low-level tests", err_count);

src/wutil.cpp

@@ -433,6 +433,40 @@ maybe_t<wcstring> wrealpath(const wcstring &pathname) {
     return str2wcstring(real_path);
 }
 
+wcstring normalize_path(const wcstring &path) {
+    // Count the leading slashes.
+    // Preserve up to 2.
+    const wchar_t sep = L'/';
+    size_t leading_slashes = 0;
+    for (wchar_t c : path) {
+        if (c != sep) break;
+        leading_slashes++;
+    }
+
+    wcstring_list_t comps = split_string(path, sep);
+    wcstring_list_t new_comps;
+    for (wcstring &comp : comps) {
+        if (comp.empty() || comp == L".") {
+            continue;
+        } else if (comp == L"..") {
+            if (new_comps.empty() || new_comps.back() == L"..") {
+                // We underflowed the ..s, retain this component.
+                new_comps.push_back(L"..");
+            } else {
+                new_comps.pop_back();
+            }
+        } else {
+            new_comps.push_back(std::move(comp));
+        }
+    }
+
+    // Prepend up to two leading slashes (as empty components).
+    new_comps.insert(new_comps.begin(), leading_slashes > 2 ? 2 : leading_slashes, wcstring());
+    // Ensure e.g. './' normalizes to '.' and not empty.
+    if (new_comps.empty()) new_comps.push_back(L".");
+    return join_strings(new_comps, sep);
+}
+
 wcstring wdirname(const wcstring &path) {
     char *tmp = wcs2str(path);
     char *narrow_res = dirname(tmp);

src/wutil.h

@@ -68,6 +68,12 @@ int wchdir(const wcstring &dir);
 /// \returns the canonicalized path, or none if the path is invalid.
 maybe_t<wcstring> wrealpath(const wcstring &pathname);
 
+/// Given an input path, "normalize" it:
+/// 1. Collapse multiple /s into a single /, except maybe at the beginning.
+/// 2. .. goes up a level.
+/// 3. Remove /./ in the middle.
+wcstring normalize_path(const wcstring &path);
+
 /// Wide character version of readdir().
 bool wreaddir(DIR *dir, wcstring &out_name);
 bool wreaddir_resolving(DIR *dir, const std::wstring &dir_path, wcstring &out_name,

tests/cd.err

@@ -0,0 +1,3 @@
+
+####################
+# cd symlink non-resolution

tests/cd.in

@@ -0,0 +1,9 @@
+logmsg cd symlink non-resolution
+set real (mktemp -d)
+set link (mktemp -u)
+ln -s $real $link
+cd $link
+test "$PWD" = "$link" || echo "\$PWD != \$link:"\n   "\$PWD: $PWD"\n   "\$link: $link"\n
+test (pwd) = "$link" || echo "(pwd) != \$link:"\n   "\$PWD: "(pwd)\n   "\$link: $link"\n
+test (pwd -P) = "$real" || echo "(pwd -P) != \$real:"\n   "\$PWD: $PWD"\n   "\$real: $real"\n
+test (pwd -P -L) = "$link" || echo "(pwd -P -L) != \$link:"\n   "\$PWD: $PWD"\n   "\$link: $link"\n

tests/cd.out

@@ -0,0 +1,3 @@
+
+####################
+# cd symlink non-resolution

tests/expansion.in

@@ -115,8 +115,8 @@ cd $saved
 mkdir $tmpdir/realhome
 ln -s $tmpdir/realhome $tmpdir/linkhome
 set expandedtilde (env HOME=$tmpdir/linkhome ../test/root/bin/fish -c 'echo ~')
-if test $expandedtilde != $tmpdir/realhome
-	echo '~ expands to' $expandedtilde ' - expected ' $tmpdir/realhome
+if test $expandedtilde != $tmpdir/linkhome
+	echo '~ expands to' $expandedtilde ' - expected ' $tmpdir/linkhome
 end
 unlink $tmpdir/linkhome
 rmdir $tmpdir/realhome

tests/test_functions/mktemp.fish

@@ -18,6 +18,8 @@ function mktemp
                 set opts $opts d
             case -t
                 set opts $opts t
+            case -u
+                set opts $opts u
             case --
                 set -e argv[1]
                 break
@@ -69,6 +71,9 @@ function mktemp
     end
 
     set -l args
+    if contains u $opts
+        set args $args -u
+    end
     if contains d $opts
         set args $args -d
     end
@@ -88,7 +93,7 @@ function mktemp
     end
     set args $args $template
 
-    command mktemp $args
+    realpath (command mktemp $args)
 end
 
 function _mktemp_help