Patchwork debuginstall: handle quoted path for editor (issue4316)

login
register
mail settings
Submitter Alexandre Garnier
Date July 29, 2014, 9:01 p.m.
Message ID <b817183e354981f9088a.1406667691@nabuchodonosor>
Download mbox | patch
Permalink /patch/5208/
State Superseded
Commit 2122b82b6987c074ce9b4283c5d2b5927701a36c
Headers show

Comments

Alexandre Garnier - July 29, 2014, 9:01 p.m.
# HG changeset patch
# User Alexandre Garnier <zigarn@gmail.com>
# Date 1406665496 -7200
#      Tue Jul 29 22:24:56 2014 +0200
# Branch stable
# Node ID b817183e354981f9088a85dea76dbc865c013d3a
# Parent  ad56fc55cbc3870d257e163469c687088627283b
debuginstall: handle quoted path for editor (issue4316)

When using an editor path with spaces and options, you can set 'ui.editor'
to '"/path to your/editor" -opt' and it works fine but 'hg debuginstall'
is complaining about it because it simply splits the editor and
tests presence of '"/path'.
Now correctly parse 'ui.editor' string by handling quoted path.
Matt Mackall - July 30, 2014, 9:12 p.m.
On Tue, 2014-07-29 at 23:01 +0200, Alexandre Garnier wrote:
> # HG changeset patch
> # User Alexandre Garnier <zigarn@gmail.com>
> # Date 1406665496 -7200
> #      Tue Jul 29 22:24:56 2014 +0200
> # Branch stable
> # Node ID b817183e354981f9088a85dea76dbc865c013d3a
> # Parent  ad56fc55cbc3870d257e163469c687088627283b
> debuginstall: handle quoted path for editor (issue4316)

> +      or util.findexe(re.search(r"^'?([^']+)'?(?:\s+.*)?", editor).group(1)))

Looks like this regex has some problems:

>>> re.search(r"^'?([^']+)'?(?:\s+.*)?", "emacs -opt").group(1)
'emacs -opt'

>>> re.search(r"^'?([^']+)'?(?:\s+.*)?", '"emacs -opt" blah').group(1)
'"emacs -opt" blah'

Probably wants to use shlex.
Alexandre Garnier - July 31, 2014, 9:34 a.m.
2014-07-30 23:12 GMT+02:00 Matt Mackall <mpm@selenic.com>:
> On Tue, 2014-07-29 at 23:01 +0200, Alexandre Garnier wrote:
>> # HG changeset patch
>> # User Alexandre Garnier <zigarn@gmail.com>
>> # Date 1406665496 -7200
>> #      Tue Jul 29 22:24:56 2014 +0200
>> # Branch stable
>> # Node ID b817183e354981f9088a85dea76dbc865c013d3a
>> # Parent  ad56fc55cbc3870d257e163469c687088627283b
>> debuginstall: handle quoted path for editor (issue4316)
>
>> +      or util.findexe(re.search(r"^'?([^']+)'?(?:\s+.*)?", editor).group(1)))
>
> Looks like this regex has some problems:
>
>>>> re.search(r"^'?([^']+)'?(?:\s+.*)?", "emacs -opt").group(1)
> 'emacs -opt'
>
>>>> re.search(r"^'?([^']+)'?(?:\s+.*)?", '"emacs -opt" blah').group(1)
> '"emacs -opt" blah'
>
> Probably wants to use shlex.
>

I kept the idea to test multiple cases:

editor = "emacs -opt"
util.findexe(editor)
    ==> False
or util.findexe(editor.split()[0])
    ==> util.findexe('emacs')   ==> True
or util.findexe(re.search(r'^"?([^"]+)"?(?:\s+.*)?', editor).group(1))
    ==> not evaluated
or util.findexe(re.search(r"^'?([^']+)'?(?:\s+.*)?",
editor).group(1)))    ==> not evaluated


editor = '"emacs -opt" blah' # Strange editor definition
util.findexe(editor)
    ==> False
or util.findexe(editor.split()[0])
    ==> util.findexe('"emacs')              ==> False
or util.findexe(re.search(r'^"?([^"]+)"?(?:\s+.*)?', editor).group(1))
    ==> util.findexe('emacs -opt')          ==> False
or util.findexe(re.search(r"^'?([^']+)'?(?:\s+.*)?",
editor).group(1)))    ==> util.findexe('"emacs -opt" blah')   ==>
False

editor = '"/path to/emacs" -opt blah'
util.findexe(editor)
    ==> False
or util.findexe(editor.split()[0])
    ==> util.findexe('"/path')           ==> False
or util.findexe(re.search(r'^"?([^"]+)"?(?:\s+.*)?', editor).group(1))
    ==> util.findexe('/path to/emacs')   ==> True
or util.findexe(re.search(r"^'?([^']+)'?(?:\s+.*)?",
editor).group(1)))    ==> not evaluated


editor = "'/path to/emacs' -opt blah"
util.findexe(editor)
    ==> False
or util.findexe(editor.split()[0])
    ==> util.findexe("'/path")                       ==> False
or util.findexe(re.search(r'^"?([^"]+)"?(?:\s+.*)?', editor).group(1))
    ==> util.findexe("'/path to/emacs' -opt blah")   ==> False
or util.findexe(re.search(r"^'?([^']+)'?(?:\s+.*)?",
editor).group(1)))    ==> util.findexe('/path to/emacs')
==> True

But shlex looks like a good solution to do this easely (didn't know it):

editor = "emacs -opt"
util.findexe(shlex.split(editor)[0])   ==>   util.findexe('emacs')   ==> True

editor = '"emacs -opt" blah'
util.findexe(shlex.split(editor)[0])   ==>   util.findexe('emacs
-opt')   ==> False

editor = '"/path to/emacs" -opt blah'
util.findexe(shlex.split(editor)[0])   ==>   util.findexe('/path
to/emacs')   ==> True (if it's the right path)

editor = "'/path to/emacs' -opt blah"
util.findexe(shlex.split(editor)[0])   ==>   util.findexe('/path
to/emacs')   ==> True (if it's the right path)

editor = '"C:/Program Files (x86)/Notepad++/notepad++.exe" -multiInst
-notabbar -nosession -noPlugin'
util.findexe(shlex.split(editor)[0])   ==>   util.findexe('C:/Program
Files (x86)/Notepad++/notepad++.exe')   ==> True

editor = "'C:/Program Files (x86)/Notepad++/notepad++.exe' -multiInst
-notabbar -nosession -noPlugin"
util.findexe(shlex.split(editor)[0])   ==>   util.findexe('C:/Program
Files (x86)/Notepad++/notepad++.exe')   ==> True

editor = '"C:/Program Files (x86)/Notepad\'s path/notepad++.exe"
-multiInst -notabbar -nosession -noPlugin'
util.findexe(shlex.split(editor)[0])   ==>   util.findexe("C:/Program
Files (x86)/Notepad's path/notepad++.exe")   ==> True

editor = sys.executable + ' -c "import sys; sys.exit(0)"'
util.findexe(shlex.split(editor)[0])   ==>
util.findexe('/usr/bin/python')   ==> True

So, unless the strange '"emacs -opt" blah', it will cover the majority
of use cases.
I'll provide a new patch.

Patch

diff -r ad56fc55cbc3 -r b817183e3549 mercurial/commands.py
--- a/mercurial/commands.py	Mon Jul 28 10:05:17 2014 +0200
+++ b/mercurial/commands.py	Tue Jul 29 22:24:56 2014 +0200
@@ -2248,7 +2248,9 @@ 
     # editor
     ui.status(_("checking commit editor...\n"))
     editor = ui.geteditor()
-    cmdpath = util.findexe(editor) or util.findexe(editor.split()[0])
+    cmdpath = (util.findexe(editor) or util.findexe(editor.split()[0])
+      or util.findexe(re.search(r'^"?([^"]+)"?(?:\s+.*)?', editor).group(1))
+      or util.findexe(re.search(r"^'?([^']+)'?(?:\s+.*)?", editor).group(1)))
     if not cmdpath:
         if editor == 'vi':
             ui.write(_(" No commit editor set and can't find vi in PATH\n"))