1480644 - Arcanist doesn't handle MANIFEST.json gracefully

Status: RESOLVED DUPLICATE of bug 1446555

(Conduit :: Phabricator, enhancement)

Description

[Xidorn Quan [:xidorn] UTC+11]

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.)

Comment 1

[Xidorn Quan [:xidorn] UTC+11]

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.

Comment 2

[Xidorn Quan [:xidorn] UTC+11]

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...