1646831 - Failed landing due to incorrect diff for empty files/lines.

Status: RESOLVED FIXED

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

Description

[Sylvestre Ledru [:Sylvestre]]

when I tried to land: https://lando.services.mozilla.com/D80029/
As suggested on chat.m.o, pushing with "moz-phab submit --arc" with my issue!

Comment 1

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

As far as I can tell this is due to moz-phab setting null metadata on binary file add/removes.
moz-phab gives the following to a file add

        "metadata": {
          "old:binary-phid": null,
          "old:file:size": 0,
          "old:file:mime-type": "",
          "new:binary-phid": "PHID-FILE-tqeyupg2ofmkpsrprekv",
          "new:file:size": 112,
          "new:file:mime-type": "image/png"
        }

Whereas arcanist will only set the new: ones for adds, and vice-versa for file removes, like

        "metadata": {
          "new:file:size": 112,
          "new:file:mime-type": "image/png",
          "new:binary-phid": "PHID-FILE-wb3sdj7rvy5vgujjhoq2"
        }

Comment 2

[Piotr Zalewa [:zalun]]

Attachment: Difference between updates from plain moz-phab and with --arc option

Comment 3

[Piotr Zalewa [:zalun]]

https://phabricator.services.mozilla.com/D80026?vs=298461&id=298777#toc
This one shows a difference between the plain moz-phab and --arc diffs.
Seems like file ending. I'll investigate further tomorrow.

Comment 4

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

(In reply to Piotr Zalewa [:zalun] from comment #3)

https://phabricator.services.mozilla.com/D80026?vs=298461&id=298777#toc
This one shows a difference between the plain moz-phab and --arc diffs.
Seems like file ending. I'll investigate further tomorrow.

Can confirm the issue is indeed the empty files. moz-phab is giving them a hunk object, whereas arcanist doesn't.
Manual test with a duff hunk: https://phabricator-dev.allizom.org/D2122

From https://phabricator.services.mozilla.com/D80026
moz-phab (Diff 298461):

      {
        "id": "2687976",
        "metadata": {
          "hash.effect": "qVzKuHbdOl_L"
        },
        "oldPath": null,
        "currentPath": "third_party/python/pyflakes/pyflakes/scripts/__init__.py",
        "awayPaths": [],
        "oldProperties": [],
        "newProperties": {
          "unix:filemode": "100644"
        },
        "type": "1",
        "fileType": "1",
        "commitHash": null,
        "addLines": "0",
        "delLines": "0",
        "hunks": [
          {
            "oldOffset": "0",
            "newOffset": "1",
            "oldLength": "0",
            "newLength": "0",
            "addLines": null,
            "delLines": null,
            "isMissingOldNewline": null,
            "isMissingNewNewline": null,
            "corpus": ""
          }
        ]
      }

arcanist (Diff 298777):

      {
        "id": "2689840",
        "metadata": [],
        "oldPath": "third_party/python/pyflakes/pyflakes/scripts/__init__.py",
        "currentPath": "third_party/python/pyflakes/pyflakes/scripts/__init__.py",
        "awayPaths": [],
        "oldProperties": [],
        "newProperties": {
          "unix:filemode": "100644"
        },
        "type": "1",
        "fileType": "1",
        "commitHash": null,
        "addLines": "0",
        "delLines": "0",
        "hunks": []
      }

Comment 5

[Piotr Zalewa [:zalun]]

Can you upload the third_party/python/attrs/srcattr/py.typed and third_party/python/pyflakes/scripts/__init__.py before and after the change?

Comment 6

[Piotr Zalewa [:zalun]]

Attachment: Bug 1646831 - Do not create hunks for empty files. r=zeid!

Comment 7

[Sylvestre Ledru [:Sylvestre]]

I am sorry but I didn't have them :/