Skip commits that don't generate a patch + fixes to get export working

Skip commits that don't generate a patch + fixes to get export working

More specifically, this CL:
- Modifies ChromiumCommit.format_patch so that it:
  1. First gets a list of files changed
  2. Filters blacklisted files (e.g. MANIFEST.json) from that list
  3. Passes the new list of files to git format-patch
- Adds LocalWPT.test_patch, which applies a patch to WPT master and returns the diff
  - This is to guard against changes like https://crrev.com/2488283003
- Fixes a number of things that broke in the export process


Most of this code was written in response to something breaking, so I apologize in advance that it is not super elegant.

BUG=657117, 671890

[jeffcarp, 2016-12-02 00:25:01]

Description was changed from

==========
Skip commits that don't generate a patch + fixes to get export working

BUG=657117
==========

to

==========
Skip commits that don't generate a patch + fixes to get export working

More specifically, this CL:
- Modifies ChromiumCommit.format_patch so that it:
  1. First gets a list of files changed
  2. Filters blacklisted files (e.g. MANIFEST.json) from that list
  3. Passes the new list of files to git format-patch
- Adds LocalWPT.test_patch, which applies a patch to WPT master and returns the
diff
  - This is to guard against changes like https://crrev.com/2488283003
- Fixes a number of things that broke in the export process


Most of this code was written in response to something breaking, so I apologize
in advance that it is not super elegant.

BUG=657117
==========

[jeffcarp, 2016-12-02 00:25:01]

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

[qyearsley, 2016-12-02 00:38:46]

Just a few comments for now - I think the main thing that could improve this CL
would be to add tests for any non-trivial new logic.

https://codereview.chromium.org/2544173002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right):

https://codereview.chromium.org/2544173002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:113: def
test_patch(self, patch):
A docstring may be helpful here.

https://codereview.chromium.org/2544173002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:120: print patch
Are these meant to be left in? If so, they should probably be logging statements
rather than prints.

https://codereview.chromium.org/2544173002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py (right):

https://codereview.chromium.org/2544173002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py:39: pr_branch_name =
local_branch_name
Why not just rename the argument to pr_branch_name, or use local_branch_name
directly below?

https://codereview.chromium.org/2544173002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py:65: path =
'/search/issues?q=repo:w3c/web-platform-tests%20is:open%20type:pr%20label:{}'.format(url_encoded_label)
For URL-encoding query strings, you can also use urllib.urlencode:

  >>> import urllib
  >>> urllib.urlencode({'q': 'foo bar<>'})
  'q=foo+bar%3C%3E'

[jeffcarp, 2016-12-02 23:55:17]

Addressed qyearsley's feedback. I have a couple more things I can clean up too,
going to work on that.

https://codereview.chromium.org/2544173002/diff/1/third_party/WebKit/Tools/Sc...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py (right):

https://codereview.chromium.org/2544173002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py:39: pr_branch_name =
local_branch_name
On 2016/12/02 at 00:38:46, qyearsley wrote:
> Why not just rename the argument to pr_branch_name, or use local_branch_name
directly below?

Oops, left that in there from deleting some other code.

https://codereview.chromium.org/2544173002/diff/1/third_party/WebKit/Tools/Sc...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py:65: path =
'/search/issues?q=repo:w3c/web-platform-tests%20is:open%20type:pr%20label:{}'.format(url_encoded_label)
On 2016/12/02 at 00:38:46, qyearsley wrote:
> For URL-encoding query strings, you can also use urllib.urlencode:
> 
>   >>> import urllib
>   >>> urllib.urlencode({'q': 'foo bar<>'})
>   'q=foo+bar%3C%3E'

Sounds good. At the moment the label we're using is `chromium-export`, which
shouldn't need to be urlencoded altogether, so I'm going to remove the line
above.

[jeffcarp, 2016-12-07 02:11:37]

On 2016/12/02 at 23:55:17, jeffcarp wrote:
> Addressed qyearsley's feedback. I have a couple more things I can clean up
too, going to work on that.
> 
>
https://codereview.chromium.org/2544173002/diff/1/third_party/WebKit/Tools/Sc...
> File third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py (right):
> 
>
https://codereview.chromium.org/2544173002/diff/1/third_party/WebKit/Tools/Sc...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py:39: pr_branch_name
= local_branch_name
> On 2016/12/02 at 00:38:46, qyearsley wrote:
> > Why not just rename the argument to pr_branch_name, or use local_branch_name
directly below?
> 
> Oops, left that in there from deleting some other code.
> 
>
https://codereview.chromium.org/2544173002/diff/1/third_party/WebKit/Tools/Sc...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/wpt_github.py:65: path =
'/search/issues?q=repo:w3c/web-platform-tests%20is:open%20type:pr%20label:{}'.format(url_encoded_label)
> On 2016/12/02 at 00:38:46, qyearsley wrote:
> > For URL-encoding query strings, you can also use urllib.urlencode:
> > 
> >   >>> import urllib
> >   >>> urllib.urlencode({'q': 'foo bar<>'})
> >   'q=foo+bar%3C%3E'
> 
> Sounds good. At the moment the label we're using is `chromium-export`, which
shouldn't need to be urlencoded altogether, so I'm going to remove the line
above.

Feedback has been addressed (bumping this CL since it's a new week)

[qyearsley, 2016-12-07 19:05:36]

Didn't see any problems; just have a few more comments.

https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py (right):

https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/chromium_commit.py:69: # Get list
of changed files
Nit: Add periods at the end of comments that are full sentences or full verb
phrases like this, for consistency.

https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py (right):

https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/local_wpt.py:130: output = ''
This could be output with _log.error instead of print, e.g.

    _log.error('Error while applying patch: %s', error)

https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right):

https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:44:
_log.info('[dry_run] Would have attempted to merge PR')
Optional minor rephrasing: "[dry run] Would attempt to merge PR."

https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:53: """
Possible rephrasing:

Creates a PR for the earliest exportable commit in Chromium if there is one.

Note that this also makes the request to github to create the PR, right? And it
always returns None?

https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:88: return
Note: we'll want to extract a separate function to get the list of exportable
commits since the last export; we could do that in this CL or another one if it
would be cleaner.

[jeffcarp, 2016-12-07 20:24:38]

Addressed qyearsley's CL feedback.

https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right):

https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:88: return
On 2016/12/07 at 19:05:36, qyearsley wrote:
> Note: we'll want to extract a separate function to get the list of exportable
commits since the last export; we could do that in this CL or another one if it
would be cleaner.

I'd prefer landing this on master and doing it in its own CL if you're ok with
that.

[jeffcarp, 2016-12-07 20:27:19]

On 2016/12/07 at 20:24:38, jeffcarp wrote:
> Addressed qyearsley's CL feedback.
> 
>
https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
> File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right):
> 
>
https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:88: return
> On 2016/12/07 at 19:05:36, qyearsley wrote:
> > Note: we'll want to extract a separate function to get the list of
exportable commits since the last export; we could do that in this CL or another
one if it would be cleaner.
> 
> I'd prefer landing this on master and doing it in its own CL if you're ok with
that.

I added a bug for the aforementioned feature:
https://bugs.chromium.org/p/chromium/issues/detail?id=672215

[jeffcarp, 2016-12-13 22:06:53]

On 2016/12/07 at 20:27:19, jeffcarp wrote:
> On 2016/12/07 at 20:24:38, jeffcarp wrote:
> > Addressed qyearsley's CL feedback.
> > 
> >
https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
> > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right):
> > 
> >
https://codereview.chromium.org/2544173002/diff/40001/third_party/WebKit/Tool...
> > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:88: return
> > On 2016/12/07 at 19:05:36, qyearsley wrote:
> > > Note: we'll want to extract a separate function to get the list of
exportable commits since the last export; we could do that in this CL or another
one if it would be cleaner.
> > 
> > I'd prefer landing this on master and doing it in its own CL if you're ok
with that.
> 
> I added a bug for the aforementioned feature:
https://bugs.chromium.org/p/chromium/issues/detail?id=672215

Update on this CL: I'm doing a big refactor for
https://bugs.chromium.org/p/chromium/issues/detail?id=672215 based off this
branch that also includes some refactoring + improvements, so don't worry about
reviewing what's here until I can upload that.

[jeffcarp, 2016-12-15 20:48:12]

Description was changed from

==========
Skip commits that don't generate a patch + fixes to get export working

More specifically, this CL:
- Modifies ChromiumCommit.format_patch so that it:
  1. First gets a list of files changed
  2. Filters blacklisted files (e.g. MANIFEST.json) from that list
  3. Passes the new list of files to git format-patch
- Adds LocalWPT.test_patch, which applies a patch to WPT master and returns the
diff
  - This is to guard against changes like https://crrev.com/2488283003
- Fixes a number of things that broke in the export process


Most of this code was written in response to something breaking, so I apologize
in advance that it is not super elegant.

BUG=657117
==========

to

==========
Skip commits that don't generate a patch + fixes to get export working

More specifically, this CL:
- Modifies ChromiumCommit.format_patch so that it:
  1. First gets a list of files changed
  2. Filters blacklisted files (e.g. MANIFEST.json) from that list
  3. Passes the new list of files to git format-patch
- Adds LocalWPT.test_patch, which applies a patch to WPT master and returns the
diff
  - This is to guard against changes like https://crrev.com/2488283003
- Fixes a number of things that broke in the export process


Most of this code was written in response to something breaking, so I apologize
in advance that it is not super elegant.

BUG=657117,671890
==========

[qyearsley, 2016-12-15 20:59:14]

test_exporter.exportable_commits looks generally good :-)

But this patch now does lots of different things in different files, so it's a
bit slower to review -- could you separate out separate refactoring CLs and
behavior-changing CLs?

https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right):

https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:126: 
Possible extra comment: this looks at commits in Chromium after the last
exported commit. To find the last exported commits, it looks in the git log for
web-platform-tests.

https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:128: A list of
ChromiumCommits
Nit: period

https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:140:
_log.info('(%d behind chromium@origin/master)',
chromium_commit.num_behind_master())
This could potentially be changed to debug-level rather than info-level.

https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py
(right):

https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:86: def
test_exportable_commits_since(self):
Name: test_exportable_commits

[foolip, 2016-12-15 22:56:57]

I've been neglecting this and some other reviews. If the two of you agree then I
don't think my review is needed, but please do poke me if there's something I
should pay attention to.

[jeffcarp, 2016-12-16 00:13:16]

On 2016/12/15 at 20:59:14, qyearsley wrote:
> test_exporter.exportable_commits looks generally good :-)
> 
> But this patch now does lots of different things in different files, so it's a
bit slower to review -- could you separate out separate refactoring CLs and
behavior-changing CLs?
> 
>
https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
> File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right):
> 
>
https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:126: 
> Possible extra comment: this looks at commits in Chromium after the last
exported commit. To find the last exported commits, it looks in the git log for
web-platform-tests.
> 
>
https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:128: A list of
ChromiumCommits
> Nit: period
> 
>
https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:140:
_log.info('(%d behind chromium@origin/master)',
chromium_commit.num_behind_master())
> This could potentially be changed to debug-level rather than info-level.
> 
>
https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
> File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py
(right):
> 
>
https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
> third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:86:
def test_exportable_commits_since(self):
> Name: test_exportable_commits

I can break it up into separate CLs, but no intermediate CL will leave the
codebase in a working state until the last one is landed since all major parts
have bug fixes. Here's a rough list of how I'll split it up:

- Delete ChromiumWPT CL
- Change files fed into format-patch CL
- test_patch CL
- Address CL feedback CL
- Add label to PR CL

[qyearsley, 2016-12-16 00:34:38]

On 2016/12/16 at 00:13:16, jeffcarp wrote:
> On 2016/12/15 at 20:59:14, qyearsley wrote:
> > test_exporter.exportable_commits looks generally good :-)
> > 
> > But this patch now does lots of different things in different files, so it's
a bit slower to review -- could you separate out separate refactoring CLs and
behavior-changing CLs?
> > 
> >
https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
> > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py (right):
> > 
> >
https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
> > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:126: 
> > Possible extra comment: this looks at commits in Chromium after the last
exported commit. To find the last exported commits, it looks in the git log for
web-platform-tests.
> > 
> >
https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
> > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:128: A list
of ChromiumCommits
> > Nit: period
> > 
> >
https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
> > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py:140:
_log.info('(%d behind chromium@origin/master)',
chromium_commit.num_behind_master())
> > This could potentially be changed to debug-level rather than info-level.
> > 
> >
https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
> > File third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py
(right):
> > 
> >
https://codereview.chromium.org/2544173002/diff/120001/third_party/WebKit/Too...
> > third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py:86:
def test_exportable_commits_since(self):
> > Name: test_exportable_commits
> 
> I can break it up into separate CLs, but no intermediate CL will leave the
codebase in a working state until the last one is landed since all major parts
have bug fixes. Here's a rough list of how I'll split it up:
> 
> - Delete ChromiumWPT CL
> - Change files fed into format-patch CL
> - test_patch CL
> - Address CL feedback CL
> - Add label to PR CL

That sounds OK to me -- since you say that it's a sequence of CLs and the code
isn't in a working state until they're all landed, I should try to review them
all quickly.

In general, if there's no smaller CL that can be made that also leaves the code
in a working state, then it's OK to have a larger CL, although it may take
longer to review. (In this case, if you decide this can't easily be broken up,
I'm also OK with reviewing this CL as-is).

In general, the ideal CL can be summarized easily in one short sentence - if it
contains multiple bug fixes and other changes and it's hard to think of a
concise description, then that's a signal that breaking it up may be good.