Bug 1570648 - Handle inaccessible Phabricator revisions r=zalun

Reviewers: zalun

Reviewed By: zalun

Bug #: 1570648

Differential Revision: https://phabricator.services.mozilla.com/D39688

Author: sigiesec

moz-phab

@@ -816,8 +816,11 @@ class ConduitAPI:
 
         # Return revisions in the same order requested.
         if ids:
+            # Skip revisions for which we do not have a query result.
             return [
-                revisions[phids_by_id[rev_id]] for rev_id in ids if phids_by_id[rev_id]
+                revisions[phids_by_id[rev_id]]
+                for rev_id in ids
+                if rev_id in phids_by_id
             ]
         else:
             return [revisions[phid] for phid in phids]
@@ -1542,6 +1545,21 @@ class Repository(object):
             if has_arc_rejections(commit["body"]):
                 commit_errors.append("contains arc fields")
 
+            if commit["rev-id"]:
+                revisions = conduit.get_revisions(ids=[int(commit["rev-id"])])
+                if len(revisions) == 0:
+                    commit_errors.append(
+                        "Phabricator did not return a query result for revision D%s"
+                        " (it might be inaccessible or not exist at all)"
+                        % commit["rev-id"]
+                    )
+
+            # commit_issues identified below this are commit_errors unless
+            # self.args.force is True, which makes them commit_warnings
+            commit_issues = (
+                commit_warnings if self.args and self.args.force else commit_errors
+            )
+
             for reviewer in commit_invalid_reviewers[commit["node"]]:
                 if "disabled" in reviewer:
                     commit_errors.append("User %s is disabled" % reviewer["name"])
@@ -1551,10 +1569,7 @@ class Repository(object):
                         reviewer["name"],
                         reviewer["until"],
                     )
-                    if self.args.force:
-                        commit_warnings.append(msg)
-                    else:
-                        commit_errors.append(msg)
+                    commit_issues.append(msg)
                 else:
                     commit_errors.append(
                         "%s is not a valid reviewer's name" % reviewer["name"]
@@ -3225,16 +3240,21 @@ def show_commit_stack(
         if commit.get("rev-id"):
             action = action_template % ("D" + commit["rev-id"])
             if validate:
-                revision = conduit.get_revisions(ids=[int(commit["rev-id"])])[0]
-                # Check if target bug ID is the same as in the Phabricator revision
-                change_bug_id = revision["fields"]["bugzilla.bug-id"] and (
-                    commit["bug-id"] != revision["fields"]["bugzilla.bug-id"]
-                )
-                # Check if comandeering is required
-                whoami = conduit.whoami()
-                if revision["fields"]["authorPHID"] != whoami["phid"]:
-                    is_author = False
+                revisions = conduit.get_revisions(ids=[int(commit["rev-id"])])
+                if len(revisions) > 0:
+                    revision = revisions[0]
+
+                    # Check if target bug ID is the same as in the Phabricator revision
+                    change_bug_id = revision["fields"]["bugzilla.bug-id"] and (
+                        commit["bug-id"] != revision["fields"]["bugzilla.bug-id"]
+                    )
 
+                    # Check if comandeering is required
+                    whoami = conduit.whoami()
+                    if "authorPHID" in revision["fields"] and (
+                        revision["fields"]["authorPHID"] != whoami["phid"]
+                    ):
+                        is_author = False
         else:
             action = action_template % "New"
 

tests/conftest.py

@@ -161,8 +161,7 @@ def in_process(monkeypatch, safe_environ, request):
     # Disable calls to sys.exit() at the end of the script.  Re-raise errors instead
     # to make test debugging easier.
     def reraise(*args, **kwargs):
-        t, v, tb = sys.exc_info()
-        raise (t, v, tb)
+        raise
 
     monkeypatch.setattr(sys, "exit", reraise)
 

tests/test_conduit.py

@@ -222,6 +222,16 @@ def test_get_revisions_search_by_phids_ordering(get_revs, m_call):
     ]
 
 
+def test_get_revisions_search_by_revids_missing(get_revs, m_call):
+    """phabricator does not return info on all rev ids"""
+    m_call.return_value = multiple_phab_result
+    assert get_revs(ids=[2, 4, 1, 3]) == [
+        dict(id=2, phid="PHID-2"),
+        dict(id=1, phid="PHID-1"),
+        dict(id=3, phid="PHID-3"),
+    ]
+
+
 @mock.patch("mozphab.ConduitAPI.call")
 def test_get_diffs(m_call):
     conduit = mozphab.conduit

tests/test_integration-git.py

@@ -475,3 +475,61 @@ def test_submit_update_bug_id(in_process, git_repo_path, init_sha):
         },
         mock.ANY,
     )
+
+
+def test_submit_update_revision_not_found(in_process, git_repo_path, init_sha):
+    call_conduit.reset_mock()
+    call_conduit.side_effect = (
+        # ping
+        dict(),
+        # response for searching D123 and D124
+        dict(
+            data=[
+                {
+                    "fields": {
+                        "bugzilla.bug-id": "1",
+                        "status": {"value": "needs-review"},
+                    },
+                    "phid": "PHID-DREV-y7x5hvdpe2gyerctdqqz",
+                    "id": 123,
+                    "attachments": {"reviewers": {"reviewers": []}},
+                }
+            ]
+        ),
+        # moz-phab asks again for D124
+        dict(data=[]),
+        # moz-phab asks again for D124
+        dict(data=[]),
+        # moz-phab asks again for D124
+        dict(data=[]),
+    )
+    testfile = git_repo_path / "X"
+    testfile.write_text(u"ą")
+    git_out("add", ".")
+    msgfile = git_repo_path / "msg"
+    msgfile.write_text(
+        u"""\
+Bug 1 - Ą
+
+Differential Revision: http://example.test/D123
+"""
+    )
+    git_out("commit", "--file", "msg")
+    testfile.write_text(u"missing repo")
+    msgfile.write_text(
+        u"""\
+Bug 1 - missing revision
+
+Differential Revision: http://example.test/D124
+"""
+    )
+    git_out("commit", "--all", "--file", "./msg")
+
+    with pytest.raises(mozphab.Error) as excinfo:
+        mozphab.main(
+            ["submit", "--yes", "--no-arc"]
+            + ["--bug", "1"]
+            + ["--message", "update message ćwikła"]
+            + [init_sha]
+        )
+    assert "query result for revision D124" in str(excinfo.value)