Reland - Support msysgit in gcl and git-cl...

Reland - 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

[Mohamed Mansour, 2011-03-12 18:21:34]

This change ensures "env" only present in msysgit.

Verified in:
- Windows XP (new VM)
- Windows 7 64bit (console)
- Windows 7 64bit (msysgit)
- Linux (ubuntu)

[M-A Ruel, 2011-03-12 18:27:28]

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

[Mohamed Mansour, 2011-03-12 18:36:57]

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.

[M-A Ruel, 2011-03-12 18:53:54]

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

[TVL, 2011-03-14 13:31:54]

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.

[TVL, 2011-03-14 13:42:16]

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.

[Mohamed Mansour, 2011-03-14 14:01:33]

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 ?

[M-A Ruel, 2011-03-14 14:04:53]

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/

[Mohamed Mansour, 2011-03-14 14:16:03]

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/
>

[TVL, 2011-03-14 14:38:40]

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/
>>
>
>