Support command line options in EDITOR/GIT_EDITOR #163
Conversation
|
@zware, @larryhastings please review. |
|
Here an example with command with spaces, and command line options: It opens vim and jump to line 25. Not tested on Windows. |
|
Yes, but you use quotes around the spaces. If the path isn't quoted, it won't run the editor properly. I don't know whether or not people put unquoted paths with spaces in environment variables, but I think it's possible to handle it--please see my previous pseudocode. |
|
I have seen your example in #156, but it will also not handle path with spaces and command line arguments. If you don't quote the path, we get: >>> p = "C:/Program Files (x86)/Notepad++/notepad++.exe -multiInst -nosession"
>>> shlex.split(p, posix=False)
['C:/Program', 'Files', '(x86)/Notepad++/notepad++.exe', '-multiInst', '-nosession']We can use this hack for windows: >>> p[:p.index('.exe') + 4]
'C:/Program Files (x86)/Notepad++/notepad++.exe'
>>> shlex.split(p[p.index('.exe') + 4:])
['-multiInst', '-nosession'] |
|
It's impossible** to handle a string that contains both a path with unquoted spaces and additional command-line arguments. My point was, I think we need to handle these two scenarios:
My proposed approach would handle both those cases; IIUC your current approach only handles the latter. I think it's reasonable to handle both these scenarios. After all, there are plenty of examples of environment variables that can contain unquoted paths containing spaces (e.g. $PATH, or %APPDATA% on Windows). And there are also examples specifically of $EDITOR containing command-line arguments, as you have demonstrated. Therefore, I don't want to merge your PR with its current approach. If you'd like to handle this more complicated logic, go ahead and try; otherwise we should abandon this PR and I'll do the more complicated thing. ** well, not impossible to handle such a string. but it would be ridiculous to try. let's not go there. |
|
Mercurial seems doing like tihs: See https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/ui.py#l1379 |
|
@larryhastings next patch implements what you suggested, supporting both @methane Mercurial solution is elegant, but it will not work with unquoted path. |
On Windows users may like to configure command line options for the editor[1]. Support this configuration. Fixes #162. [1] https://git-scm.com/book/en/v2/Getting-Started-First-Time-Git-Setup
EDITOR environment variable value may be:
- "'quoted path' -with -options"
- "unquoted path"
This previous patch supported only the first; this patch adds support to
for both options.
If an environment value is set, but the value cannot be resolved, the
user has to fix the configuration. There are two possible failures:
The environment variable value does not resolve to a file:
Error: Invalid 'EDITOR' environment variable "foo bar": Cannot find editor.
Splitting the environment variable failed:
Error: Invalid 'EDITOR' environment variable "foo bar' -with -options": No closing quotation.
|
@larryhastings, can you review the latest version again? |
| if os.path.exists(s): | ||
| return [s] | ||
| # "'quoted path to editor' -with -options"? | ||
| args = shlex.split(s, posix=os.name == 'posix') |
There was a problem hiding this comment.
I learned two new things today! The "posix" argument to shlex.split, and os.name is usually "posix".
| @@ -864,7 +882,7 @@ def find_editor(): | |||
| else: | |||
| found_path = shutil.which(fallback) | |||
| if found_path and os.path.exists(found_path): | |||
| return found_path | |||
| return [found_path] | |||
There was a problem hiding this comment.
Is this a bug fix folded in with the rest of the PR? I appreciate it, I just want to double-check. It doesn't look like the rest of the PR touches found_path.
There was a problem hiding this comment.
Yes, this looks like a bug fix, I can split it to another PR if you like.
|
|
||
| handle, tmp_path = tempfile.mkstemp(".rst") | ||
| os.close(handle) | ||
| atexit.register(lambda : os.unlink(tmp_path)) | ||
| editor_command.append(tmp_path) |
There was a problem hiding this comment.
It seems like your new code is doing nice things, but then doesn't use them. editor_command is ignored after this line; I was assuming it should replace "args".
Also, the code now appends tmp_path twice. This line adds one, and then line 913 in the original (line 932 after your patch is applied) appends tmp_path to "args".
There was a problem hiding this comment.
editor_command is used in line 837. I don't see the append you mention. I'll need to rebase and test this again, lot of time has passed since I sent this :-)
|
This PR seems to be languishing. That's fine, but I would like the If you have the time I'd be happy for you to make a one-line PR to get that fix in, so you can get credit for it. Or I can just do it if you don't care about getting credit. |
|
I'm sorry but I don't have the time to work on this. |
On Windows users may like to configure command line options for the
editor[1]. Support this configuration.
Fixes python/blurb#2.
[1] https://git-scm.com/book/en/v2/Getting-Started-First-Time-Git-Setup