1592451 - Broken revision when pushing D51019 with --no-arc (Error: Argument 1 passed to idx() must be of the type array, null given)

Status: RESOLVED FIXED

(Conduit :: moz-phab, defect, P1)

Description

[Henrik Skupin [:whimboo][⌚️UTC+2]]

I tried to push the patch as available now as D51019 with the --no-arc option, but it failed and produced an invalid revision:

https://phabricator.services.mozilla.com/D51018

The failure message is:

Unhandled Exception ("TypeError")

Argument 1 passed to idx() must be of the type array, null given, called in /app/phabricator/src/applications/differential/view/DifferentialChangesetListView.php on line 365

Comment 1

[Ian Moody [:Kwan] (UTC+0)]

To hopefully save some debugging time, I think this is because it's setting metadata for moved files to null rather than an empty array/object, e.g.:

      {
        "id": "1766514",
        "metadata": null,
        "oldPath": "None",
        "currentPath": "remote/test/browser/browser_page_bringToFront.js",
        "awayPaths": [
          "remote/test/browser/page/browser_bringToFront.js"
        ],
        "oldProperties": [],
        "newProperties": [],
        "type": "4",
        "fileType": "1",
        "commitHash": null,
        "addLines": "0",
        "delLines": "0",
        "hunks": []
      }

Comment 2

[:glob ✱]

Filed bug 1592591 to work around the issue on Phabricator; moz-phab should still be updated to avoid this in future.

Comment 3

[:glob ✱]

The following fields are non-nullable (from applications/differential/storage/DifferentialChangeset.php):

// These should all be non-nullable, and store reasonable default
// JSON values if empty.
'awayPaths' => 'text?',
'metadata' => 'text?',
'oldProperties' => 'text?',
'newProperties' => 'text?',

Comment 4

[:glob ✱]

zalun was able to reproduce the issue on phabricator-dev - https://phabricator-dev.allizom.org/D1516

moz-phab is always passing in {} as the metadata (instead of null/empty), however when the change type is 4:

                {
                    "metadata": {},
                    "oldPath": None,
                    "currentPath": "notepad.txt",
                    "awayPaths": ["something"],
                    "oldProperties": {},
                    "newProperties": {},
                    "commitHash": "2c9df17b1008069ec963dd12ca42eb3559527901",
                    "type": 4,
                    "fileType": 1,
                    "hunks": [],
                },

this results in the following in the database:

           id: 6737
       diffID: 2428
      oldFile: None
     filename: notepad.txt
    awayPaths: ["something"]
   changeType: 4
     fileType: 1
     metadata: null
oldProperties: []
newProperties: []
     addLines: 0
     delLines: 0
  dateCreated: 1572445691
 dateModified: 1572445691

What's super interesting is that null next to metadata is the string "null", not a NULL value.

Comment 5

[Piotr Zalewa [:zalun]]

Attachment: Bug 1592451 - Sending an empty array to the Phabricator r=glob!

Comment 6

[Ian Moody [:Kwan] (UTC+0)]

(In reply to Byron Jones ‹:glob› 🎈 from comment #4)

zalun was able to reproduce the issue on phabricator-dev - https://phabricator-dev.allizom.org/D1516

moz-phab is always passing in {} as the metadata (instead of null/empty), however when the change type is 4:

Is it just when the changeType is 4? Or when the hunks array is empty? I have a suspicion that for files with diffs phabricator auto-creates the metadata object when it adds the "line:first" or "hash.effect" properties, so that's why other revs weren't affected. I'd think type COPY_AWAY (5) would have been affected as well.

Anyway patch looks good, modulo glob's comment about handling empty arrays. phabsend-moz did something similar before the move to arcanist JSON dump.

Comment 7

[:glob ✱]

(In reply to Ian Moody [:Kwan] (UTC+1) from comment #6)

Is it just when the changeType is 4? Or when the hunks array is empty? I have a suspicion that for files with diffs phabricator auto-creates the metadata object when it adds the "line:first" or "hash.effect" properties, so that's why other revs weren't affected. I'd think type COPY_AWAY (5) would have been affected as well.

From my quick look at the database it was just 4, however, it's possible that other types were impacted. That data no longer exists so I can't verify.

Comment 8

[Ian Moody [:Kwan] (UTC+0)]

Thinking about it I'd imagine a copy is an order of magnitude rarer than a move, so there may simply not have been any.

Comment 9

[Andi [:andi]]

Something similar happened to me when pushing to D51242, revision 0. I got this changest, that when applying on top of m-c, i was getting: ValueError: invalid literal for int() with base 8: 'de 644'.
After re-pushing the same patch but this time no longer using option --no-arc everything when fine.

Comment 10

[Piotr Zalewa [:zalun]]

@andi (In reply to Andi-Bogdan Postelnicu [:andi] from comment #9)

Something similar happened to me when pushing to D51242, revision 0. I got this changset, that when applying on top of m-c, i was getting: ValueError: invalid literal for int() with base 8: 'de 644'.
After re-pushing the same patch but this time no longer using option --no-arc everything when fine.

Hi Andi, thanks for reporting.
This is bug 1591092 - released today in version 0.1.57 (together with the fix for this bug) - moz-phab self-update.
Until we will release bug 1591015 --no-arc will be switched off for Windows.