Migrate PATH-completion logic from complete.cpp to expand.cpp

Prior to this fix, when completing a command that doesn't have a /, we
would prepend each component of PATH and then expand the whole thing. So
any special characters in the PATH would be interpreted when performing
tab completion.

With this fix, we pull the PATH resolution out of complete.cpp and
migrate it to expand.cpp. This unifies nicely with the CDPATH resolution
already in that file. This requires introducing a new expand flag
EXPAND_SPECIAL_FOR_COMMAND, which is analogous to EXPAND_SPECIAL_CD
(which is renamed to EXPAND_SPECIAL_FOR_CD). This flag tells expand to
resolve paths against PATH instead of the working directory.

Fixes #952

6 files changed, 74 insertions, 68 deletions

diff --git a/src/complete.cpp b/src/complete.cpp
index 8af2c822..5c817b8d 100644
--- a/src/complete.cpp
+++ b/src/complete.cpp
@@ -851,8 +851,7 @@ void completer_t::complete_cmd(const wcstring &str_cmd, bool use_function, bool
 
     if (use_command)
     {
-
-        if (expand_string(str_cmd, &this->completions, EXPAND_FOR_COMPLETIONS | EXECUTABLES_ONLY | this->expand_flags(), NULL) != EXPAND_ERROR)
+        if (expand_string(str_cmd, &this->completions, EXPAND_SPECIAL_FOR_COMMAND | EXPAND_FOR_COMPLETIONS | EXECUTABLES_ONLY | this->expand_flags(), NULL) != EXPAND_ERROR)
         {
             if (this->wants_descriptions())
             {
@@ -869,52 +868,8 @@ void completer_t::complete_cmd(const wcstring &str_cmd, bool use_function, bool
     }
     if (str_cmd.find(L'/') == wcstring::npos && str_cmd.at(0) != L'~')
     {
-        if (use_command)
-        {
-
-            const env_var_t path = this->vars.get(L"PATH");
-            if (!path.missing())
-            {
-                wcstring base_path;
-                wcstokenizer tokenizer(path, ARRAY_SEP_STR);
-                while (tokenizer.next(base_path))
-                {
-                    if (base_path.empty())
-                        continue;
-
-                    /* Make sure the base path ends with a slash */
-                    if (base_path.at(base_path.size() - 1) != L'/')
-                        base_path.push_back(L'/');
-
-                    wcstring nxt_completion = base_path;
-                    nxt_completion.append(str_cmd);
-
-                    size_t prev_count =  this->completions.size();
-                    expand_flags_t expand_flags = EXPAND_FOR_COMPLETIONS | EXECUTABLES_ONLY | EXPAND_NO_FUZZY_DIRECTORIES | this->expand_flags();
-                    if (expand_string(nxt_completion,
-                                      &this->completions,
-                                      expand_flags, NULL) != EXPAND_ERROR)
-                    {
-                        /* For all new completions, if COMPLETE_REPLACES_TOKEN is set, then use only the last path component */
-                        for (size_t i=prev_count; i< this->completions.size(); i++)
-                        {
-                            completion_t &c =  this->completions.at(i);
-                            if (c.flags & COMPLETE_REPLACES_TOKEN)
-                            {
-
-                                c.completion.erase(0, base_path.size());
-                            }
-                        }
-                    }
-                }
-                if (this->wants_descriptions())
-                    this->complete_cmd_desc(str_cmd);
-            }
-        }
-
         if (use_function)
         {
-            //function_get_names( &possible_comp, cmd[0] == L'_' );
             wcstring_list_t names = function_get_names(str_cmd.at(0) == L'_');
             for (size_t i=0; i < names.size(); i++)
             {
@@ -1361,7 +1316,7 @@ void completer_t::complete_param_expand(const wcstring &str, bool do_file, bool
     
     if (handle_as_special_cd && do_file)
     {
-        flags |= DIRECTORIES_ONLY | EXPAND_SPECIAL_CD | EXPAND_NO_DESCRIPTIONS;
+        flags |= DIRECTORIES_ONLY | EXPAND_SPECIAL_FOR_CD | EXPAND_NO_DESCRIPTIONS;
     }
 
     /* Squelch file descriptions per issue 254 */
diff --git a/src/expand.cpp b/src/expand.cpp
index de67ffa8..cb68eb98 100644
--- a/src/expand.cpp
+++ b/src/expand.cpp
@@ -1800,36 +1800,47 @@ static expand_error_t expand_stage_wildcards(const wcstring &input, std::vector<
         
         const wcstring working_dir = env_get_pwd_slash();
         wcstring_list_t effective_working_dirs;
-        if (! (flags & EXPAND_SPECIAL_CD))
+        bool for_cd = !!(flags & EXPAND_SPECIAL_FOR_CD);
+        bool for_command = !!(flags & EXPAND_SPECIAL_FOR_COMMAND);
+        if (!for_cd && !for_command)
         {
             /* Common case */
             effective_working_dirs.push_back(working_dir);
         }
         else
         {
-            /* Ignore the CDPATH if we start with ./ or / */
-            if (string_prefixes_string(L"./", path_to_expand))
-            {
-                effective_working_dirs.push_back(working_dir);
-            }
-            else if (string_prefixes_string(L"/", path_to_expand))
+            /* Either EXPAND_SPECIAL_FOR_COMMAND or EXPAND_SPECIAL_FOR_CD. We can handle these mostly the same.
+               There's the following differences:
+               1. An empty CDPATH should be treated as '.', but an empty PATH should be left empty (no commands can be found).
+               2. PATH is only "one level," while CDPATH is multiple levels. That is, input like 'foo/bar' should resolve
+                  against CDPATH, but not PATH.
+
+               In either case, we ignore the path if we start with ./ or /.
+               Also ignore it if we are doing command completion and we contain a slash, per IEEE 1003.1, chapter 8 under PATH
+             */
+            if (string_prefixes_string(L"./", path_to_expand) ||
+                string_prefixes_string(L"/", path_to_expand) ||
+                (for_command && path_to_expand.find(L'/') != wcstring::npos))
             {
                 effective_working_dirs.push_back(working_dir);
             }
             else
             {
-                /* Get the CDPATH and cwd. Perhaps these should be passed in. */
-                env_var_t cdpath = env_get_string(L"CDPATH");
-                if (cdpath.missing_or_empty())
-                    cdpath = L".";
+                /* Get the PATH/CDPATH and cwd. Perhaps these should be passed in.
+                   An empty CDPATH implies just the current directory, while an empty PATH is left empty.
+                 */
+                env_var_t paths = env_get_string(for_cd ? L"CDPATH" : L"PATH");
+                if (paths.missing_or_empty())
+                    paths = for_cd ? L"." : L"";
                 
                 /* Tokenize it into directories */
-                wcstokenizer tokenizer(cdpath, ARRAY_SEP_STR);
-                wcstring next_cd_path;
-                while (tokenizer.next(next_cd_path))
+                wcstokenizer tokenizer(paths, ARRAY_SEP_STR);
+                wcstring next_path;
+                while (tokenizer.next(next_path))
                 {
                     /* Ensure that we use the working directory for relative cdpaths like "." */
-                    effective_working_dirs.push_back(path_apply_working_directory(next_cd_path, working_dir));
+                    effective_working_dirs.push_back(path_apply_working_directory(next_path, working_dir));
+
                 }
             }
         }
diff --git a/src/expand.h b/src/expand.h
index 6ae91cca..fafbcd2a 100644
--- a/src/expand.h
+++ b/src/expand.h
@@ -52,8 +52,12 @@ enum
     // Disallow directory abbreviations like /u/l/b for /usr/local/bin. Only
     // applicable if EXPAND_FUZZY_MATCH is set.
     EXPAND_NO_FUZZY_DIRECTORIES = 1 << 10,
-    // Do expansions specifically to support cd (CDPATH, etc).
-    EXPAND_SPECIAL_CD = 1 << 11
+    // Do expansions specifically to support cd
+    // This means using CDPATH as a list of potential working directories
+    EXPAND_SPECIAL_FOR_CD = 1 << 11,
+    // Do expansions specifically to support external command completions.
+    // This means using PATH as a list of potential working directories
+    EXPAND_SPECIAL_FOR_COMMAND = 1 << 12
 };
 typedef int expand_flags_t;
 
diff --git a/src/wildcard.cpp b/src/wildcard.cpp
index db211818..202b836d 100644
--- a/src/wildcard.cpp
+++ b/src/wildcard.cpp
@@ -789,8 +789,8 @@ class wildcard_expander_t
                 c.prepend_token_prefix(this->original_base);
             }
             
-            /* Hack. Implement EXPAND_SPECIAL_CD by descending the deepest unique hierarchy we can, and then appending any components to each new result. */
-            if (flags & EXPAND_SPECIAL_CD)
+            /* Hack. Implement EXPAND_SPECIAL_FOR_CD by descending the deepest unique hierarchy we can, and then appending any components to each new result. */
+            if (flags & EXPAND_SPECIAL_FOR_CD)
             {
                 wcstring unique_hierarchy = this->descend_unique_hierarchy(abs_path);
                 if (! unique_hierarchy.empty())
@@ -1138,8 +1138,8 @@ int wildcard_expand_string(const wcstring &wc, const wcstring &working_directory
     /* Fuzzy matching only if we're doing completions */
     assert((flags & (EXPAND_FUZZY_MATCH | EXPAND_FOR_COMPLETIONS)) != EXPAND_FUZZY_MATCH);
     
-    /* EXPAND_SPECIAL_CD requires DIRECTORIES_ONLY and EXPAND_FOR_COMPLETIONS and EXPAND_NO_DESCRIPTIONS */
-    assert(!(flags & EXPAND_SPECIAL_CD) ||
+    /* EXPAND_SPECIAL_FOR_CD requires DIRECTORIES_ONLY and EXPAND_FOR_COMPLETIONS and EXPAND_NO_DESCRIPTIONS */
+    assert(!(flags & EXPAND_SPECIAL_FOR_CD) ||
            ((flags & DIRECTORIES_ONLY) && (flags & EXPAND_FOR_COMPLETIONS) && (flags & EXPAND_NO_DESCRIPTIONS)));
     
     /* Hackish fix for 1631. We are about to call c_str(), which will produce a string truncated at any embedded nulls. We could fix this by passing around the size, etc. However embedded nulls are never allowed in a filename, so we just check for them and return 0 (no matches) if there is an embedded null. */
diff --git a/tests/test6.in b/tests/test6.in
index 252eb606..0d6ef487 100644
--- a/tests/test6.in
+++ b/tests/test6.in
@@ -72,3 +72,37 @@ if begin; rm -rf test6.tmp.dir; and mkdir test6.tmp.dir; end
 else
     echo "error: could not create temp environment" >&2
 end
+
+# Test command expansion with parened PATHs (#952)
+begin
+  set -l parened_path $PWD/'test6.tmp2.(paren).dir'
+  set -l parened_subpath $parened_path/subdir
+  if not begin
+        rm -rf $parened_path
+	    and mkdir $parened_path
+		and mkdir $parened_subpath
+	    and ln -s /bin/ls $parened_path/'__test6_(paren)_command'
+	    and ln -s /bin/ls $parened_subpath/'__test6_subdir_(paren)_command'
+      end
+    echo "error: could not create command expansion temp environment" >&2
+  end
+  
+  # Verify that we can expand commands when PATH has parens
+  set -l PATH $parened_path $PATH
+  set -l completed (complete -C__test6_ | cut -f 1 -d \t)
+  if test "$completed" = '__test6_(paren)_command'
+    echo "Command completion with parened PATHs test passed"
+  else
+    echo "Command completion with parened PATHs test failed. Expected __test6_(paren)_command, got $completed" >&2
+  end
+  
+  # Verify that commands with intermediate slashes do NOT expand with respect to PATH
+  set -l completed (complete -Csubdir/__test6_subdir)
+  if test -z "$completed"
+    echo "Command completion with intermediate slashes passed"
+  else
+    echo "Command completion with intermediate slashes: should output nothing, instead got $completed" >&2
+  end
+  
+  rm -rf $parened_path
+end
diff --git a/tests/test6.out b/tests/test6.out
index 268b05ea..ca6e6906 100644
--- a/tests/test6.out
+++ b/tests/test6.out
@@ -33,3 +33,5 @@ CCCC:
 implicit cd complete works
 no implicit cd complete after 'command'
 PATH does not cause incorrect implicit cd
+Command completion with parened PATHs test passed
+Command completion with intermediate slashes passed