edit command breaks for EDITOR path containing spaces #13032

omus · 2015-09-09T16:48:38Z

I noticed when using the edit function in Julia that I was getting this message:

julia> edit(edit, (String,))
Unknown editor: no line number information passed.
The method is defined at line 56.

After doing some digging I noticed that my default editor Sublime is supported by the edit function but something else was going on. My editor is set using the environmental variable EDITOR and looks like this:

$ echo $EDITOR
/Applications/Sublime\ Text.app/Contents/SharedSupport/bin/subl

In Julia the path includes the backslash but is not recognized as a path:

julia> ENV["EDITOR"]
"/Applications/Sublime\\ Text.app/Contents/SharedSupport/bin/subl"

julia> ispath(ENV["EDITOR"])
false

Since the ispath function doesn't recognize this string as a path the edit function will not recognize the editor as "subl" and state that line number support is not available. Note that if I set the EDITOR to not include the backslash ispath will pass but will cause the spawn call to fail:

julia> ENV["EDITOR"] = "/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl"
"/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl"

julia> ispath(ENV["EDITOR"])
true

julia> edit(edit, (String,))
ERROR: could not spawn `/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl /usr/local/Cellar/julia/HEAD/bin/../share/julia/base/interactiveutil.jl:56`: no such file or directory (ENOENT)
 in _jl_spawn at process.jl:255
 in anonymous at process.jl:408
 in setup_stdio at process.jl:396
 in spawn at process.jl:407
 in edit at interactiveutil.jl:37
 in edit at interactiveutil.jl:58

I have updated the code to use shell_split earlier which strips the backslash and makes it easier to find the editor name. Another improvement is allowing the if the user includes flags in their EDITOR (such as ".../subl -w") we can still determine the editor name.

I believe that this pull request fixes the issue in #11305 and is an improvement over the pull request #11329.

stevengj · 2015-09-09T19:53:12Z

I'm confused, shouldn't your EDITOR environment variable be

"/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl"

? Why does it include an explicit backslash character?

omus · 2015-09-10T18:13:29Z

@stevengj After further inspection I believe you are correct and I shouldn't be including the backslash. For some context here is what I had for my Sublime terminal setup:

SUBL2="/Applications/Sublime\ Text\ 2.app/Contents/SharedSupport/bin/subl"
SUBL3="/Applications/Sublime\ Text.app/Contents/SharedSupport/bin/subl"
SUBL=$SUBL3

alias subl2="$SUBL2"
alias subl3="$SUBL3"
alias subl="$SUBL"

export EDITOR="$SUBL"

The reason I included the backslash was to make the aliases work. Without the backslash I was getting -bash: /Applications/Sublime: No such file or directory. Upon further inspection I found that crontab -e was broken using this setup. After some tinkering my correct terminal setup is now:

SUBL2="/Applications/Sublime Text 2.app/Contents/SharedSupport/bin/subl"
SUBL3="/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl"
SUBL=$SUBL3

# Note: Single quotes used to handle spaces
alias subl2="'$SUBL2'"
alias subl3="'$SUBL3'"
alias subl="'$SUBL'"

export EDITOR="$SUBL"

Note that my updated setup still has problems with Julia's edit method not working correctly. I'll do some additional work and see if I can find a better solution.

omus · 2015-09-10T19:09:38Z

Now after running several experiments the EDITOR variable using crontab -e, git commit, and Julia's edit method I think we definitely should be escaping an EDITOR path that contains spaces.

My rational behind this decision is that the EDITOR variable can contain more than just a path to an executable and can contain flags. When an EDITOR contains both spaces in the executable path and flags it becomes difficult to determine the editor's name.

eg.

julia> shell_split("/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl -w")
3-element Array{AbstractString,1}:
 "/Applications/Sublime"                   
 "Text.app/Contents/SharedSupport/bin/subl"
 "-w"

vs.

julia> shell_split("/Applications/Sublime\\ Text.app/Contents/SharedSupport/bin/subl -w")
2-element Array{AbstractString,1}:
 "/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl"
 "-w" 

julia> shell_split("'/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl' -w")
2-element Array{AbstractString,1}:
 "/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl"
 "-w"

Alternatively, if we really wanted to we could support an EDITOR without escaped spaces by assuming that no flags will exist. Note that the command git commit works with flags and requires escaped spaces where as crontab -e only works when variable is an unescaped string without flags.

One last note: I'll be adding the following to my terminal setup to keep crontab and Julia happy:

export JULIA_EDITOR="'/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl'"

omus · 2015-09-10T19:19:40Z

@stevengj Here is the short answer to your original question:

The backslash was included to escape the path such that the spawn call could find the executable. Without it spawn tries to run /Applications/Sublime which isn't a valid executable.

eg.

julia> ENV["EDITOR"] = "/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl"
"/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl"

julia> edit(edit, (String,))
ERROR: could not spawn `/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl /usr/local/Cellar/julia/HEAD/bin/../share/julia/base/interactiveutil.jl:56`: no such file or directory (ENOENT)
 in _jl_spawn at process.jl:255
 in anonymous at process.jl:408
 in setup_stdio at process.jl:396
 in spawn at process.jl:407
 in edit at interactiveutil.jl:37
 in edit at interactiveutil.jl:58

stevengj · 2015-09-10T19:29:18Z

@omus, it is a bug in jl_spawn (or something) if it can't handle executable paths with spaces.

See also here ... if you have paths with spaces, you should generally not escape them in environment variables, but rather you (we) should properly quote them when they are used.

stevengj · 2015-09-10T19:35:21Z

In particular, it looks like the culprit is the call to shell_split in the edit function, not in spawn.

The bug was introduced by #6865 by @jayschwa in order to handle sublime arguments, but the "cure" introduced a bug whenever the sublime path contains a space.

stevengj · 2015-09-10T19:40:23Z

In general, any call to shell_split seems to have this problem. I'm not sure what the right solution is. How do we distinguish between executing the file named "foo file" and executing the file "foo" with the argument "file"?

I guess we could call shell_split only starting at hyphen (or a double hyphen) preceded by whitespace, although that heuristic would fail for a file perversely named "foo -bar" (which is a perfectly valid filename).

stevengj · 2015-09-10T19:43:58Z

(But in any case, the proposed solution, of recognizing backslashed spaces in a pathname, is definitely wrong.)

omus · 2015-09-10T19:45:52Z

@stevengj I'll note that the current implementation of edit (prior to this pull request) does work with an escaped pathname using a backslash. My fix just lets edit determine the editor name from the path such that the line number is passed in.

omus · 2015-09-10T19:48:53Z

I agree that escaping/quoting should happen as late as possible. In this case however I don't think you can escape later due to the example you illustrated with foo file. What if you have foo bar baz? Is this the executable foo with two arguments? Maybe it is the executable foo bar with one argument or just the executable foo bar baz.

stevengj · 2015-09-10T22:43:21Z

@omus, the problem is that there are two conflicting desires here:

support paths with spaces: favors spawn($editor $file -l $line) and similar
support EDITOR commands that include options (sublime --wait): requires you to split editor into multiple strings passed to spawn: use shell_split or similar

Your patch fixes the former at the price of breaking the latter (#6865). Maybe this is a good idea, or maybe there is a heuristic to mostly support both (e.g. split after substrings beginning with " --").

stevengj · 2015-09-10T22:45:38Z

I'm quite surprised that it worked with an escaped backslash. If you have a file "foo bar", you can't open it with open("foo\\ bar"). Why would spawn("foo\\ bar") work? Is this some hidden OSX or libuv feature?

omus · 2015-09-10T23:07:40Z

The reason the escaped backslash calls were working was due to shell_split

julia> subl = "/Applications/Sublime\\ Text.app/Contents/SharedSupport/bin/subl"
"/Applications/Sublime\\ Text.app/Contents/SharedSupport/bin/subl"

julia> `$subl`
`'/Applications/Sublime\ Text.app/Contents/SharedSupport/bin/subl'`

julia> run(`$subl`)
ERROR: could not spawn `'/Applications/Sublime\ Text.app/Contents/SharedSupport/bin/subl'`: no such file or directory (ENOENT)
 in _jl_spawn at process.jl:255
 in anonymous at process.jl:408
 in setup_stdio at /usr/local/Cellar/julia/HEAD/lib/julia/sys.dylib
 in __spawn#58__ at /usr/local/Cellar/julia/HEAD/lib/julia/sys.dylib
 in run at /usr/local/Cellar/julia/HEAD/lib/julia/sys.dylib

julia> import Base: shell_split; `$(shell_split(subl))`
`'/Applications/Sublime Text.app/Contents/SharedSupport/bin/subl'`

julia> run(`$(shell_split(subl))`)
# Success

omus · 2015-09-10T23:19:11Z

My goal here is to have Julia's edit command work with any EDITOR variable as git would. From what I can tell that means supporting:

Editors on the path (vim)
Editors including a path (/usr/bin/vim)
Editors with a space in the path (/Applications/Sublime\ Text/.../subl, '/Applications/Sublime Text/.../subl')
Allowing flags (subl -w)

I may wrong here but I feel like we need to be supporting what is allowed by the shell and not forcing users to update their environment to suit Julia.

omus · 2015-09-11T17:31:16Z

When working within Julia with Cmd I agree that you should not be escaping paths since this can be handled gracefully within Julia:

julia> cmd = "foo bar"; file = "baz"; c = `$cmd $file`
`'foo bar' baz`

However when working with paths coming from outside of Julia, like ENV for example, I believe that you should be using shell_split or shell_parse to convert the command into something that is compatible with Cmd. If fact I believe that this is the purpose of the shell_split command. @StefanKarpinski can you comment if this is intended use for shell_split? I think your the creator of the function. The two other occurrences shell_split is used in Base Julia it appears to be used for this purpose.

stevengj · 2015-09-11T19:32:31Z

I can't find any sources that actually document a recommendation to backslash-escape spaces in the contents of variables like EDITOR. Can you give any pointers to indicate that this is not just an idiosyncrasy of specific programs like git?

Even in the shell, all the sources that I can find advise using environment variables quoted like "$THIS" to handle embedded spaces, and most C programs will just call getenv to get paths and won't do any parsing to unescape backslashes.

omus · 2015-09-12T00:56:27Z

Unfortunately the EDITOR variable doesn't have that much surrounding documentation it. I'm approaching this as a bash user that is used to quoting in order to avoid word splitting. Here's a quick overview of why this is necessary in bash:

$ touch '/tmp/a b'

$ bash -c "ls /tmp/a b"
ls: /tmp/a: No such file or directory
ls: b: No such file or directory

$ bash -c "ls /tmp/a\ b"
/tmp/a b

$ bash -c "ls '/tmp/a b'"
/tmp/a b

Julia behaves in the exact same way as bash:

julia> run(`ls /tmp/a b`)
ls: /tmp/a: No such file or directory
ls: b: No such file or directory
ERROR: failed process: Process(`ls /tmp/a b`, ProcessExited(1)) [1]
 in run at /usr/local/Cellar/julia/HEAD/lib/julia/sys.dylib

julia> run(`ls /tmp/a\ b`)
/tmp/a b

julia> run(`ls '/tmp/a b'`)
/tmp/a b

julia> run(`ls "/tmp/a b"`)
/tmp/a b

julia> string(`ls /tmp/a\ b`) == string(`ls '/tmp/a b'`) == string(`ls "/tmp/a b"`)
true

The fundamental reason we need to support for quoting in this particular case is to allow Julia to support an EDITOR variable that contains both spaces in a path and arguments. Otherwise foo bar baz means run the program 'foo bar baz' and not run the program foo with arguments bar and baz (the later is bash and Julia's current behaviour).

I'm starting to get confused to what the fundamental problem is exactly. The proposed solution doesn't change the prior behaviour in regards to:

Editors on the path (vim)
Editors including a path (/usr/bin/vim)

But adds support for the following for all editors:

Allowing flags (subl -w)
Editors with a space in the path. Requires quoting.

I can't think of another solution to this problem that allows for both flags and spaces in paths.

stevengj · 2015-09-12T02:58:14Z

I see; I had thought your patch didn't support arguments, but I see that I was mistaken. (I'm used to quoting, but not in environment variables like EDITOR or PATH, as opposed to quoting around them as in "$PATH"; I've never seen that recommended before.)

Looks reasonable to me now; the key point is that it is not really a change of our quoting/parsing behavior, but rather it just makes it more uniform for all editors. You'll need to squash the commits, of course.

Is there any way to add a test for this? Maybe at least some tests for shell_split can be added?

omus · 2015-09-12T03:23:47Z

I'll do some experimenting with how to test this. I definitely don't want to actually launch the editor during a test but I could probably abstract out the generation of the Cmd such that we can check it against what it intends to run. Does that sound reasonable?

Am I correct in that you want me to combine all of the commits before this is merged?

stevengj · 2015-09-12T12:39:08Z

Yes, squashing means combining all the commits. You can do that with an interactive rebase (git rebase -i) followed by a forced push (git push -f).

stevengj · 2015-09-12T12:45:04Z

I think it's probably enough to add a couple of tests for shell_split. Refactoring edit seems like more trouble than it is worth.

omus · 2015-09-14T20:05:20Z

I am aware of squashing but I was surprised that I was asked to do it for a PR. Note that --force-with-lease is safer option for a forced push.

I focused on adding tests for quoting. Let me know if you want additional tests.

omus · 2015-09-14T20:19:32Z

I think I can abstract out the part of edit that finds the editor. If I find I nice way to do it it will allow us to have reasonable test cases for edit that will help avoid the introduction of bugs.

omus · 2015-09-15T14:20:39Z

@stevengj I added test cases for shell_split and I went through the effort of abstracting the part of edit that finds the editor. The new editor function returns an array that can be used easily inside of backticks. This abstraction allowed me to test to ensure that both spaces and arguments are being handled correctly.

One behavioural change I did was to remove the following check:

ispath(editor) && !isreadable(editor) && error("can't find \"$editor\"")

Which isn't a very useful as it will only error if the program is exists in the system (ispath) and the permissions are not set to readable. I can add this back in if you wish.

Let me know if you are happy with all of these changes. If you are I will squash the last two changes together and this PR should be ready to be merged.

stevengj · 2015-09-15T17:30:58Z

base/interactiveutil.jl

+    editor()
+
+Determines the editor to use when running functions like `edit`. You can change the editor
+by setting JULIA_EDITOR, VISUAL, or EDITOR as an environmental variable.


Document that it returns an array to splat into backticks?

stevengj · 2015-09-15T17:33:32Z

Looks good, though I'm not completely sure these tests should go into two separate files.

omus · 2015-09-15T20:16:25Z

I reworked the shell_split tests that were for edit to be more general.

Looks good, though I'm not completely sure these tests should go into two separate files.

Do you want me to put the tests in the same file or remove the redundant tests?

stevengj · 2015-09-15T20:37:40Z

Maybe put the editor tests in replutil.jl and the shell_split tests in spawn.jl?

omus · 2015-09-16T20:48:08Z

Ok I moved the tests into the appropriate files.

stevengj · 2015-09-16T23:40:27Z

LGTM. Squash the commits, then probably good to merge?

omus · 2015-09-17T13:43:36Z

Lets merge this! Thanks for your patience on this PR @stevengj

yuyichao · 2015-09-17T13:48:24Z

base/interactiveutil.jl

+for use within backticks. You can change the editor by setting JULIA_EDITOR, VISUAL, or
+EDITOR as an environmental variable.
+"""
+function editor()


Should this be sync'd to the rst doc?

editor isn't an exported function so I would guess this should not be included in the external documentation. Opinions?

Should be fine to leave this as is for now....

omus · 2015-09-17T19:48:09Z

I randomly stumbled upon withenv and included it in test/replutil.jl

When working with Sublime as my default editor on OS X I had set my editor to: EDITOR=/Applications/Sublime\ Text.app/Contents/SharedSupport/bin/subl Since the `ispath` function did not recognize this as a path (because contained a backslash) this resulted in my editor not being recognized as "subl" and line number support being unavailable. The section of `edit` that deals with determining the user's editor has been abstracted into a new function `editor`. Test cases have been added for `editor` along with some basic tests for `shell_split`. Note that `startswith` is used to identify alternative editor names: - Sublime Text can refer to "subl" or "sublime". - Emacs can refer to "emacs" or "emacsclient".

edit command breaks for EDITOR path containing spaces

tkelman · 2015-09-17T23:59:08Z

My preference would be to backport this after 0.4.0, since it might require some hands-on interactive testing to get the kinks out.

stevengj · 2015-09-18T02:38:34Z

Yes, 0.4.1 seems fine. I hate to see anything non-critical go into 0.4.0 this late.

omus · 2015-09-18T03:39:15Z

Completely fine with me.

When working with Sublime as my default editor on OS X I had set my editor to: EDITOR=/Applications/Sublime\ Text.app/Contents/SharedSupport/bin/subl Since the `ispath` function did not recognize this as a path (because contained a backslash) this resulted in my editor not being recognized as "subl" and line number support being unavailable. The section of `edit` that deals with determining the user's editor has been abstracted into a new function `editor`. Test cases have been added for `editor` along with some basic tests for `shell_split`. Note that `startswith` is used to identify alternative editor names: - Sublime Text can refer to "subl" or "sublime". - Emacs can refer to "emacs" or "emacsclient". (cherry picked from commit 53fcec2) ref #13032

stevengj changed the title ~~Updated edit to handle backslashes better~~ edit command breaks for EDITOR path containing spaces Sep 10, 2015

stevengj added the bug Indicates an unexpected problem or unintended behavior label Sep 10, 2015

omus force-pushed the edit_backslash branch from f1a74ad to db4caa8 Compare September 14, 2015 19:59

stevengj reviewed Sep 15, 2015
View reviewed changes

omus force-pushed the edit_backslash branch from 48b064d to 4a5448d Compare September 17, 2015 13:41

yuyichao reviewed Sep 17, 2015
View reviewed changes

omus force-pushed the edit_backslash branch from 4a5448d to 53fcec2 Compare September 17, 2015 19:46

stevengj added the backport pending 0.4 label Sep 17, 2015

stevengj added a commit that referenced this pull request Sep 17, 2015

Merge pull request #13032 from omus/edit_backslash

66eb856

edit command breaks for EDITOR path containing spaces

stevengj merged commit 66eb856 into JuliaLang:master Sep 17, 2015

tkelman added this to the 0.4.1 milestone Sep 20, 2015

This was referenced Sep 22, 2015

edit doesn't work with EDITOR set to 'emacsclient -t' (or other values with options) #8120

Closed

Use shell_split in edit(). Fixes #8120 #10510

Closed

edit() fails on Windows if the editor path contains slashes #9331

Closed

omus deleted the edit_backslash branch October 29, 2015 21:51

tkelman removed the backport pending 0.4 label Nov 9, 2015

edit command breaks for EDITOR path containing spaces #13032

edit command breaks for EDITOR path containing spaces #13032

Conversation

omus commented Sep 9, 2015

stevengj commented Sep 9, 2015

omus commented Sep 10, 2015

omus commented Sep 10, 2015

omus commented Sep 10, 2015

stevengj commented Sep 10, 2015

stevengj commented Sep 10, 2015

stevengj commented Sep 10, 2015

stevengj commented Sep 10, 2015

omus commented Sep 10, 2015

omus commented Sep 10, 2015

stevengj commented Sep 10, 2015

stevengj commented Sep 10, 2015

omus commented Sep 10, 2015

omus commented Sep 10, 2015

omus commented Sep 11, 2015

stevengj commented Sep 11, 2015

omus commented Sep 12, 2015

stevengj commented Sep 12, 2015

omus commented Sep 12, 2015

stevengj commented Sep 12, 2015

stevengj commented Sep 12, 2015

omus commented Sep 14, 2015

omus commented Sep 14, 2015

omus commented Sep 15, 2015

stevengj Sep 15, 2015

Choose a reason for hiding this comment

omus Sep 15, 2015

Choose a reason for hiding this comment

stevengj commented Sep 15, 2015

omus commented Sep 15, 2015

stevengj commented Sep 15, 2015

omus commented Sep 16, 2015

stevengj commented Sep 16, 2015

omus commented Sep 17, 2015

yuyichao Sep 17, 2015

Choose a reason for hiding this comment

omus Sep 17, 2015

Choose a reason for hiding this comment

yuyichao Sep 17, 2015

Choose a reason for hiding this comment

omus commented Sep 17, 2015

tkelman commented Sep 17, 2015

stevengj commented Sep 18, 2015

omus commented Sep 18, 2015