Simplify the initialization of Git objects in Host.

Simplify the initialization of Git objects in Host.

This CL makes calling Host.initialize_scm() unnecessary by making
it so that the Git object is initialized when required. It also
merges scm() and scm_for_path(), and moves initialization logic
into the Git class.

BUG=676012
Review-Url: https://codereview.chromium.org/2663623003
Cr-Commit-Position: refs/heads/master@{#447582}
Committed: https://chromium.googlesource.com/chromium/src/+/7091f37e5b763d57e4752a4e69cf46719b8ea1c4

[qyearsley, 2017-01-28 21:52:31]

The CQ bit was checked by qyearsley@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-01-28 21:52:35]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[qyearsley, 2017-01-28 21:52:48]

qyearsley@chromium.org changed reviewers:
+ jeffcarp@chromium.org

[commit-bot: I haz the power, 2017-01-28 23:13:53]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-01-28 23:13:54]

Dry run: This issue passed the CQ dry run.

[jeffcarp, 2017-01-31 22:24:56]

https://codereview.chromium.org/2663623003/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py
(right):

https://codereview.chromium.org/2663623003/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py:84: # We
hack the git.bat name into the SVN class.
The 'SVN class' reference is no longer accurate (was it even accurate in the
previous version?)

https://codereview.chromium.org/2663623003/diff/1/third_party/WebKit/Tools/Sc...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py
(right):

https://codereview.chromium.org/2663623003/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py:344:
scm = port_obj.host.scm(path=path)
Some invocations use path= and some don't, would be good for this CL to make
them consistent

https://codereview.chromium.org/2663623003/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/bot/commit_announcer.py
(right):

https://codereview.chromium.org/2663623003/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/tool/bot/commit_announcer.py:53:
self.git = tool.scm(tool.scm().checkout_root)
Was wondering why this was named tool, not host, and discovered that WebKitPatch
inherits from host, which seems like a weird pattern (no action needed, just
weird)
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

[qyearsley, 2017-02-01 00:58:48]

https://codereview.chromium.org/2663623003/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py
(right):

https://codereview.chromium.org/2663623003/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py:84: # We
hack the git.bat name into the SVN class.
On 2017/01/31 at 22:24:56, jeffcarp wrote:
> The 'SVN class' reference is no longer accurate (was it even accurate in the
previous version?)

Gah, I'm not sure, I think it might originally have been a mistake. Now
adjusted. I could also potentially remove this second paragraph?

https://codereview.chromium.org/2663623003/diff/1/third_party/WebKit/Tools/Sc...
File
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py
(right):

https://codereview.chromium.org/2663623003/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/layout_tests/models/test_run_results.py:344:
scm = port_obj.host.scm(path=path)
On 2017/01/31 at 22:24:56, jeffcarp wrote:
> Some invocations use path= and some don't, would be good for this CL to make
them consistent

Done

https://codereview.chromium.org/2663623003/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/tool/bot/commit_announcer.py
(right):

https://codereview.chromium.org/2663623003/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/tool/bot/commit_announcer.py:53:
self.git = tool.scm(tool.scm().checkout_root)
On 2017/01/31 at 22:24:56, jeffcarp wrote:
> Was wondering why this was named tool, not host, and discovered that
WebKitPatch inherits from host, which seems like a weird pattern (no action
needed, just weird)
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

That's right, it is weird :-/ It would probably make more sense if WebKitPatch
had a Host member rather than being a subclass of Host.

There are probably other refactorings that we should do first, but I added a
FIXME note anyway for now. Does that seem OK?

[jeffcarp, 2017-02-01 17:27:03]

Thanks for updating, lgtm with one knit

https://codereview.chromium.org/2663623003/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py
(right):

https://codereview.chromium.org/2663623003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py:84: # we
set the executable name on a class variable.
Nit: I think this should be 'instance variable' not class variable

[qyearsley, 2017-02-01 17:46:13]

https://codereview.chromium.org/2663623003/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py
(right):

https://codereview.chromium.org/2663623003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py:84: # we
set the executable name on a class variable.
On 2017/02/01 at 17:27:03, jeffcarp wrote:
> Nit: I think this should be 'instance variable' not class variable

You're right, it's set here as an instance variable, although it was originally
set on the class object on line 49.

Messing around with this a bit:

>>> class Foo(object):
...   x = 1
...   def set_x(self, new_x):
...     self.x = new_x
... 
>>> Foo.x
1
>>> a = Foo()
>>> a.x
1
>>> a.set_x(20)
>>> a.x
20
>>> Foo.x
1

But actually, I'm not sure I really understand the second paragraph here -- I've
read we're not supposed to use shell=True since there's some security issue, and
I guess that saying that subprocess.Popen detects the difference between git and
git.bat means that just "git" doesn't work on windows and won't use git.bat, so
we want to explicitly invoke git with "git.bat".

In the latest patchset I removed the second paragraph; does that seem OK?

[jeffcarp, 2017-02-01 18:27:55]

https://codereview.chromium.org/2663623003/diff/20001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py
(right):

https://codereview.chromium.org/2663623003/diff/20001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/scm/git.py:84: # we
set the executable name on a class variable.
On 2017/02/01 at 17:46:13, qyearsley wrote:
> On 2017/02/01 at 17:27:03, jeffcarp wrote:
> > Nit: I think this should be 'instance variable' not class variable
> 
> You're right, it's set here as an instance variable, although it was
originally set on the class object on line 49.
> 
> Messing around with this a bit:
> 
> >>> class Foo(object):
> ...   x = 1
> ...   def set_x(self, new_x):
> ...     self.x = new_x
> ... 
> >>> Foo.x
> 1
> >>> a = Foo()
> >>> a.x
> 1
> >>> a.set_x(20)
> >>> a.x
> 20
> >>> Foo.x
> 1
> 
> But actually, I'm not sure I really understand the second paragraph here --
I've read we're not supposed to use shell=True since there's some security
issue, and I guess that saying that subprocess.Popen detects the difference
between git and git.bat means that just "git" doesn't work on windows and won't
use git.bat, so we want to explicitly invoke git with "git.bat".
> 
> In the latest patchset I removed the second paragraph; does that seem OK?

👍

[qyearsley, 2017-02-01 18:30:34]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2017-02-01 18:30:35]

The patchset sent to the CQ was uploaded after l-g-t-m from
jeffcarp@chromium.org
Link to the patchset: https://codereview.chromium.org/2663623003/#ps40001
(title: "Remove second paragraph in comment about awesome windows git hack")

[commit-bot: I haz the power, 2017-02-01 18:31:01]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-02-01 19:44:38]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1485973834735730,
"parent_rev": "6b1918d42ca66325641b8b121c245edab5ffa5c5", "commit_rev":
"7091f37e5b763d57e4752a4e69cf46719b8ea1c4"}

[commit-bot: I haz the power, 2017-02-01 19:45:10]

Description was changed from

==========
Simplify the initialization of Git objects in Host.

This CL makes calling Host.initialize_scm() unnecessary by making
it so that the Git object is initialized when required. It also
merges scm() and scm_for_path(), and moves initialization logic
into the Git class.

BUG=676012
==========

to

==========
Simplify the initialization of Git objects in Host.

This CL makes calling Host.initialize_scm() unnecessary by making
it so that the Git object is initialized when required. It also
merges scm() and scm_for_path(), and moves initialization logic
into the Git class.

BUG=676012

Review-Url: https://codereview.chromium.org/2663623003
Cr-Commit-Position: refs/heads/master@{#447582}
Committed:
https://chromium.googlesource.com/chromium/src/+/7091f37e5b763d57e4752a4e69cf...
==========

[commit-bot: I haz the power, 2017-02-01 19:45:11]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/7091f37e5b763d57e4752a4e69cf...

[lushnikov, 2017-02-02 00:08:22]

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

The reason for reverting is: Speculative revert. This might be the cause of
build flakiness, see crbug.com/687707.