Fix for https://github.com/ridiculousfish/fishfish/issues/3

cd's special autosuggestion was broken, now fixed. Also, there's some tests for it now.

common.cpp

@@ -84,7 +84,9 @@ parts of fish.
 
 struct termios shell_modes;      
 
+// Note we foolishly assume that pthread_t is just a primitive. But it might be a struct.
 static pthread_t main_thread_id = 0;
+static bool thread_assertions_configured_for_testing = false;
 
 wchar_t ellipsis_char;
 
@@ -1926,6 +1928,10 @@ void set_main_thread() {
  	main_thread_id = pthread_self();
 }
 
+void configure_thread_assertions_for_testing(void) {
+    thread_assertions_configured_for_testing = true;
+}
+
 /* Notice when we've forked */
 static pid_t initial_pid;
 
@@ -1950,7 +1956,7 @@ bool is_main_thread() {
 
 void assert_is_main_thread(const char *who)
 {
-    if (! is_main_thread()) {
+    if (! is_main_thread() && ! thread_assertions_configured_for_testing) {
         fprintf(stderr, "Warning: %s called off of main thread. Break on debug_thread_error to debug.\n", who);
         debug_thread_error();
     }
@@ -1966,7 +1972,7 @@ void assert_is_not_forked_child(const char *who)
 
 void assert_is_background_thread(const char *who)
 {
-    if (is_main_thread()) {
+    if (is_main_thread() && ! thread_assertions_configured_for_testing) {
         fprintf(stderr, "Warning: %s called on the main thread (may block!). Break on debug_thread_error to debug.\n", who);
         debug_thread_error();
     }

common.h

@@ -721,6 +721,9 @@ double timef();
 void set_main_thread();
 bool is_main_thread();
 
+/** Configures thread assertions for testing */
+void configure_thread_assertions_for_testing();
+
 /** Set up a guard to complain if we try to do certain things (like take a lock) after calling fork */
 void setup_fork_guards(void);
 

fish_tests.cpp

@@ -579,6 +579,45 @@ static void test_path()
 	}	
 }
 
+/** Test is_potential_path */
+static void test_is_potential_path()
+{
+    say(L"Testing is_potential_path");
+    if (system("rm -Rf /tmp/is_potential_path_test/")) {
+        err(L"Failed to remove /tmp/is_potential_path_test/");
+    }
+    
+    /* Directories */
+    if (system("mkdir -p /tmp/is_potential_path_test/alpha/")) err(L"mkdir failed");
+    if (system("mkdir -p /tmp/is_potential_path_test/beta/")) err(L"mkdir failed");
+    
+    /* Files */
+    if (system("touch /tmp/is_potential_path_test/aardvark")) err(L"touch failed");
+    if (system("touch /tmp/is_potential_path_test/gamma")) err(L"touch failed");
+    
+    const wcstring wd = L"/tmp/is_potential_path_test/";
+    const wcstring_list_t wds(1, wd);
+    
+    wcstring tmp;
+    assert(is_potential_path(L"al", wds, true, &tmp) && tmp == L"alpha/");
+    assert(is_potential_path(L"alpha/", wds, true, &tmp) && tmp == L"alpha/");
+    assert(is_potential_path(L"aard", wds, false, &tmp) && tmp == L"aardvark");
+    
+    assert(! is_potential_path(L"balpha/", wds, true, &tmp));
+    assert(! is_potential_path(L"aard", wds, true, &tmp));
+    assert(! is_potential_path(L"aarde", wds, true, &tmp));
+    assert(! is_potential_path(L"aarde", wds, false, &tmp));
+    
+    assert(is_potential_path(L"/tmp/is_potential_path_test/aardvark", wds, false, &tmp) && tmp == L"/tmp/is_potential_path_test/aardvark");
+    assert(is_potential_path(L"/tmp/is_potential_path_test/al", wds, true, &tmp) && tmp == L"/tmp/is_potential_path_test/alpha/");
+    assert(is_potential_path(L"/tmp/is_potential_path_test/aardv", wds, false, &tmp) && tmp == L"/tmp/is_potential_path_test/aardvark");
+    
+    assert(! is_potential_path(L"/tmp/is_potential_path_test/aardvark", wds, true, &tmp));
+    assert(! is_potential_path(L"/tmp/is_potential_path_test/al/", wds, false, &tmp));
+    assert(! is_potential_path(L"/tmp/is_potential_path_test/ar", wds, false, &tmp));
+    
+}
+
 /** Test the 'test' builtin */
 int builtin_test( parser_t &parser, wchar_t **argv );
 static bool run_test_test(int expected, wcstring_list_t &lst) {
@@ -927,6 +966,7 @@ int main( int argc, char **argv )
 {
 	setlocale( LC_ALL, "" );
 	srand( time( 0 ) );
+    configure_thread_assertions_for_testing();
 
 	program_name=L"(ignore)";
 	
@@ -951,6 +991,7 @@ int main( int argc, char **argv )
 	test_expand();
     test_test();
 	test_path();
+    test_is_potential_path();
     test_colors();
     test_autosuggest();
     history_tests_t::test_history();

highlight.cpp

@@ -103,8 +103,9 @@ static wcstring apply_working_directory(const wcstring &path, const wcstring &wo
     }
 }
 
-/* Tests whether the specified string cpath is the prefix of anything we could cd to. directories is a list of possible parent directories (typically either the working directory, or the cdpath). This does I/O! */
+/* Tests whether the specified string cpath is the prefix of anything we could cd to. directories is a list of possible parent directories (typically either the working directory, or the cdpath). This does I/O!
-static bool is_potential_path( const wcstring &cpath, const wcstring_list_t &directories, bool require_dir = false, wcstring *out_path = NULL)
+*/
+bool is_potential_path(const wcstring &const_path, const wcstring_list_t &directories, bool require_dir, wcstring *out_path)
 {
     ASSERT_IS_BACKGROUND_THREAD();
     
@@ -113,7 +114,7 @@ static bool is_potential_path( const wcstring &cpath, const wcstring_list_t &dir
 	int has_magic = 0;
 	bool result = false;
     
-    wcstring path(cpath);
+    wcstring path(const_path);
     expand_tilde(path);
     if (! unescape_string(path, 1))
         return false;
@@ -196,22 +197,28 @@ static bool is_potential_path( const wcstring &cpath, const wcstring_list_t &dir
                         *out_path = clean_path;
                 }
                 else if ((dir = wopendir(dir_name))) {
-                    /* We opened the dir_name; look for a string where the base name prefixes it */
+                    // We opened the dir_name; look for a string where the base name prefixes it
                     wcstring ent;
                     
                     // Don't ask for the is_dir value unless we care, because it can cause extra filesystem acces */
                     bool is_dir = false;
                     while (wreaddir_resolving(dir, dir_name, ent, require_dir ? &is_dir : NULL))
                     {
-                        /* TODO: support doing the right thing on case-insensitive filesystems like HFS+ */
+                        // TODO: support doing the right thing on case-insensitive filesystems like HFS+
                         if (string_prefixes_string(base_name, ent) && (! require_dir || is_dir))
                         {
                             result = true;
                             if (out_path) {
-                                out_path->assign(dir_name);
+                            
-                                out_path->push_back(L'/');
+                                /* We want to return the path in the same "form" as it was given. Take the given path, get its basename. Append that to the output if the given path has the basename as the prefix (which it won't if the given path contains no slashes). Then append the directory entry. */
+                                
+                                out_path->clear();
+                                const wcstring path_base = wdirname(const_path);
+                                if (string_prefixes_string(path_base, const_path)) {
+                                    out_path->append(path_base);
+                                    out_path->push_back(L'/');
+                                }
                                 out_path->append(ent);
-                                path_make_canonical(*out_path);
                                 /* We actually do want a trailing / for directories, since it makes autosuggestion a bit nicer */
                                 if (is_dir)
                                     out_path->push_back(L'/');
@@ -230,23 +237,35 @@ static bool is_potential_path( const wcstring &cpath, const wcstring_list_t &dir
 
 /* Given a string, return whether it prefixes a path that we could cd into. Return that path in out_path */
 static bool is_potential_cd_path(const wcstring &path, const wcstring &working_directory, wcstring *out_path) {
-    /* Get the CDPATH */
-    env_var_t cdpath = env_get_string(L"CDPATH");
-    if (cdpath.missing_or_empty())
-        cdpath = L".";
-    
-    /* Tokenize it into directories */
     wcstring_list_t directories;
-    wcstokenizer tokenizer(cdpath, ARRAY_SEP_STR);
+    
-    wcstring next_path;
+    if (string_prefixes_string(L"./", path)) {
-    while (tokenizer.next(next_path))
+        /* Ignore the CDPATH in this case; just use the working directory */
-    {
+        directories.push_back(working_directory);
-        /* Ensure that we use the working directory for relative cdpaths like "." */
+    } else {
-        directories.push_back(apply_working_directory(next_path, working_directory));
+        /* Get the CDPATH */
+        env_var_t cdpath = env_get_string(L"CDPATH");
+        if (cdpath.missing_or_empty())
+            cdpath = L".";
+        
+        /* Tokenize it into directories */
+        wcstokenizer tokenizer(cdpath, ARRAY_SEP_STR);
+        wcstring next_path;
+        while (tokenizer.next(next_path))
+        {
+            /* Ensure that we use the working directory for relative cdpaths like "." */
+            directories.push_back(apply_working_directory(next_path, working_directory));
+        }
     }
     
-    /* Call is_potential_path */
+    /* Call is_potential_path with all of these directories */
-    return is_potential_path(path, directories, true /* require_dir */, out_path);
+    bool result = is_potential_path(path, directories, true /* require_dir */, out_path);
+#if 0
+    if (out_path) {
+        printf("%ls -> %ls\n", path.c_str(), out_path->c_str());
+    }
+#endif
+    return result;
 }
 
 rgb_color_t highlight_get_color( int highlight, bool is_background )
@@ -724,7 +743,6 @@ bool autosuggest_suggest_special(const wcstring &str, const wcstring &working_di
         
     ASSERT_IS_BACKGROUND_THREAD();
     
-    
     /* Parse the string */
     wcstring parsed_command;
     wcstring_list_t parsed_arguments;
@@ -741,7 +759,7 @@ bool autosuggest_suggest_special(const wcstring &str, const wcstring &working_di
         /* We always return true because we recognized the command. This prevents us from falling back to dumber algorithms; for example we won't suggest a non-directory for the cd command. */
         result = true;
         outSuggestion.clear();
-        
+
         if (is_potential_cd_path(dir, working_directory, &suggested_path)) {
 
             /* suggested_path needs to actually have dir as a prefix (perhaps with different case). Handle stuff like ./ */

highlight.h

@@ -115,6 +115,11 @@ bool autosuggest_special_validate_from_history(const wcstring &str, const wcstri
 */
 bool autosuggest_suggest_special(const wcstring &str, const wcstring &working_directory, wcstring &outString);
 
+/* Tests whether the specified string cpath is the prefix of anything we could cd to. directories is a list of possible parent directories (typically either the working directory, or the cdpath). This does I/O!
+
+    This is used only internally to this file, and is exposed only for testing.
+*/
+bool is_potential_path(const wcstring &const_path, const wcstring_list_t &directories, bool require_dir = false, wcstring *out_path = NULL);
 
 #endif