Refactor source_control.py and add a test.

Refactor source_control.py and add a test.

Originally, Simon made SourceControl a class becaused we expected to
add a SvnSourceControl subclass which implemented the same methods in
different ways. However, after the git transition we're fairly sure
that we don't want to do this.

So, all of the functions in the SourceControl class can be made top-level
functions that don't need a "self" argument. This way, there's no need
to pass around a bogus SourceControl object; the functions can be called
directly.

I also added tests for some methods that seemed relatively testable.

PTAL and give suggestions :-)

BUG=

Committed: https://crrev.com/880741668d3ed0e08ea2229bb8965998abe775a3
Cr-Commit-Position: refs/heads/master@{#299851}

[qyearsley, 2014-10-14 17:48:34]

qyearsley@chromium.org changed reviewers:
+ ojan@chromium.org, prasadv@chromium.org

[qyearsley, 2014-10-14 17:48:35]

Hey guys, this CL is intended to be just a refactoring and adding of a test, but
the line change count is pretty large (partly because all lines in
source_control.py were un-indented and moved slightly).

https://codereview.chromium.org/650223005/diff/1/tools/auto_bisect/source_con...
File tools/auto_bisect/source_control.py (right):

https://codereview.chromium.org/650223005/diff/1/tools/auto_bisect/source_con...
tools/auto_bisect/source_control.py:16: return output.strip() == 'true'
This function is used to make the same check that was made when creating a
SourceControl object. Now in the main() method of bisect_perf_regression.py we
can check this before starting.

https://codereview.chromium.org/650223005/diff/1/tools/auto_bisect/source_con...
tools/auto_bisect/source_control.py:32: '--delete_unversioned_trees',
'--nohooks', '--revision', revision])
Sorry that the alignment in this diff is off -- most of these functions are
unchanged from how they were originally. SyncToRevisoinWithGClient starts on
line 39 in the "before" version.

https://codereview.chromium.org/650223005/diff/1/tools/auto_bisect/source_con...
tools/auto_bisect/source_control.py:84: search, cwd=None):
This is the only function in here that I made a non-trivial change to. I split
the CrOS-related part out and added a comment.

[ojan, 2014-10-14 22:59:09]

lgtm

FYI, I'll be working on this stuff less now. I think this work should continue
as a high priority, but it probably won't be as part of the code yellow (the
"tree open" part of which will be winding down). The point is, I won't be
available to do reviews in the future.

https://codereview.chromium.org/650223005/diff/1/tools/auto_bisect/source_con...
File tools/auto_bisect/source_control.py (right):

https://codereview.chromium.org/650223005/diff/1/tools/auto_bisect/source_con...
tools/auto_bisect/source_control.py:61: """Syncs to the specified revision.
IMO, docstring comments like this should be removed. They don't tell me useful
information. They just say what the code obviously does and get out of sync with
the code (e.g. in this one, the argument name is wrong).

[qyearsley, 2014-10-14 23:14:55]

Alright. I mainly added you as a reviewer because you had some suggestions
before.

In any case, in the future I'll keep in mind the principle of avoiding
unnecessary comments. :-)

https://codereview.chromium.org/650223005/diff/1/tools/auto_bisect/source_con...
File tools/auto_bisect/source_control.py (right):

https://codereview.chromium.org/650223005/diff/1/tools/auto_bisect/source_con...
tools/auto_bisect/source_control.py:61: """Syncs to the specified revision.
On 2014/10/14 22:59:09, ojan-only-code-yellow-reviews wrote:
> IMO, docstring comments like this should be removed. They don't tell me useful
> information. They just say what the code obviously does and get out of sync
with
> the code (e.g. in this one, the argument name is wrong).

Ah, you're right. Docstring removed.

In some other cases, the first sentence of the docstring doesn't add much, but
the other parts (details about the arguments and return value) do explain
something. In that case, would you still add a first line?


Note, here's what the Google python style guide says on the subject:

A function must have a docstring, unless it meets all of the following criteria:
 * not externally visible
 * very short
 * obvious
A docstring should give enough information to write a call to the function
without reading the function's code...

https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Comme...

[ojan, 2014-10-14 23:26:15]

On 2014/10/14 at 23:14:55, qyearsley wrote:
> Alright. I mainly added you as a reviewer because you had some suggestions
before.
> 
> In any case, in the future I'll keep in mind the principle of avoiding
unnecessary comments. :-)
> 
>
https://codereview.chromium.org/650223005/diff/1/tools/auto_bisect/source_con...
> File tools/auto_bisect/source_control.py (right):
> 
>
https://codereview.chromium.org/650223005/diff/1/tools/auto_bisect/source_con...
> tools/auto_bisect/source_control.py:61: """Syncs to the specified revision.
> On 2014/10/14 22:59:09, ojan-only-code-yellow-reviews wrote:
> > IMO, docstring comments like this should be removed. They don't tell me
useful
> > information. They just say what the code obviously does and get out of sync
with
> > the code (e.g. in this one, the argument name is wrong).
> 
> Ah, you're right. Docstring removed.
> 
> In some other cases, the first sentence of the docstring doesn't add much, but
the other parts (details about the arguments and return value) do explain
something. In that case, would you still add a first line?
> 
> 
> Note, here's what the Google python style guide says on the subject:
> 
> A function must have a docstring, unless it meets all of the following
criteria:
>  * not externally visible
>  * very short
>  * obvious
> A docstring should give enough information to write a call to the function
without reading the function's code...

Yeah, I guess I don't really agree with that. There's a discussion on the infra
team mailing list about this where there appears to be disagreement. So, feel
free to ignore me. :) The externally visible part is what I disagree with. If
the docstring doesn't tell you anything that the function name + argument names
already tell you then you shouldn't write it even if it's externally visible.
Also, I think I'd take a different interpretation of "obvious" than the authors
of that style guide might.

> 
>
https://google-styleguide.googlecode.com/svn/trunk/pyguide.html?showone=Comme...

[prasadv, 2014-10-16 00:14:59]

lgtm

[qyearsley, 2014-10-16 05:08:03]

The CQ bit was checked by qyearsley@chromium.org

[commit-bot: I haz the power, 2014-10-16 05:08:10]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/650223005/40001

[commit-bot: I haz the power, 2014-10-16 05:31:28]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2014-10-16 05:32:17]

Patchset 3 (id:??) landed as
https://crrev.com/880741668d3ed0e08ea2229bb8965998abe775a3
Cr-Commit-Position: refs/heads/master@{#299851}

[prasadv, 2014-10-20 17:11:33]

A revert of this CL (patchset #3 id:40001) has been created in
https://codereview.chromium.org/661803003/ by prasadv@chromium.org.

The reason for reverting is: This broke syncing source for dependency repos
while bisecting them, 
http://build.chromium.org/p/tryserver.chromium.perf/builders/android_nexus7_p....