Closed
Bug 1480644
Opened 6 years ago
Closed 6 years ago
Arcanist doesn't handle MANIFEST.json gracefully
Categories
(Conduit :: Phabricator, enhancement)
Conduit
Phabricator
Tracking
(Not tracked)
RESOLVED
DUPLICATE
of bug 1446555
People
(Reporter: xidorn, Unassigned)
Details
It's not clear whether this should be tracked in our bugzilla... probably it should given that the problematic file is in our repo.
Today when I tried to submit a commit via Arcanist which includes a new web-platform test (which consequently requires update to testing/web-platform/meta/MANIFEST.json), the client complains about:
Diff for 'testing/web-platform/meta/MANIFEST.json' with context is
5,079,639 bytes in length. Generally, source changes should not be this
large. If this file is a huge text file, try using the '--less-context'
flag. If the file is not a text file, you can mark it 'binary'. Mark this
file as 'binary' and continue? [y/N]
If I refuse to mark it binary, it refuses to submit.
This is unfortunate, because MANIFEST.json is indeed a text file, and should be present in the review interface. It is indeed a huge file, but the diff isn't that bad.
The diff in this case is just something like:
diff --git a/testing/web-platform/meta/MANIFEST.json b/testing/web-platform/meta/MANIFEST.json
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -135404,16 +135404,28 @@
[
"/css/css-text/overflow-wrap/reference/overflow-wrap-min-content-size-002-ref.html",
"=="
]
],
{}
]
],
+ "css/css-text/overflow-wrap/overflow-wrap-min-content-size-003.html": [
+ [
+ "/css/css-text/overflow-wrap/overflow-wrap-min-content-size-003.html",
+ [
+ [
+ "/css/css-text/overflow-wrap/reference/overflow-wrap-min-content-size-003-ref.html",
+ "=="
+ ]
+ ],
+ {}
+ ]
+ ],
"css/css-text/overflow-wrap/word-wrap-001.html": [
[
"/css/css-text/overflow-wrap/word-wrap-001.html",
[
[
"/css/css-text/overflow-wrap/overflow-wrap-001-ref.html",
"=="
]
@@ -258249,16 +258261,21 @@
{}
]
],
"css/css-text/overflow-wrap/reference/overflow-wrap-min-content-size-002-ref.html": [
[
{}
]
],
+ "css/css-text/overflow-wrap/reference/overflow-wrap-min-content-size-003-ref.html": [
+ [
+ {}
+ ]
+ ],
"css/css-text/support/1x1-green.png": [
[
{}
]
],
"css/css-text/support/1x1-lime.png": [
[
{}
@@ -543061,16 +543078,20 @@
"css/css-text/overflow-wrap/overflow-wrap-min-content-size-001.html": [
"e4a7ef4a3852d328e8410b81ef20c4d3de0d771e",
"reftest"
],
"css/css-text/overflow-wrap/overflow-wrap-min-content-size-002.html": [
"5b3b1f19d7ae6374224da75567b3ba5279d16127",
"reftest"
],
+ "css/css-text/overflow-wrap/overflow-wrap-min-content-size-003.html": [
+ "e2bd4f769625954f92506f6063b48c240e9260cb",
+ "reftest"
+ ],
"css/css-text/overflow-wrap/reference/overflow-wrap-break-word-001-ref.html": [
"0e0300a72dc920a5ffb54cda6fbe84a2f517d010",
"support"
],
"css/css-text/overflow-wrap/reference/overflow-wrap-break-word-002-ref.html": [
"5dca68381729c017bac1724d8a195b33af847eaf",
"support"
],
@@ -543085,16 +543106,20 @@
"css/css-text/overflow-wrap/reference/overflow-wrap-min-content-size-001-ref.html": [
"99d964777c663fb8ca37be00c162ddfbb82951c9",
"support"
],
"css/css-text/overflow-wrap/reference/overflow-wrap-min-content-size-002-ref.html": [
"055ffbf3ca1377aaa502ffa02c52c8e49604a286",
"support"
],
+ "css/css-text/overflow-wrap/reference/overflow-wrap-min-content-size-003-ref.html": [
+ "a71b3a34d6920a5404cf7c954fa1c8ab66b788b4",
+ "support"
+ ],
"css/css-text/overflow-wrap/word-wrap-001.html": [
"dd5f0f2bf132de85c7a1045e88aa3ad2b72616c1",
"reftest"
],
"css/css-text/overflow-wrap/word-wrap-002.html": [
"380fb8ec4fde4decb82e52961ce5ef71a0a6c965",
"reftest"
],
which isn't unreasonably large.
I think Arcanist needs to learn how to handle this kind of files. Continuing to complain about this file would discourage people from adding new web-platform test.
(Alternatively, testing team should probably find a solution to avoid having this huge file to work around the restriction we've seen in many places.)
Reporter | ||
Comment 1•6 years ago
|
||
Also, neither making it --less-context nor marking it binary is an ideal solution.
With using --less-context, it seems context of all text files are stripped in the review interface ("Context not available." is shown).
When marking it binary, it seems to be uploading the whole MANIFEST.json (or at least a large part of it) which can be really slow when the upstream bandwidth isn't great.
Reporter | ||
Comment 2•6 years ago
|
||
I guess Arcanist should probably also try compressing the content before uploading even it's a binary, because it can potentially just be a huge text file...
You need to log in
before you can comment on or make changes to this bug.
Description
•