Replace gcl_try with submit_try

Replace gcl_try with submit_try

submit_try supports both git and svn and hooks directly into depot_tools rather
than calling it in a subprocess.
Committed: https://code.google.com/p/skia/source/detail?r=7352

[EricB, 2013-01-23 18:22:36]

This is mostly additions to gcl_try, with the exception of the modifications to
SubmitTryRequest.

[rmistry, 2013-01-23 18:30:34]

LGTM

https://codereview.appspot.com/7202044/diff/1/tools/submit_try
File tools/submit_try (right):

https://codereview.appspot.com/7202044/diff/1/tools/submit_try#newcode1
tools/submit_try:1: #!/usr/bin/python
This file looks like it includes gcl_try with some additions. The additions will
be easier to see if you do a 'svn move gcl_try submit_try' (I would copy
submit_try somewhere then do the svn move and then copy from your tmp location
back to submit_try).

[rmistry, 2013-01-23 18:35:29]

Err I did not mean to LGTM yet

On 2013/01/23 18:30:34, rmistry wrote:
> LGTM
> 
> https://codereview.appspot.com/7202044/diff/1/tools/submit_try
> File tools/submit_try (right):
> 
> https://codereview.appspot.com/7202044/diff/1/tools/submit_try#newcode1
> tools/submit_try:1: #!/usr/bin/python
> This file looks like it includes gcl_try with some additions. The additions
will
> be easier to see if you do a 'svn move gcl_try submit_try' (I would copy
> submit_try somewhere then do the svn move and then copy from your tmp location
> back to submit_try).

[EricB, 2013-01-23 18:36:22]

Patch set 2. Indeed, that does make it easier to read.

[rmistry, 2013-01-23 18:47:57]

https://codereview.appspot.com/7202044/diff/4/tools/submit_try
File tools/submit_try (right):

https://codereview.appspot.com/7202044/diff/4/tools/submit_try#newcode35
tools/submit_try:35: which = 'where'
might be clearer if it was called which_cmd

https://codereview.appspot.com/7202044/diff/4/tools/submit_try#newcode37
tools/submit_try:37: which = 'which'
optional:

which_cmd = 'where' if os.name == 'nt' else 'which'

https://codereview.appspot.com/7202044/diff/4/tools/submit_try#newcode62
tools/submit_try:62: URL_STR = 'URL: '
dont need to all caps in the current scope, only at the module level.

https://codereview.appspot.com/7202044/diff/4/tools/submit_try#newcode92
tools/submit_try:92: 'defined in the codereview.settings file.')
Make sure it is defined in the %s file. % CODEREVIEW_SETTINGS

https://codereview.appspot.com/7202044/diff/4/tools/submit_try#newcode102
tools/submit_try:102: 'GET')
This indentation looks a little off, maybe move this one line up?

https://codereview.appspot.com/7202044/diff/4/tools/submit_try#newcode145
tools/submit_try:145: args: command-line arguments.
Since you are listing the type of other parameters maybe list the type of args
as well?

[EricB, 2013-01-23 19:06:30]

Patch set 4.

https://codereview.appspot.com/7202044/diff/4/tools/submit_try
File tools/submit_try (right):

https://codereview.appspot.com/7202044/diff/4/tools/submit_try#newcode35
tools/submit_try:35: which = 'where'
On 2013/01/23 18:47:57, rmistry wrote:
> might be clearer if it was called which_cmd

Done.

https://codereview.appspot.com/7202044/diff/4/tools/submit_try#newcode37
tools/submit_try:37: which = 'which'
On 2013/01/23 18:47:57, rmistry wrote:
> optional:
> 
> which_cmd = 'where' if os.name == 'nt' else 'which'

Done.

https://codereview.appspot.com/7202044/diff/4/tools/submit_try#newcode62
tools/submit_try:62: URL_STR = 'URL: '
On 2013/01/23 18:47:57, rmistry wrote:
> dont need to all caps in the current scope, only at the module level.

Moved to module level, added explanatory comments.

https://codereview.appspot.com/7202044/diff/4/tools/submit_try#newcode92
tools/submit_try:92: 'defined in the codereview.settings file.')
On 2013/01/23 18:47:57, rmistry wrote:
> Make sure it is defined in the %s file. % CODEREVIEW_SETTINGS

Done.

https://codereview.appspot.com/7202044/diff/4/tools/submit_try#newcode102
tools/submit_try:102: 'GET')
On 2013/01/23 18:47:57, rmistry wrote:
> This indentation looks a little off, maybe move this one line up?

Done.

https://codereview.appspot.com/7202044/diff/4/tools/submit_try#newcode145
tools/submit_try:145: args: command-line arguments.
On 2013/01/23 18:47:57, rmistry wrote:
> Since you are listing the type of other parameters maybe list the type of args
> as well?

Done.  I tried to explain the properties that the object should have.

[rmistry, 2013-01-23 19:41:07]

https://codereview.appspot.com/7202044/diff/8002/tools/submit_try
File tools/submit_try (right):

https://codereview.appspot.com/7202044/diff/8002/tools/submit_try#newcode16
tools/submit_try:16: import argparse
I just noticed that this is new in version 3.2, I could not access it in my
locally installed python (2.6.5). Should we consider using an older built-in
module instead?

https://codereview.appspot.com/7202044/diff/8002/tools/submit_try#newcode28
tools/submit_try:28: # TODO(borenet): Share this information from a single
location.
Put in a link to http://code.google.com/p/skia/issues/detail?id=1081 ?

https://codereview.appspot.com/7202044/diff/8002/tools/submit_try#newcode80
tools/submit_try:80: if len(url) == len(repo_root):
Can you explain this line and why we return 'svn' below?

[EricB, 2013-01-23 20:35:25]

Patch set 5

https://codereview.appspot.com/7202044/diff/8002/tools/submit_try
File tools/submit_try (right):

https://codereview.appspot.com/7202044/diff/8002/tools/submit_try#newcode16
tools/submit_try:16: import argparse
On 2013/01/23 19:41:07, rmistry wrote:
> I just noticed that this is new in version 3.2, I could not access it in my
> locally installed python (2.6.5). Should we consider using an older built-in
> module instead?

That's interesting.  I'm using 2.7 on my machine.  I must have the module
installed in addition to the standard set.  I think httplib will suffice.

https://codereview.appspot.com/7202044/diff/8002/tools/submit_try#newcode28
tools/submit_try:28: # TODO(borenet): Share this information from a single
location.
On 2013/01/23 19:41:07, rmistry wrote:
> Put in a link to http://code.google.com/p/skia/issues/detail?id=1081 ?

Done.

https://codereview.appspot.com/7202044/diff/8002/tools/submit_try#newcode80
tools/submit_try:80: if len(url) == len(repo_root):
On 2013/01/23 19:41:07, rmistry wrote:
> Can you explain this line and why we return 'svn' below?

"root" is where the current checkout is based.  Most devs check out
http://skia.googlecode.com/svn/trunk, but it is also possible to check out at
http://skia.googlecode.com/svn, in which case the method at line 82 will just
give an empty string.  The ApplyPatch buildstep needs "root" to know where to go
before applying the patch.  "svn" is special-cased as the root of the checkout.

[rmistry, 2013-01-23 20:46:59]

LGTM

https://codereview.appspot.com/7202044/diff/8002/tools/submit_try
File tools/submit_try (right):

https://codereview.appspot.com/7202044/diff/8002/tools/submit_try#newcode80
tools/submit_try:80: if len(url) == len(repo_root):
On 2013/01/23 20:35:25, EricB wrote:
> On 2013/01/23 19:41:07, rmistry wrote:
> > Can you explain this line and why we return 'svn' below?
> 
> "root" is where the current checkout is based.  Most devs check out
> http://skia.googlecode.com/svn/trunk, but it is also possible to check out at
> http://skia.googlecode.com/svn, in which case the method at line 82 will just
> give an empty string.  The ApplyPatch buildstep needs "root" to know where to
go
> before applying the patch.  "svn" is special-cased as the root of the
checkout.

In that case will url == repo_root work as well?

[EricB, 2013-01-23 20:54:39]

Patch set 6 committed as r7352.

https://codereview.appspot.com/7202044/diff/8002/tools/submit_try
File tools/submit_try (right):

https://codereview.appspot.com/7202044/diff/8002/tools/submit_try#newcode80
tools/submit_try:80: if len(url) == len(repo_root):
On 2013/01/23 20:46:59, rmistry wrote:
> On 2013/01/23 20:35:25, EricB wrote:
> > On 2013/01/23 19:41:07, rmistry wrote:
> > > Can you explain this line and why we return 'svn' below?
> > 
> > "root" is where the current checkout is based.  Most devs check out
> > http://skia.googlecode.com/svn/trunk, but it is also possible to check out
at
> > http://skia.googlecode.com/svn, in which case the method at line 82 will
just
> > give an empty string.  The ApplyPatch buildstep needs "root" to know where
to
> go
> > before applying the patch.  "svn" is special-cased as the root of the
> checkout.
> 
> In that case will url == repo_root work as well?

Yes. Somehow I missed that obvious fact. I changed it since that's clearer.