Try to get current CL number if none given in rebaseline-from-try-jobs.

Try to get current CL number if none given in rebaseline-from-try-jobs.

BUG=474273
Committed: https://crrev.com/db71ae4ef1ed37fcbf93de993ad4d4fcf8843788
Cr-Commit-Position: refs/heads/master@{#405014}

[qyearsley, 2016-07-12 23:46:48]

qyearsley@chromium.org changed reviewers:
+ dcampb@google.com, dpranke@chromium.org, wkorman@chromium.org

[wkorman, 2016-07-12 23:54:53]

lgtm

https://codereview.chromium.org/2144843003/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs.py
(right):

https://codereview.chromium.org/2144843003/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs.py:45:
issue_number = options.issue
Maybe move all of this into separate:

def get_issue_number(self, options)

just to make execute() easier to read.

Also only actually need Git() instance in that method, and only if not in
options. It does look like there's a bit of filesystem activity on create via:

https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...

so could actually be worth avoiding construction unless needed.

dpranke@ had mentioned there was a general goal in webkitpy to move away from
scm-specific logic, but what we're doing here seems useful/important. But may
want to ping him to ask before submit.

[Dirk Pranke, 2016-07-13 00:52:07]

lgtm

https://codereview.chromium.org/2144843003/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs.py
(right):

https://codereview.chromium.org/2144843003/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs.py:45:
issue_number = options.issue
On 2016/07/12 23:54:53, wkorman wrote:
> Maybe move all of this into separate:
> 
> def get_issue_number(self, options)
> 
> just to make execute() easier to read.
> 
> Also only actually need Git() instance in that method, and only if not in
> options. It does look like there's a bit of filesystem activity on create via:
> 
>
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> 
> so could actually be worth avoiding construction unless needed.
> 
> dpranke@ had mentioned there was a general goal in webkitpy to move away from
> scm-specific logic, but what we're doing here seems useful/important. But may
> want to ping him to ask before submit.

Getting the issue number from git needs to be done locally, so this is correct.

[qyearsley, 2016-07-13 02:26:28]

https://codereview.chromium.org/2144843003/diff/40001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs.py
(right):

https://codereview.chromium.org/2144843003/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs.py:45:
issue_number = options.issue
On 2016/07/13 at 00:52:06, Dirk Pranke wrote:
> On 2016/07/12 23:54:53, wkorman wrote:
> > Maybe move all of this into separate:
> > 
> > def get_issue_number(self, options)
> > 
> > just to make execute() easier to read.
> > 
> > Also only actually need Git() instance in that method, and only if not in
> > options. It does look like there's a bit of filesystem activity on create
via:
> > 
> >
https://cs.chromium.org/chromium/src/third_party/WebKit/Tools/Scripts/webkitp...
> > 
> > so could actually be worth avoiding construction unless needed.
> > 
> > dpranke@ had mentioned there was a general goal in webkitpy to move away
from
> > scm-specific logic, but what we're doing here seems useful/important. But
may
> > want to ping him to ask before submit.
> 
> Getting the issue number from git needs to be done locally, so this is
correct.

Now extracted a method called _get_issue_number from execute, and changed it to
construct a Git instance only when no issue number is passed.

https://codereview.chromium.org/2144843003/diff/60001/third_party/WebKit/Tool...
File
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs.py
(right):

https://codereview.chromium.org/2144843003/diff/60001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/tool/commands/rebaseline_from_try_jobs.py:75:
return Git(cwd=self._tool.filesystem.dirname(self._tool.path()))
Also changed the cwd argument here so that this command still works even if
webkit-patch is invoked from outside the repository.

[qyearsley, 2016-07-13 02:52:29]

The CQ bit was checked by qyearsley@chromium.org

[qyearsley, 2016-07-13 02:52:29]

The patchset sent to the CQ was uploaded after l-g-t-m from
dpranke@chromium.org, wkorman@chromium.org
Link to the patchset: https://codereview.chromium.org/2144843003/#ps60001
(title: "Always use a directory inside the repo as cwd.")

[commit-bot: I haz the power, 2016-07-13 02:54:05]

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, 2016-07-13 04:21:33]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-07-13 04:21:37]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-07-13 04:23:45]

Description was changed from

==========
Try to get current CL number if none given in rebaseline-from-try-jobs.

BUG=474273
==========

to

==========
Try to get current CL number if none given in rebaseline-from-try-jobs.

BUG=474273

Committed: https://crrev.com/db71ae4ef1ed37fcbf93de993ad4d4fcf8843788
Cr-Commit-Position: refs/heads/master@{#405014}
==========

[commit-bot: I haz the power, 2016-07-13 04:23:47]

Patchset 4 (id:??) landed as
https://crrev.com/db71ae4ef1ed37fcbf93de993ad4d4fcf8843788
Cr-Commit-Position: refs/heads/master@{#405014}