Teach fish how to put completion data inside a closing quote

Fixes https://github.com/fish-shell/fish-shell/issues/552

complete.h

@@ -80,27 +80,20 @@ enum
 
        Warning: The contents of the completion_t structure is actually
        different if this flag is set! Specifically, the completion string
-       contains the _entire_ completion token, not only the current
+       contains the _entire_ completion token, not merely its suffix.
     */
     COMPLETE_NO_CASE = 1 << 1,
 
-    /**
-       This completion is the whole argument, not just the remainder. This
-       flag must never be set on completions returned from the complete()
-       function. It is strictly for internal use in the completion code.
-    */
-    COMPLETE_WHOLE_ARGUMENT = 1 << 2,
-
     /**
        This completion may or may not want a space at the end - guess by
        checking the last character of the completion.
     */
-    COMPLETE_AUTO_SPACE = 1 << 3,
+    COMPLETE_AUTO_SPACE = 1 << 2,
 
     /**
        This completion should be inserted as-is, without escaping.
     */
-    COMPLETE_DONT_ESCAPE = 1 << 4
+    COMPLETE_DONT_ESCAPE = 1 << 3
 };
 typedef int complete_flags_t;
 

fish_tests.cpp

@@ -955,6 +955,57 @@ static void test_colors()
     assert(rgb_color_t(L"mooganta").is_none());
 }
 
+static void test_1_completion(wcstring line, const wcstring &completion, complete_flags_t flags, bool append_only, wcstring expected, long source_line)
+{
+    // str is given with a caret, which we use to represent the cursor position
+    // find it
+    const size_t in_cursor_pos = line.find(L'^');
+    assert(in_cursor_pos != wcstring::npos);
+    line.erase(in_cursor_pos, 1);
+    
+    const size_t out_cursor_pos = expected.find(L'^');
+    assert(out_cursor_pos != wcstring::npos);
+    expected.erase(out_cursor_pos, 1);
+    
+    size_t cursor_pos = in_cursor_pos;
+    wcstring result = completion_apply_to_command_line(completion, flags, line, &cursor_pos, append_only);
+    if (result != expected)
+    {
+        fprintf(stderr, "line %ld: %ls + %ls -> [%ls], expected [%ls]\n", source_line, line.c_str(), completion.c_str(), result.c_str(), expected.c_str());
+    }
+    assert(result == expected);
+    assert(cursor_pos == out_cursor_pos);
+}
+
+static void test_completions()
+{
+    #define TEST_1_COMPLETION(a, b, c, d, e) test_1_completion(a, b, c, d, e, __LINE__)
+    say(L"Testing completions");
+    TEST_1_COMPLETION(L"foo^", L"bar", 0, false, L"foobar ^");
+    TEST_1_COMPLETION(L"foo^ baz", L"bar", 0, false, L"foobar ^ baz"); //we really do want to insert two spaces here - otherwise it's hidden by the cursor
+    TEST_1_COMPLETION(L"'foo^", L"bar", 0, false, L"'foobar' ^");
+    TEST_1_COMPLETION(L"'foo'^", L"bar", 0, false, L"'foobar' ^");
+    TEST_1_COMPLETION(L"'foo\\'^", L"bar", 0, false, L"'foo\\'bar' ^");
+    TEST_1_COMPLETION(L"foo\\'^", L"bar", 0, false, L"foo\\'bar ^");
+    
+    // Test append only
+    TEST_1_COMPLETION(L"foo^", L"bar", 0, true, L"foobar ^");
+    TEST_1_COMPLETION(L"foo^ baz", L"bar", 0, true, L"foobar ^ baz");
+    TEST_1_COMPLETION(L"'foo^", L"bar", 0, true, L"'foobar' ^");
+    TEST_1_COMPLETION(L"'foo'^", L"bar", 0, true, L"'foo'bar ^");
+    TEST_1_COMPLETION(L"'foo\\'^", L"bar", 0, true, L"'foo\\'bar' ^");
+    TEST_1_COMPLETION(L"foo\\'^", L"bar", 0, true, L"foo\\'bar ^");
+    
+    TEST_1_COMPLETION(L"foo^", L"bar", COMPLETE_NO_SPACE, false, L"foobar^");
+    TEST_1_COMPLETION(L"'foo^", L"bar", COMPLETE_NO_SPACE, false, L"'foobar^");
+    TEST_1_COMPLETION(L"'foo'^", L"bar", COMPLETE_NO_SPACE, false, L"'foobar'^");
+    TEST_1_COMPLETION(L"'foo\\'^", L"bar", COMPLETE_NO_SPACE, false, L"'foo\\'bar^");
+    TEST_1_COMPLETION(L"foo\\'^", L"bar", COMPLETE_NO_SPACE, false, L"foo\\'bar^");
+
+    TEST_1_COMPLETION(L"foo^", L"bar", COMPLETE_NO_CASE, false, L"bar ^");
+    TEST_1_COMPLETION(L"'foo^", L"bar", COMPLETE_NO_CASE, false, L"bar ^");
+}
+
 static void perform_one_autosuggestion_test(const wcstring &command, const wcstring &wd, const wcstring &expected, long line)
 {
     wcstring suggestion;
@@ -1655,6 +1706,7 @@ int main(int argc, char **argv)
     test_word_motion();
     test_is_potential_path();
     test_colors();
+    test_completions();
     test_autosuggestion_combining();
     test_autosuggest_suggest_special();
     history_tests_t::test_history();

history.cpp

@@ -1658,6 +1658,9 @@ void history_t::add_with_file_detection(const wcstring &str)
     ASSERT_IS_MAIN_THREAD();
     path_list_t potential_paths;
     
+    /* Hack hack hack - if the command is likely to trigger an exit, then don't do background file detection, because we won't be able to write it to our history file before we exit. */
+    bool impending_exit = false;
+
     tokenizer_t tokenizer(str.c_str(), TOK_SQUASH_ERRORS);
     for (; tok_has_next(&tokenizer); tok_next(&tokenizer))
     {
@@ -1671,12 +1674,19 @@ void history_t::add_with_file_detection(const wcstring &str)
                 if (unescape_string(potential_path, false) && string_could_be_path(potential_path))
                 {
                     potential_paths.push_back(potential_path);
+                    
+                    /* What a hack! */
+                    impending_exit = impending_exit || contains(potential_path, L"exec", L"exit", L"reboot");
                 }
             }
         }
     }
     
-    if (! potential_paths.empty())
+    if (potential_paths.empty() || impending_exit)
+    {
+        this->add(str);
+    }
+    else
     {
         /* We have some paths. Make a context. */
         file_detection_context_t *context = new file_detection_context_t(this, str);

reader.cpp

@@ -380,6 +380,9 @@ static int interrupted=0;
   Prototypes for a bunch of functions defined later on.
 */
 
+static bool is_backslashed(const wcstring &str, size_t pos);
+static wchar_t unescaped_quote(const wcstring &str, size_t pos);
+
 /**
    Stores the previous termios mode so we can reset the modes when
    we execute programs and when the shell exits.
@@ -626,9 +629,10 @@ void reader_data_t::command_line_changed()
 }
 
 
-/** Remove any duplicate completions in the list. This relies on the list first being sorted. */
+/** Sorts and remove any duplicate completions in the list. */
-static void remove_duplicates(std::vector<completion_t> &l)
+static void sort_and_make_unique(std::vector<completion_t> &l)
 {
+    sort(l.begin(), l.end());
     l.erase(std::unique(l.begin(), l.end()), l.end());
 }
 
@@ -937,21 +941,21 @@ static size_t comp_ilen(const wchar_t *a, const wchar_t *b)
    \param flags A union of all flags describing the completion to insert. See the completion_t struct for more information on possible values.
    \param command_line The command line into which we will insert
    \param inout_cursor_pos On input, the location of the cursor within the command line. On output, the new desired position.
+   \param append_only Whether we can only append to the command line, or also modify previous characters. This is used to determine whether we go inside a trailing quote.
    \return The completed string
 */
-static wcstring completion_apply_to_command_line(const wcstring &val_str, int flags, const wcstring &command_line, size_t *inout_cursor_pos)
+wcstring completion_apply_to_command_line(const wcstring &val_str, complete_flags_t flags, const wcstring &command_line, size_t *inout_cursor_pos, bool append_only)
 {
     const wchar_t *val = val_str.c_str();
     bool add_space = !(flags & COMPLETE_NO_SPACE);
     bool do_replace = !!(flags & COMPLETE_NO_CASE);
     bool do_escape = !(flags & COMPLETE_DONT_ESCAPE);
-    const size_t cursor_pos = *inout_cursor_pos;
     
-    //	debug( 0, L"Insert completion %ls with flags %d", val, flags);
+    const size_t cursor_pos = *inout_cursor_pos;
+    bool back_into_trailing_quote = false;
 
     if (do_replace)
     {
-
         size_t move_cursor;
         const wchar_t *begin, *end;
         wchar_t *escaped;
@@ -994,6 +998,19 @@ static wcstring completion_apply_to_command_line(const wcstring &val_str, int fl
         if (do_escape)
         {
             parse_util_get_parameter_info(command_line, cursor_pos, &quote, NULL, NULL);
+            
+            /* If the token is reported as unquoted, but ends with a (unescaped) quote, and we can modify the command line, then delete the trailing quote so that we can insert within the quotes instead of after them. See https://github.com/fish-shell/fish-shell/issues/552 */
+            if (quote == L'\0' && ! append_only && cursor_pos > 0)
+            {
+                /* The entire token is reported as unquoted...see if the last character is an unescaped quote */
+                wchar_t trailing_quote = unescaped_quote(command_line, cursor_pos - 1);
+                if (trailing_quote != L'\0')
+                {
+                    quote = trailing_quote;
+                    back_into_trailing_quote = true;
+                }
+            }
+            
             replaced = parse_util_escape_string_with_quote(val_str, quote);
         }
         else
@@ -1001,12 +1018,21 @@ static wcstring completion_apply_to_command_line(const wcstring &val_str, int fl
             replaced = val;
         }
         
+        size_t insertion_point = cursor_pos;
+        if (back_into_trailing_quote)
+        {
+            /* Move the character back one so we enter the terminal quote */
+            assert(insertion_point > 0);
+            insertion_point--;
+        }
+        
+        /* Perform the insertion and compute the new location */
         wcstring result = command_line;
-        result.insert(cursor_pos, replaced);
+        result.insert(insertion_point, replaced);
-        size_t new_cursor_pos = cursor_pos + replaced.size();
+        size_t new_cursor_pos = insertion_point + replaced.size() + (back_into_trailing_quote ? 1 : 0);
         if (add_space)
         {
-            if (quote && (command_line.c_str()[cursor_pos] != quote))
+            if (quote != L'\0' && unescaped_quote(command_line, insertion_point) != quote)
             {
                 /* This is a quoted parameter, first print a quote */
                 result.insert(new_cursor_pos++, wcstring(&quote, 1));
@@ -1027,10 +1053,10 @@ static wcstring completion_apply_to_command_line(const wcstring &val_str, int fl
    \param flags A union of all flags describing the completion to insert. See the completion_t struct for more information on possible values.
 
 */
-static void completion_insert(const wchar_t *val, int flags)
+static void completion_insert(const wchar_t *val, complete_flags_t flags)
 {
     size_t cursor = data->buff_pos;
-    wcstring new_command_line = completion_apply_to_command_line(val, flags, data->command_line, &cursor);
+    wcstring new_command_line = completion_apply_to_command_line(val, flags, data->command_line, &cursor, false /* not append only */);
     reader_set_buffer(new_command_line, cursor);
 
     /* Since we just inserted a completion, don't immediately do a new autosuggestion */
@@ -1259,7 +1285,7 @@ struct autosuggestion_context_t
         {
             const completion_t &comp = completions.at(0);
             size_t cursor = this->cursor_pos;
-            this->autosuggestion = completion_apply_to_command_line(comp.completion.c_str(), comp.flags, this->search_string, &cursor);
+            this->autosuggestion = completion_apply_to_command_line(comp.completion.c_str(), comp.flags, this->search_string, &cursor, true /* append only */);
             return 1;
         }
 
@@ -2706,23 +2732,40 @@ static int wchar_private(wchar_t c)
 
 /**
    Test if the specified character in the specified string is
-   backslashed.
+   backslashed. pos may be at the end of the string, which indicates
+   if there is a trailing backslash.
 */
-static bool is_backslashed(const wchar_t *str, size_t pos)
+static bool is_backslashed(const wcstring &str, size_t pos)
 {
-    size_t count = 0;
+    /* note pos == str.size() is OK */
-    size_t idx = pos;
+    if (pos > str.size())
+        return false;
+    
+    size_t count = 0, idx = pos;
     while (idx--)
     {
-        if (str[idx] != L'\\')
+        if (str.at(idx) != L'\\')
             break;
-
         count++;
     }
 
     return (count % 2) == 1;
 }
 
+static wchar_t unescaped_quote(const wcstring &str, size_t pos)
+{
+    wchar_t result = L'\0';
+    if (pos < str.size())
+    {
+        wchar_t c = str.at(pos);
+        if ((c == L'\'' || c == L'"') && ! is_backslashed(str, pos))
+        {
+            result = c;
+        }
+    }
+    return result;
+}
+
 
 const wchar_t *reader_readline()
 {
@@ -2913,7 +2956,7 @@ const wchar_t *reader_readline()
                     if (next_comp != NULL)
                     {
                         size_t cursor_pos = cycle_cursor_pos;
-                        const wcstring new_cmd_line = completion_apply_to_command_line(next_comp->completion, next_comp->flags, cycle_command_line, &cursor_pos);
+                        const wcstring new_cmd_line = completion_apply_to_command_line(next_comp->completion, next_comp->flags, cycle_command_line, &cursor_pos, false);
                         reader_set_buffer(new_cmd_line, cursor_pos);
 
                         /* Since we just inserted a completion, don't immediately do a new autosuggestion */
@@ -2923,35 +2966,42 @@ const wchar_t *reader_readline()
                 else
                 {
                     /* Either the user hit tab only once, or we had no visible completion list. */
-                    const wchar_t *begin, *end;
+                    const wchar_t *cmdsub_begin, *cmdsub_end;
                     const wchar_t *token_begin, *token_end;
-                    const wchar_t *buff = data->command_line.c_str();
+                    const wchar_t * const buff = data->command_line.c_str();
-                    long cursor_steps;
 
                     /* Clear the completion list */
                     comp.clear();
                     
-                    parse_util_cmdsubst_extent(buff, data->buff_pos, &begin, &end);
+                    /* Figure out the extent of the command substitution surrounding the cursor. This is because we only look at the current command substitution to form completions - stuff happening outside of it is not interesting. */
+                    parse_util_cmdsubst_extent(buff, data->buff_pos, &cmdsub_begin, &cmdsub_end);
                     
-                    parse_util_token_extent(begin, data->buff_pos - (begin-buff), &token_begin, &token_end, 0, 0);
+                    /* Figure out the extent of the token within the command substitution. Note we pass cmdsub_begin here, not buff */
+                    parse_util_token_extent(cmdsub_begin, data->buff_pos - (cmdsub_begin-buff), &token_begin, &token_end, 0, 0);
                     
-                    cursor_steps = token_end - buff- data->buff_pos;
+                    /* Figure out how many steps to get from the current position to the end of the current token. */
-                    data->buff_pos += cursor_steps;
+                    size_t end_of_token_offset = token_end - buff;
-                    if (is_backslashed(buff, data->buff_pos))
+
+                    /* Move the cursor to the end */
+                    if (data->buff_pos != end_of_token_offset)
+                    {
+                        data->buff_pos = end_of_token_offset;
+                        reader_repaint();
+                    }
+                    
+                    /* Remove a trailing backslash. This may trigger an extra repaint, but this is rare. */
+                    if (is_backslashed(data->command_line, data->buff_pos))
                     {
                         remove_backward();
                     }
                     
-                    reader_repaint();
+                    /* Construct a copy of the string from the beginning of the command substitution up to the end of the token we're completing */
-
+                    const wcstring buffcpy = wcstring(cmdsub_begin, token_end);
-                    size_t len = data->buff_pos - (begin-buff);
-                    const wcstring buffcpy = wcstring(begin, len);
 
                     data->complete_func(buffcpy, comp, COMPLETE_DEFAULT, NULL);
 
                     /* Munge our completions */
-                    sort(comp.begin(), comp.end());
+                    sort_and_make_unique(comp);
-                    remove_duplicates(comp);
                     prioritize_completions(comp);
 
                     /* Record our cycle_command_line */
@@ -3130,7 +3180,7 @@ const wchar_t *reader_readline()
                 /*
                  Allow backslash-escaped newlines
                  */
-                if (is_backslashed(data->command_line.c_str(), data->buff_pos))
+                if (is_backslashed(data->command_line, data->buff_pos))
                 {
                     insert_char('\n');
                     break;

reader.h

@@ -217,5 +217,8 @@ int reader_search_mode();
 /* Given a command line and an autosuggestion, return the string that gets shown to the user. Exposed for testing purposes only. */
 wcstring combine_command_and_autosuggestion(const wcstring &cmdline, const wcstring &autosuggestion);
 
+/* Apply a completion string. Exposed for testing only. */
+wcstring completion_apply_to_command_line(const wcstring &val_str, complete_flags_t flags, const wcstring &command_line, size_t *inout_cursor_pos, bool append_only);
+
 
 #endif