git completion broken when using redirection operator #1296

Closed
Jean-Daniel opened this Issue Feb 10, 2014 · 6 comments

Projects

None yet

4 participants

@Jean-Daniel

While using the git command, when I try to redirect the output to a file and try to use tab completion for the file path, instead of interpreting it as a file path, the completion machinery try to substitute git internal paths:

me@mymac ~/P/b/l/lldb> git diff > ~/De
~/Demaster             (Branch)  ~/Demirror/release_33  (Branch)  ~/Deorigin/lldbsrv  (Branch)  ~/Deorigin/platform    (Branch)  ~/Deorigin/release_33  (Branch)
~/Demirror/master      (Branch)  ~/Demirror/release_34  (Branch)  ~/Deorigin/master   (Branch)  ~/Deorigin/plugins     (Branch)  ~/Deorigin/release_34  (Branch)
~/Demirror/release_32  (Branch)  ~/Denative             (Branch)  ~/Deorigin/native   (Branch)  ~/Deorigin/release_32  (Branch)  ~/Desktop/                     

In the previous example, I expect to redirect the output in a file on /Desktop, but instead of completing '/De' in Desktop, fish shell propose a bunch of broken suggestions based on the prefix I want to complete, and the current repository branches name.

@ridiculousfish ridiculousfish added this to the next-major milestone Feb 10, 2014
@wwwjfy
wwwjfy commented Apr 27, 2014

I've met the same problem, and spent some time spotting the problem.
By bisecting, I found it's an issue from ce857b0
I found that in function complete in complete.cpp, all_arguments doesn't include one after the redirection sign (< or >), causing current argument is empty, instead of ~/De in the above example.

It's annoying to redirect a file as input or output, so it'll be great to have it fixed shortly. :)

@wwwjfy
wwwjfy commented May 2, 2014

I did some changes on ce857b0 to try to fix it, which seems to work. But when it comes to the latest master, something is wrong. Obviously, something else changes on this topic. Hope this helps. :)

It simply makes the target after the redirection token also a symbol_argument.

diff --git a/highlight.cpp b/highlight.cpp
index 3acaf49..45e1381 100644
--- a/highlight.cpp
+++ b/highlight.cpp
@@ -1856,7 +1856,7 @@ void highlighter_t::color_redirection(const parse_node_t &redirection_node)
         return;

     const parse_node_t *redirection_primitive = this->parse_tree.get_child(redirection_node, 0, parse_token_type_redirection); //like 2>
-    const parse_node_t *redirection_target = this->parse_tree.get_child(redirection_node, 1, parse_token_type_string); //like &1 or file path
+    const parse_node_t *redirection_target = this->parse_tree.get_child(redirection_node, 1, symbol_argument); //like &1 or file path

     if (redirection_primitive != NULL)
     {
diff --git a/parse_productions.cpp b/parse_productions.cpp
index 2279554..40c822d 100644
--- a/parse_productions.cpp
+++ b/parse_productions.cpp
@@ -421,7 +421,7 @@ RESOLVE_ONLY(argument)

 PRODUCTIONS(redirection) =
 {
-    {parse_token_type_redirection, parse_token_type_string}
+    {parse_token_type_redirection, symbol_argument}
 };
 RESOLVE_ONLY(redirection)

diff --git a/parse_tree.cpp b/parse_tree.cpp
index 97421da..5b44caf 100644
--- a/parse_tree.cpp
+++ b/parse_tree.cpp
@@ -1097,7 +1097,7 @@ enum token_type parse_node_tree_t::type_for_redirection(const parse_node_t &redi
     assert(redirection_node.type == symbol_redirection);
     enum token_type result = TOK_NONE;
     const parse_node_t *redirection_primitive = this->get_child(redirection_node, 0, parse_token_type_redirection); //like 2>
-    const parse_node_t *redirection_target = this->get_child(redirection_node, 1, parse_token_type_string); //like &1 or file path
+    const parse_node_t *redirection_target = this->get_child(redirection_node, 1, symbol_argument); //like &1 or file path

     if (redirection_primitive != NULL && redirection_primitive->has_source())
     {
@ridiculousfish
Member

Good thinking, but the redirection target should not be an argument (e.g. it should not get syntax highlighted as an argument). I'll try to tackle this tonight.

@ridiculousfish
Member

Should be fixed by 16b9829 . Thanks for reporting this.

@wwwjfy
wwwjfy commented May 3, 2014

Thanks for the prompt fix.
I propose a tweak:

diff --git a/complete.cpp b/complete.cpp
index 101faee..cf915e7 100644
--- a/complete.cpp
+++ b/complete.cpp
@@ -1981,7 +1981,7 @@ void complete(const wcstring &cmd_with_subcmds, std::vector<completion_t> &comps
                 bool in_redirection = false;
                 if (matching_arg_index == (size_t)(-1))
                 {
-                    const parse_node_t *redirection = tree.find_node_matching_source_location(symbol_redirection, pos, plain_statement);
+                    const parse_node_t *redirection = tree.find_node_matching_source_location(symbol_redirection, adjusted_pos, plain_statement);
                     in_redirection = (redirection != NULL);
                 }

The adjusted_pos is to ignore the trailing spaces, as pos being used, any trailing spaces cause in_redirection false while it should be true.

@ridiculousfish
Member

Quite right, good catch. Fixed as e530af1

@zanchey zanchey modified the milestone: next-minor, next-major Sep 27, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment