Path with space completion fix for REPL #8838

dhoegh · 2014-10-28T17:20:12Z

This fixes completion for paths with spaces in both shell mode and julia mode ex:

shell>cd C:\\Program\ Files\ <tab>
shell>cd C:\\Program\ Files\ (x86)\\
julia>cd("C:\\Program\ Files\ <tab>
julia>cd ("C:\\Program\ Files\ (x86)\\

The main changes is in the completions that is used when completing in julia mode. The changes to get it working in shell mode is only 5 lines. If any of you have time to look at it @tkelman, @lucasb-eyer it would be greatly appreciated.

@StefanKarpinski could you approve the changes I made in the shell_parse? The main reason for the change is when a string is parsed to the shell_parse function like: "C:\\Progams\\ " this would throw an error before due to the ending space is removed and then the string has a dangling \. To prevent it from removing the space I use the keyword argument like: Base.shell_parse(string[r], true, strip_r=!endswith(string[r], "\\ ") and then it do not trim the space away when the string ends on "\\ ".

StefanKarpinski · 2014-10-29T17:23:51Z

base/string.jl

-function shell_parse(raw::String, interp::Bool)
-    s = strip(raw)
+function shell_parse(raw::String, interp::Bool; strip_r::Bool=true)
+    s = strip_r ? strip(raw) : lstrip(raw)


Why wouldn't you just call the strip function on the argument before calling this function?

I just need the function not to strip the last space away when "\\ " occurs. I just need to disable the strip it do on the raw variable. I could do the following and then just strip the input string before it is sent to the function.

function shell_parse(raw::String, interp::Bool; should_strip::Bool=true) s = should_strip ? strip(raw) : raw

And then the keyword argument is just a bool that controls whether the string should be stripped.

Again, that's a pointless option: if you want to strip the argument before doing anything else, then strip it before passing it to the function. You can write the call site like this:

Base.shell_parse(endswith(scs, "\\ ") ? strip(scs) : scs, true)

The point is that today it look like:

function shell_parse(raw::String, interp::Bool) s = strip(raw)

This means that the string is default stripped in both ends, but I would like to keep the last space in when the string endswith(scs, "\\ ") scs="C:\\Progam\\ " and today I cannot force it not to strip the last space away in the string.

Any transformation of the string to be parsed to be done before passing it to the parsing function can be done without complicating the signature of the parsing function. Please do it that way.

I'm slightly suspicious that shell parse is involved in parsing this in the first place, but that's a different story.

Yes exactly, I will change it to you suggestion @StefanKarpinski.

Should the stripping respect quoting with \? It seems like it should. Maybe that's the only problem here.

Because that's how the shell parses things: if strips leading and trailing whitespace and then splits on spaces (modulo quoting).

not quite, the shell has to be able to gracefully handle files that end in spaces. I can make a file "a ", and a file "a", and bash can tell the difference:

~/Documents/no-backup/julia$ cat a I'm a ~/Documents/no-backup/julia$ cat a\ I'm a_space

dropping the trailing spaces can even lead to bugs: http://www.exploit-monday.com/2013/02/WindowsFileConfusion.html

In that case, the correct fix here does seem to be to make sure that quoted spaces are respected by shell_parse.

dhoegh · 2014-11-13T20:14:09Z

I have now fixed the shell_parse function to respect the space when a string endwith "\\ ". Is this what you expected? @StefanKarpinski, @JeffBezanson. Should it be merged in a separate pull request and then the rest of the details about completion could be discussed here.

~~In the updated version the pull request can now also auto complete "$(ab<tab>"~~

StefanKarpinski · 2014-11-14T18:50:21Z

Still not quite what I had in mind. I'll take a crack at it later.

…es\\

dhoegh · 2014-11-23T15:49:44Z

I have rebased and simplified the code and made some test cases. @StefanKarpinski did you get time for making the shell_parse to behave as wanted? What I suggest now is that it can be merged without the change in the shell_parse function, it will just not have any suggestions when there is a space in the end of the path in shell mode, but it will work in line 2.

shell>cd C:\\Program\ Files\ <tab>
shell>cd C:\\Program\ Files\ (x8<tab>
shell>cd C:\\Program\ Files\ (x86)\\

For completion in strings it works perfectly. If it gets merged without a version of the shell_parse function that respects the "\\ " then I will file an issue to track the problem.
@tkelman, @lucasb-eyer it would be greatly appriciated if you had time to test it.

StefanKarpinski · 2014-11-23T16:01:30Z

Sorry, I haven't gotten around to this yet. However, the correct way to fix this issue is to get rid of the whitespace stripping I originally put in and instead build whitespace stripping into the shell_parse internal logic so that it respects backslash escaping. That is correct on all platforms, not just Windows. Looking at your current version of this patch, this may already be what you're doing. Is that correct?

dhoegh · 2014-11-23T16:09:29Z

Yes it strips whitespaces from the front and from the end it strips the whitespaces but it respects the backslash escaped spaces.

StefanKarpinski · 2014-11-23T16:24:05Z

Ah, ok, carry on then. I approve of this approach. Sorry for snoozing on this so long – I swear last I looked it didn't work this way!

StefanKarpinski · 2014-11-23T16:24:50Z

@tkelman, can you sanity check this and merge if it seems reasonable?

tkelman · 2014-11-23T16:48:04Z

I'll add it to my list of things to test after 0.3.3.

dhoegh · 2014-11-23T16:52:41Z

Awesome good work at 0.3.3 👍

tkelman · 2014-11-23T19:43:58Z

@dhoegh this works pretty nicely, I like it. Thanks for your continued improvements here!

Path with space completion fix for REPL

dhoegh · 2014-11-23T19:46:59Z

@tkelman you are very fast:+1:, should it be backported to 0.3.4?

tkelman · 2014-11-23T19:48:27Z

@dhoegh possibly but not right away, let's wait for people to run with it on master for a few days at least.

tkelman · 2014-11-25T00:44:13Z

Whoops, I merged this too soon, no backporting until the bug mentioned here #9137 (comment) is fixed

Fix error regarding unescape_string introduced by #8838

tkelman · 2014-11-25T18:41:21Z

This should be squashed along with #9143 and #9137 when backporting

edit: backport is at #9164

Backport of #8838 and #9143 squashed

dhoegh · 2014-12-01T13:47:39Z

base/REPLCompletions.jl

@@ -187,12 +187,13 @@ function completions(string, pos)
    partial = string[1:pos]
    inc_tag = Base.incomplete_tag(parse(partial , raise=false))
    if inc_tag in [:cmd, :string]
-        startpos = nextind(partial, rsearch(partial, non_filename_chars, pos))
+        m = match(r"[\t\n\r\"'`@\$><=;|&\{]| (?!\\)",reverse(partial))
+        startpos = length(partial)-(m == nothing ? 1 : m.offset) + 2


@ivarne, @tkelman I think I have made the same mistanke here it should properly be:
startpos = nextind(partial, nextind(partial, length(partial) - (m == nothing ? 1 : m.offset)))
It does not crash the REPL but it can't complete on "ß C:\\.
Sorry about the mess I made now I understand the difficulties in indexing in strings.

It doesn't crash when it should, because of #7811.

Since m.offset is guaranteed to be the offset of an ASCII character (that's what you were searching for), and ASCII characters are always represented by a single code unit in UTF-xx, isn't ~~length(partial) - (m == nothing ? 1 : m.offset) + 2~~ ~~endof(s) - start + 1~~ nextind(s,endof(s)) - start okay here?

dhoegh changed the title ~~Space completion~~ Path with space completion fix for REPL Oct 28, 2014

StefanKarpinski reviewed Oct 29, 2014
View reviewed changes

MikeInnes force-pushed the master branch from 5c60996 to b1c3df3 Compare November 14, 2014 17:07

dhoegh force-pushed the space_completion branch 9 times, most recently from f333eec to 1289276 Compare November 23, 2014 13:49

Fixes space completion in paths in the REPL like: cd C:\\Program\ Fil…

adca441

…es\\

dhoegh force-pushed the space_completion branch from 1289276 to adca441 Compare November 23, 2014 13:59

tkelman added a commit that referenced this pull request Nov 23, 2014

Merge pull request #8838 from dhoegh/space_completion

229f4aa

Path with space completion fix for REPL

tkelman merged commit 229f4aa into JuliaLang:master Nov 23, 2014

tkelman added backport pending and removed backport pending labels Nov 23, 2014

tkelman added a commit that referenced this pull request Nov 25, 2014

Merge pull request #9143 from dhoegh/fix_shell_completion_error

dcd9ac2

Fix error regarding unescape_string introduced by #8838

tkelman added backport pending building Build system, or building Julia or its dependencies and removed building Build system, or building Julia or its dependencies labels Nov 25, 2014

tkelman removed the backport pending label Nov 27, 2014

JeffBezanson added a commit that referenced this pull request Nov 30, 2014

Merge pull request #9164 from dhoegh/Backport_space_in_path_completion

52a8049

Backport of #8838 and #9143 squashed

dhoegh reviewed Dec 1, 2014
View reviewed changes

dhoegh mentioned this pull request Dec 2, 2014

Fix path completion when UTF8 char is occurring before match characters. #9227

Merged

dhoegh mentioned this pull request Dec 10, 2014

Path containing spaces cause tab-completion issues on windows #9302

Closed

dhoegh mentioned this pull request Jan 31, 2015

Improve windows shell mode; fix edit() function for windows #7108

Closed

dhoegh deleted the space_completion branch February 3, 2015 14:55

dhoegh mentioned this pull request Feb 26, 2015

fix #10324 #10336

Merged

ssfrr mentioned this pull request Oct 1, 2019

Autocomplete escape sequence bug #29304

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Path with space completion fix for REPL #8838

Path with space completion fix for REPL #8838

dhoegh commented Oct 28, 2014

StefanKarpinski Oct 29, 2014

dhoegh Oct 29, 2014

StefanKarpinski Oct 29, 2014

dhoegh Oct 29, 2014

StefanKarpinski Oct 29, 2014

StefanKarpinski Oct 29, 2014

dhoegh Oct 29, 2014

JeffBezanson Oct 29, 2014

vtjnash Nov 5, 2014

StefanKarpinski Nov 5, 2014

dhoegh commented Nov 13, 2014

StefanKarpinski commented Nov 14, 2014

dhoegh commented Nov 23, 2014

StefanKarpinski commented Nov 23, 2014

dhoegh commented Nov 23, 2014

StefanKarpinski commented Nov 23, 2014

StefanKarpinski commented Nov 23, 2014

tkelman commented Nov 23, 2014

dhoegh commented Nov 23, 2014

tkelman commented Nov 23, 2014

dhoegh commented Nov 23, 2014

tkelman commented Nov 23, 2014

tkelman commented Nov 25, 2014

tkelman commented Nov 25, 2014

dhoegh Dec 1, 2014

ivarne Dec 1, 2014

stevengj Dec 3, 2014

Path with space completion fix for REPL #8838

Path with space completion fix for REPL #8838

Conversation

dhoegh commented Oct 28, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dhoegh commented Nov 13, 2014

StefanKarpinski commented Nov 14, 2014

dhoegh commented Nov 23, 2014

StefanKarpinski commented Nov 23, 2014

dhoegh commented Nov 23, 2014

StefanKarpinski commented Nov 23, 2014

StefanKarpinski commented Nov 23, 2014

tkelman commented Nov 23, 2014

dhoegh commented Nov 23, 2014

tkelman commented Nov 23, 2014

dhoegh commented Nov 23, 2014

tkelman commented Nov 23, 2014

tkelman commented Nov 25, 2014

tkelman commented Nov 25, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment