Fixes #8483 Windows completion error

[dhoegh]

This should fix #8483 I am not a Julia power user yet so could some of you guy's check it's working because I have not compiled base with it, @lucasb-eyer, @tkelman?

[tkelman]

This seems to work fairly well in shell mode, as long as I use forward slashes for everything. If I use double-backslashes at all, the completions do the wrong thing: shell> cd C:\\<tab><tab> is listing the contents of my D drive, not C. That is probably a separate bug though, it looks like it's been present for a while.

And in julia> mode I'm getting different behavior for completions of julia> cd("C:\\Use<tab><tab> (which does not list anything, it's trying to complete under D again and I don't have a D:\Users), vs julia> cd("C:/Use<tab><tab> (which completes to cd("C:/Users\ with a trailing backslash, if I double the backslash then I start getting completions as if I'm at the base of D: again...)

Since you've been looking into this section of code, I'd quite appreciate attempts to make this behave more consistently. Would also be good to add more tests for Windows behavior in test/replcompletions.jl. See #8685 (comment) for how to rebuild the system image and run the tests from a binary installation of Julia.

Also it's a good idea in the future to make proposed changes in a new branch for each new PR, so you can have multiple independent PR's open at the same time, and track changes to master if necessary.

[lucasb-eyer]

Unfortunately, I can't test julia on windows for now. Maybe we should just replace joinpath here by manual joining/appending with a forward-slash? It sounds like backslash is a world of problems for windows users. Of course, the better alternative would be to fix that weird behavior with backslashes, but I can't help with that.

[dhoegh]

Thank you for the tips @tkelman.
The behavior with julia> cd("C:\\Use<tab><tab> don't give me anything either, but julia> cd("C:\\ completes in the C:\Users\Hoegh\Julia-0.4.0-dev or the equivalent when I do it in 0.3 which is not correct.

This example don't work in 0.4 before my patch shell>cd ..\\Documents and Settings\\ autocompletes with what I have in my C:\\ so that. So the issue with inserting/using front on windows as Julia did in 0.3 is actual just a circumvention of the real problem with the auto completion is broken when using backslashes and that it autocompletes with a single backslash.

Do you guy's have any idea of what is going on with broke \\ autocompletion and where it could go wrong?

[tkelman]

Unfortunately I don't, by now I'm sure you know more about the REPL code than I do (don't think I've ever touched those files or studied them in detail). There might be some heuristic for completion somewhere for when something is "path-like," but I wouldn't know exactly where to find it. It sounds like there's a fair bit of conflation somewhere in the code between the meaning of / on Unix vs the top-level contents of the drive you are currently located in on Windows, and a failure to take into account the "current drive" state, in addition to problems recognizing double-backslash as a directory separator in completions.

[dhoegh]

I have been working a bit more and it seems that the reason to the weird behavior of \\ is that when Base.shell_parse("cd ..\Do"[1:9], true)
It returns "\Do" which is sent to
´Base.REPLCompletions.complete_path("\Do",4)´ which off cause not return the wanted path. So to get rid of the weird behavior of \\ is within Base.shell_parse function.

@tkelman it could be nice the precompilation trick to be documented somewhere to make it easier for windows people to get started developing. Can you just delete sys.ji to run without precompilation?

[tkelman]

That sounds promising!

it could be nice the precompilation trick to be documented somewhere to make it easier for windows people to get started developing

Yeah this is true, I should probably copy-paste various instructions I've written down on the google group or issue comments and put them into CONTRIBUTING.md or somewhere else in the docs. There's a WIP PR for adding a Julia script to do the same thing but across other platforms too, but there the linker gets involved to produce the fast-startup shared library version of the system image so it's more complicated.

Can you just delete sys.ji to run without precompilation?

Not as far as I know, I'm not sure whether a JIT-everything, precompile-nothing mode would be practical or possible.

[pao]

Not as far as I know, I'm not sure whether a JIT-everything, precompile-nothing mode would be practical or possible.

It would be painfully slow to start up, since you'd have to run through everything necessary to build sys.ji every startup, including all the bootstrapping. Easier to rebuild the system image on demand.

[dhoegh]

Hi I have been working a bit more on solving the problem. It now autocompletes with \\ and I have fixed the issue with shell>cd ..\\Documents\\ to give the correct answer.
One problem that need to be solved:
-ls don't work with double backslash any help? It does not work going up in folders as:
ls ..\\Hoegh\\Documents\\ could you help? @tkelman
A minor problem is it cannot auto complete with folders containing spaces:
cd ../My Documents/
However, it cannot do this today either.

[lucasb-eyer]

The space-in-path problem is also present on linux and is worth opening a separate issue imo. The auto-complete probably should either escape the space or surround the path with double-quotes. But the auto-complete also fails to complete correctly-escaped paths like ls /home/beyer/folder\ with\ spa<tab>.

As for your patch, you should probably add (windows-only) tests to test/replcompletions.jl.

I don't have any idea about the ls ../<tab> except that I can say "works on linux", sorry.

[dhoegh]

To my knowledge the reason why it does not complete correctly is in the Base.shell_parse function but i do not have a solution to it:(

[tkelman]

So on windows ls ..\\Hoegh\\Documents\\ actually calls out to the MSYS version of ls that's included as part of command-line git. I don't think MSYS ls works all that well with double-backslashes, but there's not much we can do about that.

Eventually we'll be getting rid of that command-line git and replacing it with libgit2 for the package manager. What to do about common utilities like ls, grep, etc is still probably TBD. It looks like if we switch to busybox-win32, its version of ls can handle double-backslashes in a smarter way.

[dhoegh]

Can we then close this? It should be backported to 0.3.

[tkelman]

It's a problem in both master and release-0.3 I believe. Are you saying this is merge-able in your opinion, or you'd like to do it differently?

[dhoegh]

Yes, it is. I do not think I can do it better right now and it is in a better condition than it is in 0.3-0.4.

[tkelman]

I'll do some testing of these changes and let you know how I like it, and let others review it as well. The Travis failures on Mac are unrelated, they're due to @staticfloat bumping the openblas formula to v0.2.12, but he hasn't built a bottle (binary) yet.

Edit: and I think the Arpack and SuiteSparse Homebrew bottles might now need to be rebuilt to account for the different openblas library version?

[tkelman]

@dhoegh ok shell mode completion is now working quite well, except for paths with spaces (that can be a separate issue as @lucasb-eyer said). But julia> cd("C:\\<tab><tab> is now just listing the contents of the current directory, which isn't right.

[dhoegh]

I have fixed cd("C:\\<tab><tab> @tkelman

[tkelman]

Great! This works very nicely, I'm in favor of merging this as soon as someone else gives the okay on the deletion in dhoegh@d574f03.

The Travis failure on Mac was unrelated, a problem with arpack due to the openblas bump in homebrew-julia.

[dhoegh]

Hi @tkelman
Could you confirme that this bug has nothing to do with my change in autocompletion?

It occurs when writting using B<tab><tab>
I have just installed a third version of Julia, which I have left untouched, and it also seems to fail so it is just to be on the safe side.

[tkelman]

I can reproduce that either with or without your change, so I think it's unrelated. It only appears to happen if I haven't run Pkg.init() yet, and the message is slightly different for me:

julia> using BERROR: unable to read directory D:\cygwin64\home\Tony\.julia\v0.4: No error

This looks similar to the error that used to cause shell> C:\\<tab><tab> to fail due to permissions errors checking isdir() on some folders, but coming from some package/module related completion mode? Definitely a bug, would be good to fix (probably as a separate PR from a different branch nevermind looks like the fix was easier than I thought it would be) if you can find where the error's coming from. A try-catch around whatever is trying to read the nonexistent package directory should help.

[StefanKarpinski]

@dhoegh, can you explain what the purpose of this part of the change is?

[dhoegh]

If the string ends on \\ it give an error even though cd C:\\ is valid on windows:

julia> Base.shell_parse("cd C:\\",true)
ERROR: dangling backslash
 in shell_parse at string.jl:1146

And if string ends on cd C:\\P is parsed then it splits the dir path and this makes the complete_path complete on only "P" instead of the path.

julia> Base.shell_parse("cd C:\\P",true)
(:((("cd",),("C:","P"))),0:-1)

[StefanKarpinski]

This seems like the wrong place to fix this – a dangling backslash in a shell-quoted expression is still an error. In C:\\ the backslash is already "quoted" by the first backslash. I may be missing something here though.

[tkelman]

It gets escaped before the function sees it though:

julia> for c in "C:\\"; println(c); end
C
:
\

so it looks like a dangling backslash to the shell_parse function. We could instead leave in this section of code but skip it on Windows?

[dhoegh]

Since 47d3ecf the auto completion is completing with a ``` in shell> and if `\` where used then the auto completion did not work. This pull changes it to always auto complete and use `\` in paths and fixes the completion behavior. As @tkelman mention this part could be skipped on windows. Else we should role `Base.REPLCompletions.shell_completions` back to before 47d3ecf and use `/` as path separator again in shell mode on windows.

[dhoegh]

@tkelman, @StefanKarpinski I have fixed the problem regarding shell_parse. @tkelman could you check everything is working properly before a merge and backport to 0.3.

[StefanKarpinski]

Ah, this seems much less scary. I'd still like to understand this a little better, but it seems ok at a glance.

[dhoegh]

The reason why it works is in how the shell_parse returns an expression of tuples for example:

args, last_parse = shell_parse("ls -a ..\\\\test\\\\do", true)
(:((("ls",),("-a",),("..","\\test","\\do"))),0:-1)

where we would like the path at args.args[end] to be completed, this tuple is then joined, and the completion is working from there.
This do not effect Unix since it return:

args, last_parse = shell_parse("ls -a ../test/do", true)
(:((("ls",),("-a",),("../test/do",))),0:-1)

[dhoegh]

I think isexpr(args.args[end],:tuple) && could be removed since shell_parse() always returns a tuple.

[dhoegh]

Git isssue I am fixing them plus a squash.

[dhoegh]

Ok sound good. The point is in how the shell_parse groups the input in tuples:

julia> Base.shell_parse("cd -l ..\\\\test\\\\do", true)
(:((("cd",),("-l",),("..","\\test","\\do"))),0:-1)
julia> Base.shell_parse("cd -l ../test/do", true)
(:((("cd",),("-l",),("../test/do",))),0:-1)

It will group the path like above and hence should not effect the path on unix. This grouping is what I am thinking of taking advantage of when making the paths work with spaces, it would be look like this:

julia> Base.shell_parse("cd -l ../test/do\\ te", true)
(:((("cd",),("-l",),("../test/do"," te"))),0:-1)

Hence you will actually get the right path sent to "complete_path" when this is joined.

This also means that you with this pull request could do:

shell> cd C:\\Program\ F<tap>
shell> cd C:\\PProgram Files

And I plan to fix this behavior in a second pull request.

[dhoegh]

Hi, is there a plan for review/anyone having any objections?

@tkelman, @lucasb-eyer, I have made a fix for the space completion thing in commit 7e1f33a. Im happy with how it is turned out to get it in shell mode, which is five lines of code and I am quite happy with how it works. However, I do not know if I think it belongs in the julia mode like:

julia>cd("C:\\Program\ Fil<tab>

Where the backslash seems wrong. It's also account for all changes from line 151-203. I would appreciate if any of you could try it. I am going to file a pull request on it when this one is merged.

[tkelman]

@dhoegh I would love if someone else who's qualified can look at this. I have no control over others' time to do so though.

Why is the backslash wrong in Julia mode? If what we're completing on is a file/folder name, then backslash-space is the way bash in cygwin or msys2 completes paths, being consistent works for me.

Can you come up with a functioning version of the spaces-in-paths patch that doesn't change any of the same lines as this one? It seems mostly independent, and you could open a new independent PR for review maybe before this one gets merged. If this gets merged before the new one does, you can rebase the new one to include any changes to the same lines of code.

[dhoegh]

Maybe it's just me, I would love to get a pr going so the solution could be discussed, but I need two lines from this one.

[tkelman]

Which two lines? The only overlap I see in 7e1f33a is the deletion of non_filename_chars, which shouldn't be strictly necessary to start with?

[dhoegh]

The 257 and 259.

[dhoegh]

Else it's not going to work.

[tkelman]

Oh damn, gotcha. Exactly the parts I'm not sure about.

[dhoegh]

Yeah I know we should probably ping some who have a understanding of the shell_parse function.

[lucasb-eyer]

Actually, I'd say completely remove \\ because on linux I can create a file or directory named lol\hi, I just tried it: touch lol\\hi && ls. Or is there any other repercussion I'm missing?

[dhoegh]

I have no objection in removing it. Lucas could you try and give 257 and 259 a look and see if you can think/find any undesireable behivor on Linux caused by these lines? These two lines is also needed to get spaces in paths working on windows and Linux.

[lucasb-eyer]

re 257 and 259:

As I understand it, I think shell_parse is the one who should be responsible for handling escaped spaces. If it did, you wouldn't need to change those two lines, right? Especially, I don't think the following is the right thing:

julia> Base.shell_parse("ls \"folder with spaces\"")
(:((("ls",),("folder with spaces",))),0:-1)

julia> Base.shell_parse("ls folder\ with\ spaces")
(:((("ls",),("folder",),("with",),("spaces",))),0:-1)

A little git blame -L 1067,1161 base/string.jl shows that shell_parse's main author is @StefanKarpinski about four years ago. Care to give your opinion Stefan?

(I don't currently have the time to work on a patch, unfortunately.)

Edit: I did give this PR a quick try and didn't find any obvious problems, but I don't know this code any better than you.

[dhoegh]

@lucasb-eyer, when you enter shell>ls folder\ with\ spaces it is going to be parsed to the function like:

julia> Base.shell_parse("ls folder\\ with\\ spaces")
(:((("ls",),("folder"," with"," spaces"))),0:-1)

Which is correctly grouped and it just needs to be joined as I do in 259.
The same case is for windows where it comes like:

julia> Base.shell_parse("cd ..\\\\test\\\\dow", true)
(:((("cd",),("..","\\test","\\dow"))),0:-1)

So i think the shell_parse works as intended.
edit:
in you example:

julia> Base.shell_parse("ls folder\ with\ spaces")
(:((("ls",),("folder",),("with",),("spaces",))),0:-1)

I would end up joining "spaces" with nothing so the resulting path="spaces". And with the double escape character it works as intended.

[lucasb-eyer]

Right, I missed double-escaping it. I don't see anything else then, though I'm not an expert.

[dhoegh]

@lucasb-eyer thank you for your time.

[tkelman]

I'm tempted to just click merge on this and see whether anything breaks / anyone cares.

[StefanKarpinski]

Yeah, go for it.

[tkelman]

Yay, thanks. Hope you had a good flight.

[tkelman]

@JuliaBackports if this doesn't cause any problems over the next week or so