Skip to content

Commit

Permalink
Handle non-UTF8 diffs in the WPT upstream script
Browse files Browse the repository at this point in the history
The output of `git diff` and `git show` can include non-UTF8 text or
binary data, so the WPT upstream script should handle this properly.

Fixes #29620.
  • Loading branch information
mrobinson committed Apr 12, 2023
1 parent cf4160b commit 5db5771
Show file tree
Hide file tree
Showing 4 changed files with 42 additions and 6 deletions.
22 changes: 21 additions & 1 deletion etc/ci/upstream-wpt-changes/test.py
Expand Up @@ -229,7 +229,7 @@ class TestApplyCommitsToWPT(unittest.TestCase):

def run_test(self, pr_number: int, commit_data: dict):
def make_commit(data):
with open(os.path.join(TESTS_DIR, data[2]), encoding="utf-8") as file:
with open(os.path.join(TESTS_DIR, data[2]), "rb") as file:
return {"author": data[0], "message": data[1], "diff": file.read()}

commits = [make_commit(data) for data in commit_data]
Expand Down Expand Up @@ -272,6 +272,15 @@ def test_two_commits(self):
[
["test author <test@author>", "test commit message", "18746.diff"],
["another person <two@author>", "a different message", "wpt.diff"],
["another person <two@author>", "adding some non-utf8 chaos", "add-non-utf8-file.diff"],
],
)

def test_non_utf8_commit(self):
self.run_test(
100,
[
["test author <nonutf8@author>", "adding some non-utf8 chaos", "add-non-utf8-file.diff"],
],
)

Expand Down Expand Up @@ -455,6 +464,17 @@ def test_open_new_non_upstreamable_pr_with_preexisting_upstream_pr(self):
]
)

def test_opened_upstreamable_pr_with_non_utf8_file_contents(self):
self.assertListEqual(
self.run_test("opened.json", ["add-non-utf8-file.diff"]),
[
"CreateOrUpdateBranchForPRStep:1:servo-wpt-sync/wpt/servo_export_18746",
"OpenPRStep:servo-wpt-sync/wpt/servo_export_18746→wpt/wpt#1",
"CommentStep:servo/servo#18746:🤖 Opened new upstream WPT pull request "
"(wpt/wpt#1) with upstreamable changes.",
],
)

def test_open_new_upstreamable_pr_with_preexisting_upstream_pr_not_apply_cleanly_to_upstream(
self,
):
Expand Down
7 changes: 7 additions & 0 deletions etc/ci/upstream-wpt-changes/tests/add-non-utf8-file.diff
@@ -0,0 +1,7 @@
diff --git a/tests/wpt/web-platform-tests/non-utf8-file.txt b/tests/wpt/web-platform-tests/non-utf8-file.txt
new file mode 100644
index 0000000..5a992e0
--- /dev/null
+++ b/tests/wpt/web-platform-tests/non-utf8-file.txt
@@ -0,0 +1 @@
+foo bér ÿ "
13 changes: 10 additions & 3 deletions etc/ci/upstream-wpt-changes/wptupstreamer/__init__.py
Expand Up @@ -50,7 +50,7 @@ def __init__(self, path: str, sync: WPTSync):
self.path = path
self.sync = sync

def run(self, *args, env: dict = {}):
def run_without_encoding(self, *args, env: dict = {}):
command_line = ["git"] + list(args)
logging.info(" → Execution (cwd='%s'): %s",
self.path, " ".join(command_line))
Expand All @@ -63,12 +63,19 @@ def run(self, *args, env: dict = {}):
try:
return subprocess.check_output(
command_line, cwd=self.path, env=env, stderr=subprocess.STDOUT
).decode("utf-8")
)
except subprocess.CalledProcessError as exception:
logging.warning("Process execution failed with output:\n%s",
exception.output.decode("utf-8"))
exception.output.decode("utf-8", errors="surrogateescape"))
raise exception

def run(self, *args, env: dict = {}):
return (
self
.run_without_encoding(*args, env=env)
.decode("utf-8", errors="surrogateescape")
)


@dataclasses.dataclass()
class SyncRun:
Expand Down
6 changes: 4 additions & 2 deletions etc/ci/upstream-wpt-changes/wptupstreamer/step.py
Expand Up @@ -114,7 +114,9 @@ def _get_upstreamable_commits_from_local_servo_repo(self, sync: WPTSync):
# TODO: If we could cleverly parse and manipulate the full commit diff
# we could avoid cloning the servo repository altogether and only
# have to fetch the commit diffs from GitHub.
diff = local_servo_repo.run(
# NB: The output of git show might include binary files or non-UTF8 text, so
# store the content of the diff as a `bytes``.
diff = local_servo_repo.run_without_encoding(
"show", "--binary", "--format=%b", sha, "--", UPSTREAMABLE_PATH
)

Expand All @@ -140,7 +142,7 @@ def _apply_filtered_servo_commit_to_wpt(self, run: SyncRun, commit: dict):
strip_count = UPSTREAMABLE_PATH.count("/") + 1

try:
with open(patch_path, "w", encoding="utf-8") as file:
with open(patch_path, "wb") as file:
file.write(commit["diff"])
run.sync.local_wpt_repo.run(
"apply", PATCH_FILE_NAME, "-p", str(strip_count)
Expand Down

0 comments on commit 5db5771

Please sign in to comment.