Use "svn cat" in tools/submit_try

Use "svn cat" in tools/submit_try

(SkipBuildbotRuns)
Committed: https://code.google.com/p/skia/source/detail?r=8197

[borenet, 2013-03-14 13:58:40]

Part 2 of 2.

[rmistry, 2013-03-14 14:28:21]

LGTM

[epoger, 2013-03-14 14:45:01]

Hold on, I have some suggestions coming...

[epoger, 2013-03-14 14:51:41]

I don't think this is ready to go...

https://codereview.chromium.org/12726006/diff/2001/tools/submit_try
File tools/submit_try (right):

https://codereview.chromium.org/12726006/diff/2001/tools/submit_try#newcode33
tools/submit_try:33: proc = subprocess.Popen([SVN, 'cat',
global_variables_file],
It seems silly to have this subprocess-launching boilerplate code pasted
multiple places in the file.  Instead, maybe extract it into a call_svn()
function that raises an exception in case of failure?

Taking it a step further, can we make use of
https://code.google.com/p/skia/source/browse/trunk/tools/svn.py instead?  Feel
free to improve that as necessary...

https://codereview.chromium.org/12726006/diff/2001/tools/submit_try#newcode40
tools/submit_try:40: return json.loads(global_variables)
should be json.load() instead of json.loads() ?

should be global_variables_file instead of global_variables?

[epoger, 2013-03-14 14:54:49]

https://codereview.chromium.org/12726006/diff/2001/tools/submit_try
File tools/submit_try (right):

https://codereview.chromium.org/12726006/diff/2001/tools/submit_try#newcode40
tools/submit_try:40: return json.loads(global_variables)
On 2013/03/14 14:51:41, epoger wrote:
> should be json.load() instead of json.loads() ?
> 
> should be global_variables_file instead of global_variables?

Oh, I see this part is actually fine.  Sorry about the false alarm.

[borenet, 2013-03-14 17:18:05]

Patch set 4.

https://codereview.chromium.org/12726006/diff/2001/tools/submit_try
File tools/submit_try (right):

https://codereview.chromium.org/12726006/diff/2001/tools/submit_try#newcode33
tools/submit_try:33: proc = subprocess.Popen([SVN, 'cat',
global_variables_file],
On 2013/03/14 14:51:41, epoger wrote:
> It seems silly to have this subprocess-launching boilerplate code pasted
> multiple places in the file.  Instead, maybe extract it into a call_svn()
> function that raises an exception in case of failure?
> 
> Taking it a step further, can we make use of
> https://code.google.com/p/skia/source/browse/trunk/tools/svn.py instead?  Feel
> free to improve that as necessary...

Done.

https://codereview.chromium.org/12726006/diff/2001/tools/submit_try#newcode40
tools/submit_try:40: return json.loads(global_variables)
On 2013/03/14 14:54:49, epoger wrote:
> On 2013/03/14 14:51:41, epoger wrote:
> > should be json.load() instead of json.loads() ?
> > 
> > should be global_variables_file instead of global_variables?
> 
> Oh, I see this part is actually fine.  Sorry about the false alarm.

Removed the whole function, since it shouldn't get called more than once anyway.

[epoger, 2013-03-14 17:48:32]

https://codereview.chromium.org/12726006/diff/12001/tools/submit_try
File tools/submit_try (right):

https://codereview.chromium.org/12726006/diff/12001/tools/submit_try#newcode74
tools/submit_try:74: cmd = [svn.SVN, 'info']
Given that the "info" command, unlike "svn cat", is dependent on the directory
within which it is run, I think you'd be better off using the svn.Svn object in
this case...

repo = svn.Svn('.')
output = repo.GetInfo()

You will have to add GetInfo() to svn.py, but that will be easy:

def GetInfo(self):
    return self._RunCommand([SVN, 'info'])

https://codereview.chromium.org/12726006/diff/12001/tools/svn.py
File tools/svn.py (right):

https://codereview.chromium.org/12726006/diff/12001/tools/svn.py#newcode23
tools/svn.py:23: SVN = 'svn.bat'
I think this is fine... although I think it worked before anyway, since Windows
will run "svn.bat" if you call "svn", anyway.

https://codereview.chromium.org/12726006/diff/12001/tools/svn.py#newcode151
tools/svn.py:151: def Cat(svn_url):
Maybe move this outside of the Svn class declaration, and then the caller can
just say "svn.Cat()" instead of "svn.Svn.Cat()"?

Given that this operates outside the context of an Svn object (which is tied to
a particular directory on local disk), I think that would be clearer.

But I don't feel particularly strongly about this.

[borenet, 2013-03-14 18:25:18]

Patch set 6.

https://codereview.chromium.org/12726006/diff/12001/tools/submit_try
File tools/submit_try (right):

https://codereview.chromium.org/12726006/diff/12001/tools/submit_try#newcode74
tools/submit_try:74: cmd = [svn.SVN, 'info']
On 2013/03/14 17:48:32, epoger wrote:
> Given that the "info" command, unlike "svn cat", is dependent on the directory
> within which it is run, I think you'd be better off using the svn.Svn object
in
> this case...
> 
> repo = svn.Svn('.')
> output = repo.GetInfo()
> 
> You will have to add GetInfo() to svn.py, but that will be easy:
> 
> def GetInfo(self):
>     return self._RunCommand([SVN, 'info'])

Done.

https://codereview.chromium.org/12726006/diff/12001/tools/svn.py
File tools/svn.py (right):

https://codereview.chromium.org/12726006/diff/12001/tools/svn.py#newcode23
tools/svn.py:23: SVN = 'svn.bat'
On 2013/03/14 17:48:32, epoger wrote:
> I think this is fine... although I think it worked before anyway, since
Windows
> will run "svn.bat" if you call "svn", anyway.

Actually, my experience with this is that that is the case when you run things
by hand, but shelling out from a python script is somehow different and you get
errors if you don't provide the extension.

https://codereview.chromium.org/12726006/diff/12001/tools/svn.py#newcode151
tools/svn.py:151: def Cat(svn_url):
On 2013/03/14 17:48:32, epoger wrote:
> Maybe move this outside of the Svn class declaration, and then the caller can
> just say "svn.Cat()" instead of "svn.Svn.Cat()"?
> 
> Given that this operates outside the context of an Svn object (which is tied
to
> a particular directory on local disk), I think that would be clearer.
> 
> But I don't feel particularly strongly about this.

Done.

[epoger, 2013-03-14 18:26:46]

LGTM at patchset 6!
Thanks for the cleanup!

https://codereview.chromium.org/12726006/diff/19001/tools/svn.py
File tools/svn.py (right):

https://codereview.chromium.org/12726006/diff/19001/tools/svn.py#newcode68
tools/svn.py:68: """Run "svn info" and return a dictionary containing its
output.
nice!

[borenet, 2013-03-18 18:18:37]

Committed patchset #6 manually as r8197 (presubmit successful).