Refactor and improve bug line generation for push-to-trunk.

Refactor and improve bug line generation for push-to-trunk.


BUG=
R=jkummerow@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=17892

[Michael Achenbach, 2013-11-18 17:54:59]

PTAL

[Jakob Kummerow, 2013-11-19 13:39:29]

First round of comments. I haven't looked at the tests in much detail yet; I'll
do that on the next patch set.

https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_in...
File tools/push-to-trunk/common_includes.py (right):

https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_in...
tools/push-to-trunk/common_includes.py:102: out = filter(lambda x:
re.search(r"^[ \t]*BUG[ \t]*=", x), out)
I don't think we need any of these three "filter" lines any more. AddIssues()
below should handle all of that anyway.

https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_in...
tools/push-to-trunk/common_includes.py:109: def AddSafe(bugs, bug):
I think you don't need this function at all, if below in the regexes you match
against (\d+) instead of (.*).

https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_in...
tools/push-to-trunk/common_includes.py:116: ref = re.match(r"^[ \t]*BUG[ \t]*=[
\t]*(.*?)[ \t]*$", text)
I think the regex can be simplified to:

r"^[ \t]*BUG=(.*?)$"

Reasons:
- whitespace around '=' doesn't happen in practice
- whitespace at the end would be strip()'ed away below anyway.
- the simpler a regex, the better :-)

https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_in...
tools/push-to-trunk/common_includes.py:121: match = re.match(r"^v8[ \t]*:[
\t]*(.*)$", bug)
similarly, we don't need to support whitespace around ':'

https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_in...
tools/push-to-trunk/common_includes.py:141: FormatIssues("", v8bugs)
V8 bugs first, please :-)

[Michael Achenbach, 2013-11-19 15:02:57]

PTAL at patch 2.

https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_in...
File tools/push-to-trunk/common_includes.py (right):

https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_in...
tools/push-to-trunk/common_includes.py:102: out = filter(lambda x:
re.search(r"^[ \t]*BUG[ \t]*=", x), out)
On 2013/11/19 13:39:29, Jakob wrote:
> I don't think we need any of these three "filter" lines any more. AddIssues()
> below should handle all of that anyway.

Done.

https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_in...
tools/push-to-trunk/common_includes.py:109: def AddSafe(bugs, bug):
On 2013/11/19 13:39:29, Jakob wrote:
> I think you don't need this function at all, if below in the regexes you match
> against (\d+) instead of (.*).

Done.

https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_in...
tools/push-to-trunk/common_includes.py:116: ref = re.match(r"^[ \t]*BUG[ \t]*=[
\t]*(.*?)[ \t]*$", text)
Regarding the inner whitespace I had
https://code.google.com/p/chromium/codesearch#chromium/tools/depot_tools/git_...
as reference. That's what is allowed when depot tools use it.

Agreed on the outer whitespace. Added a strip.

https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_in...
tools/push-to-trunk/common_includes.py:121: match = re.match(r"^v8[ \t]*:[
\t]*(.*)$", bug)
On 2013/11/19 13:39:29, Jakob wrote:
> similarly, we don't need to support whitespace around ':'

Done.

https://codereview.chromium.org/65933003/diff/1/tools/push-to-trunk/common_in...
tools/push-to-trunk/common_includes.py:141: FormatIssues("", v8bugs)
On 2013/11/19 13:39:29, Jakob wrote:
> V8 bugs first, please :-)

Done. That required some test changes...

[Jakob Kummerow, 2013-11-19 16:20:27]

lgtm

[Michael Achenbach, 2013-11-20 08:25:23]

Committed patchset #2 manually as r17892.