Chromium Code Reviews
|
|
Chromium Code Reviews|
Created:
9 years, 9 months ago by Mohamed Mansour Modified:
9 years, 6 months ago Reviewers:
TVL, M-A Ruel CC:
chromium-reviews Visibility:
Public. |
DescriptionReland - Support msysgit in gcl and git-cl
An explicit msysgit check is needed since it requires
the usage of 'env' to open the editor.
BUG=70550, 70548
TEST=Win,Linux
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=77946
Patch Set 1 #
Total comments: 4
Patch Set 2 : '' #
Total comments: 1
Patch Set 3 : '' #
Total comments: 3
Created: 9 years, 9 months ago
Messages
Total messages: 10 (0 generated)
This change ensures "env" only present in msysgit. Verified in: - Windows XP (new VM) - Windows 7 64bit (console) - Windows 7 64bit (msysgit) - Linux (ubuntu)
http://codereview.chromium.org/6680019/diff/1/gcl.py File gcl.py (right): http://codereview.chromium.org/6680019/diff/1/gcl.py#newcode1071 gcl.py:1071: msysgit_found = os.environ['PATH'].find('mingw\\bin') != -1 if sys.platform == 'win32' and 'mingw\\bin' in os.environ['PATH']: cmd.insert(0, 'env') http://codereview.chromium.org/6680019/diff/1/git_cl/git_cl.py File git_cl/git_cl.py (right): http://codereview.chromium.org/6680019/diff/1/git_cl/git_cl.py#newcode717 git_cl/git_cl.py:717: if sys.platform == 'win32': same
http://codereview.chromium.org/6680019/diff/1/gcl.py File gcl.py (right): http://codereview.chromium.org/6680019/diff/1/gcl.py#newcode1071 gcl.py:1071: msysgit_found = os.environ['PATH'].find('mingw\\bin') != -1 On 2011/03/12 18:27:28, Marc-Antoine Ruel wrote: > if sys.platform == 'win32' and 'mingw\\bin' in os.environ['PATH']: > cmd.insert(0, 'env') Done. http://codereview.chromium.org/6680019/diff/1/git_cl/git_cl.py File git_cl/git_cl.py (right): http://codereview.chromium.org/6680019/diff/1/git_cl/git_cl.py#newcode717 git_cl/git_cl.py:717: if sys.platform == 'win32': On 2011/03/12 18:27:28, Marc-Antoine Ruel wrote: > same Done.
lgtm http://codereview.chromium.org/6680019/diff/3002/gcl.py File gcl.py (right): http://codereview.chromium.org/6680019/diff/3002/gcl.py#newcode1067 gcl.py:1067: # Msysgit requires the usage of 'env' to be present. The only way to nit: Can you move lines 1067 and 1068 after 1070? Same for git-cl.py
http://codereview.chromium.org/6680019/diff/1005/git_cl/git_cl.py File git_cl/git_cl.py (right): http://codereview.chromium.org/6680019/diff/1005/git_cl/git_cl.py#newcode720 git_cl/git_cl.py:720: subprocess.check_call(cmd) this has broken macosx and probably link when folks have editor set to things like 'edit -w --resume'. They needed to bounce through env to handle the fact that editor is a command and not a single path.
http://codereview.chromium.org/6680019/diff/1005/git_cl/git_cl.py File git_cl/git_cl.py (right): http://codereview.chromium.org/6680019/diff/1005/git_cl/git_cl.py#newcode720 git_cl/git_cl.py:720: subprocess.check_call(cmd) On 2011/03/14 13:31:54, TVL wrote: > this has broken macosx and probably link when folks have editor set to things > like 'edit -w --resume'. They needed to bounce through env to handle the fact > that editor is a command and not a single path. Sorry, it's not env as much as shell=True.
http://codereview.chromium.org/6680019/diff/1005/git_cl/git_cl.py File git_cl/git_cl.py (right): http://codereview.chromium.org/6680019/diff/1005/git_cl/git_cl.py#newcode720 git_cl/git_cl.py:720: subprocess.check_call(cmd) Hi TVL, would changing cmd = [editor, filename] to "cmd = editor.split() + [filename]" work ?
Main issue is when there is quotes in EDITOR variable content. Better to keep shell=True. Le 14 mars 2011 10:01, <mhm@chromium.org> a écrit : > > http://codereview.chromium.org/6680019/diff/1005/git_cl/git_cl.py > File git_cl/git_cl.py (right): > > http://codereview.chromium.org/6680019/diff/1005/git_cl/git_cl.py#newcode720 > git_cl/git_cl.py:720: subprocess.check_call(cmd) > Hi TVL, would changing cmd = [editor, filename] to "cmd = editor.split() > + [filename]" work ? > > http://codereview.chromium.org/6680019/
I don't quite understand how "shell=True" worked before. For example: Opens up VIM with the file contents of "foo" and places the cursor at the end of the document. > subprocess.check_call( ['vim', '+', '/home/m0/foo']) While adding shell=True doesn't even load the file contents, just an empty editor. > subprocess.check_call(['vim', '+', '/home/m0/foo'], shell=True) That is not the effect that we needed. I verified if people set editor to be "vim +" It needs to split it first. Unfortunately I am at work now so I cannot submit any change now :( If any one of you can verify Mac and submit a CL then I can review otherwise I have to wait till I come back home. Sorry :( Kind regards, Mohamed Mansour On Mon, Mar 14, 2011 at 10:04 AM, Marc-Antoine Ruel <maruel@chromium.org>wrote: > Main issue is when there is quotes in EDITOR variable content. Better to > keep shell=True. > Le 14 mars 2011 10:01, <mhm@chromium.org> a écrit : > > > > > http://codereview.chromium.org/6680019/diff/1005/git_cl/git_cl.py > > File git_cl/git_cl.py (right): > > > > > http://codereview.chromium.org/6680019/diff/1005/git_cl/git_cl.py#newcode720 > > git_cl/git_cl.py:720: subprocess.check_call(cmd) > > Hi TVL, would changing cmd = [editor, filename] to "cmd = editor.split() > > + [filename]" work ? > > > > http://codereview.chromium.org/6680019/ >
Was the script also sticking 'env' in as the first argument so it bounced though that on the invoke? ie: ['env', 'vim +', '/home/m0/foo']. The code below is probably failing because '+' trips up things in shell? TVL On Mon, Mar 14, 2011 at 10:15 AM, Mohamed Mansour <mhm@chromium.org> wrote: > I don't quite understand how "shell=True" worked before. For example: > > Opens up VIM with the file contents of "foo" and places the cursor at the > end of the document. > >> subprocess.check_call( ['vim', '+', '/home/m0/foo']) > > > While adding shell=True doesn't even load the file contents, just an empty > editor. > >> subprocess.check_call(['vim', '+', '/home/m0/foo'], shell=True) > > > That is not the effect that we needed. I verified if people set editor to > be "vim +" It needs to split it first. Unfortunately I am at work now so I > cannot submit any change now :( If any one of you can verify Mac and submit > a CL then I can review otherwise I have to wait till I come back home. > > Sorry :( > > > > > Kind regards, > Mohamed Mansour > > > > On Mon, Mar 14, 2011 at 10:04 AM, Marc-Antoine Ruel <maruel@chromium.org>wrote: > >> Main issue is when there is quotes in EDITOR variable content. Better to >> keep shell=True. >> Le 14 mars 2011 10:01, <mhm@chromium.org> a écrit : >> >> > >> > http://codereview.chromium.org/6680019/diff/1005/git_cl/git_cl.py >> > File git_cl/git_cl.py (right): >> > >> > >> http://codereview.chromium.org/6680019/diff/1005/git_cl/git_cl.py#newcode720 >> > git_cl/git_cl.py:720: subprocess.check_call(cmd) >> > Hi TVL, would changing cmd = [editor, filename] to "cmd = editor.split() >> > + [filename]" work ? >> > >> > http://codereview.chromium.org/6680019/ >> > > |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
