New issue
Advanced search Search tips

Issue 759817 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Sep 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 2
Type: Bug



Sign in to add a comment

[WPT Export] Transformation of Gerrit CL corrupts the patch

Project Member Reported by robertma@chromium.org, Aug 28 2017

Issue description

WPT Exporter needs to filter and transform patches from Gerrit in order to apply them against the WPT repo.

The transformation is found broken when processing this CL:
https://chromium-review.googlesource.com/c/chromium/src/+/627939

Log:
https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Fchromium.infra.cron%2Fwpt-exporter%2F35855%2F%2B%2Frecipes%2Fsteps%2FExport_Chromium_commits_and_in-flight_CLs_to_WPT%2F0%2Fstdout

Found Gerrit in-flight CL: "Support the CORS preflight cache on workers." https://chromium-review.googlesource.com/c/627939
No in-flight PR found for CL. Creating...
Gerrit CL patch did not apply cleanly:
Failed to run "['git', 'apply', '-']" exit_code: 128 cwd: /tmp/wpt
output: error: corrupt patch at line 79

It is suspected to be related to whitespace / empty line handling. The second hunk of the patch (which is a WPT change that needs to be preserved) ends with an empty line. It seems the exporter trims the empty line and hence corrupts the patch.
 
This reminds me a bit of  bug 743153  (applying patches from the gerrit api doesn't support changes to binary files).

In that bug, Jeff suggested "...I think the resolution would be to change the provisional PR flow to fetch and check out the actual Chromium git branch of that CL instead of using the patch from the Gerrit API."

With a Chromium checkout we can actually check out a Gerrit patchset, for example to get the diff from the current branch and patchset 9 of CL 876543, I think we can use something like:

git fetch origin refs/changes/43/876543/9 && git diff FETCH_HEAD
That sounds reasonable.

Although we can fix the transformation and filtering, the complexity of this piece of logic starts to outweigh the simplicity of the patch API. Besides, the patch API lacks crucial binary support. We can kill two birds with one stone if we switch to the git approach.
Status: Started (was: Available)
See  bug 743153 
Cc: -robertma@chromium.org
Owner: robertma@chromium.org
Project Member

Comment 5 by bugdroid1@chromium.org, Sep 15 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/2df6d6918634e0ad0f7b70db39b09363105eb068

commit 2df6d6918634e0ad0f7b70db39b09363105eb068
Author: Robert Ma <robertma@chromium.org>
Date: Fri Sep 15 15:09:19 2017

[WPT Export] Get patches of in-flight CLs from their git branches

This change fetches the git branch of a CL and uses `git format-patch`
to get the patch, instead of using Gerrit API which does not support
binary changes.

With this change in the workflow, we can also consolidate the processes
for both in-flight CLs and Chromium commits, which greatly simplifies
the code.

Related testing is also improved along the way to use more mocks and
avoid unnecessary end-to-end testing.

Bug:  743153 ,  759817 
Change-Id: I069b06e78f30089a31527974f0751032a822bd70
Reviewed-on: https://chromium-review.googlesource.com/658284
Commit-Queue: Robert Ma <robertma@chromium.org>
Reviewed-by: Quinten Yearsley <qyearsley@chromium.org>
Cr-Commit-Position: refs/heads/master@{#502258}
[modify] https://crrev.com/2df6d6918634e0ad0f7b70db39b09363105eb068/third_party/WebKit/Tools/Scripts/webkitpy/common/checkout/git_mock.py
[modify] https://crrev.com/2df6d6918634e0ad0f7b70db39b09363105eb068/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit.py
[modify] https://crrev.com/2df6d6918634e0ad0f7b70db39b09363105eb068/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit_mock.py
[modify] https://crrev.com/2df6d6918634e0ad0f7b70db39b09363105eb068/third_party/WebKit/Tools/Scripts/webkitpy/w3c/gerrit_unittest.py
[delete] https://crrev.com/313d77a53f2eafbc69ae61c2612491a8b88b4755/third_party/WebKit/Tools/Scripts/webkitpy/w3c/resources/expected.patch
[delete] https://crrev.com/313d77a53f2eafbc69ae61c2612491a8b88b4755/third_party/WebKit/Tools/Scripts/webkitpy/w3c/resources/expected2.patch
[delete] https://crrev.com/313d77a53f2eafbc69ae61c2612491a8b88b4755/third_party/WebKit/Tools/Scripts/webkitpy/w3c/resources/sample.patch
[delete] https://crrev.com/313d77a53f2eafbc69ae61c2612491a8b88b4755/third_party/WebKit/Tools/Scripts/webkitpy/w3c/resources/sample2.patch
[modify] https://crrev.com/2df6d6918634e0ad0f7b70db39b09363105eb068/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter.py
[modify] https://crrev.com/2df6d6918634e0ad0f7b70db39b09363105eb068/third_party/WebKit/Tools/Scripts/webkitpy/w3c/test_exporter_unittest.py

Status: Fixed (was: Started)

Sign in to add a comment