Closed Bug 1480644 Opened 6 years ago Closed 6 years ago

Arcanist doesn't handle MANIFEST.json gracefully

Categories

(Conduit :: Phabricator, enhancement)

enhancement
Not set
normal

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.)
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.
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...
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.