Handle commits with missing prs during landing setup

sync/command.py

@@ -12,6 +12,7 @@
 
 from . import listen
 from .base import ProcessData
+from .commit import WptCommit
 from .phab import listen as phablisten
 from . import log
 from .tasks import setup
@@ -308,6 +309,16 @@ def get_parser() -> argparse.ArgumentParser:
     parser_migrate = subparsers.add_parser("migrate", help="Migrate to latest data storage format")
     parser_migrate.set_defaults(func=do_migrate)
 
+    parser_set_pr = subparsers.add_parser(
+        "set-pr",
+        help="Set PR number for a wpt commit (set to `0` if the commit really doesn't have an associated PR)",
+    )
+    parser_set_pr.add_argument("pr_number", default=None, type=int, help="The PR number")
+    parser_set_pr.add_argument(
+        "commits", default=None, type=str, nargs="*", help="The wpt commit hash"
+    )
+    parser_set_pr.set_defaults(func=do_set_pr)
+
     return parser
 
 
@@ -1173,6 +1184,20 @@ def do_migrate(git_gecko: Repo, git_wpt: Repo, **kwargs: Any) -> None:
         git2_repo.references.delete(ref_name)
 
 
+def do_set_pr(git_gecko: Repo, git_wpt: Repo, pr_number: int, commits: list[str]) -> None:
+    if pr_number is None or commits is None:
+        logger.error("No PR number or commits is not provided")
+    else:
+        if pr_number != 0:
+            pr = env.gh_wpt.get_pull(pr_number)
+            if pr is None:
+                logger.error(f"PR {pr_number} not found")
+        else:
+            for commit in commits:
+                wpt_commit = WptCommit(git_wpt, commit)
+                wpt_commit.notes["wpt_pr"] = str(pr_number)
+
+
 def set_config(opts: list[str]) -> None:
     for opt in opts:
         keys, value = opt.split("=", 1)

sync/landing.py

@@ -1011,6 +1011,7 @@ def landable_commits(
     prev_wpt_head: str,
     wpt_head: str | None = None,
     include_incomplete: bool = False,
+    landing: LandingSync | None = None,
 ) -> tuple[str, LandableCommits] | None:
     """Get the list of commits that are able to land.
 
@@ -1026,8 +1027,12 @@ def landable_commits(
     landable_commits = []
     for pr, commits in pr_commits:
         last = False
-        if not pr:
-            # Assume this was some trivial fixup:
+        if pr is None:
+            message = record_missing_pr_number(commits, landing)
+            raise AbortError(message)
+
+        if pr == 0:
+            # If PR number is set to `0`, it means that the commit really doesn't have an assosieted PR.
             continue
 
         first_commit = first_non_merge(commits)
@@ -1209,6 +1214,7 @@ def update_landing(
                 landing.wpt_commits.base.sha1,
                 landing.wpt_commits.head.sha1,
                 include_incomplete=include_incomplete,
+                landing=landing,
             )
             if landable is None:
                 raise AbortError("No new commits are landable")
@@ -1367,16 +1373,19 @@ def needinfo_users() -> list[str]:
 
 
 def record_failure(
-    sync: LandingSync, log_msg: str, bug_msg: str, fixup_msg: Any | None = None
+    sync: LandingSync | None, log_msg: str, bug_msg: str, fixup_msg: Any | None = None
 ) -> str:
     if fixup_msg is None:
         fixup_msg = "Run `wptsync landing` with either --accept-failures or --retry"
-    logger.error(f"Bug {sync.bug}:{log_msg}\n{fixup_msg}")
-    sync.error = log_msg
-    assert sync.bug is not None
-    with env.bz.bug_ctx(sync.bug) as bug:
-        bug.add_comment(f"{bug_msg}\nThis requires fixup from a wpt sync admin.")
-        bug.needinfo(*needinfo_users())
+    if sync is not None:
+        logger.error(f"Bug {sync.bug}:{log_msg}\n{fixup_msg}")
+        sync.error = log_msg
+        assert sync.bug is not None
+        with env.bz.bug_ctx(sync.bug) as bug:
+            bug.add_comment(f"{bug_msg}\nThis requires fixup from a wpt sync admin.")
+            bug.needinfo(*needinfo_users())
+    else:
+        logger.error(f"{log_msg}\n{fixup_msg}")
     return log_msg
 
 
@@ -1407,6 +1416,15 @@ def record_rebase_failure(sync: LandingSync) -> str:
     return record_failure(sync, log_msg, bug_msg, fixup_msg)
 
 
+def record_missing_pr_number(commits: list[WptCommit], sync: LandingSync | None) -> str:
+    commit_hashes = [commit.sha1 for commit in commits]
+    commit_hashes_string = ", ".join(commit_hashes)
+
+    msg = f"""The PR number is missing for {commit_hashes_string}"""
+    fixup_msg = "Add the PR number with `wptsync set-pr` and run `wptsync landing`"
+    return record_failure(sync, msg, msg, fixup_msg)
+
+
 def update_metadata(
     sync: LandingSync, try_push: TryPush, tasks: TryPushTasks | None = None
 ) -> None:

test/test_landing.py

@@ -326,6 +326,12 @@ def test_landing_reapply(
 
     trypush.Mach = mock_mach
 
+    # Add an upstream commit that has metadata
+    pr = pull_request([(b"Upstream change 1", {"upstream1": b"UPSTREAM1\n"})])
+    head_rev = pr._commits[0]["sha"]
+    downstream.new_wpt_pr(git_gecko, git_wpt, pr)
+    downstream_sync = set_pr_status(pr.number, "success")
+
     # Add first gecko change
     test_changes = {"change1": b"CHANGE1\n"}
     rev = upstream_gecko_commit(test_changes=test_changes, bug=1111, message=b"Add change1 file")
@@ -365,11 +371,6 @@ def test_landing_reapply(
     git_wpt_upstream.git.checkout("master")
     git_wpt_upstream.git.merge(remote_branch, ff_only=True)
 
-    # Add an upstream commit that has metadata
-    pr = pull_request([(b"Upstream change 1", {"upstream1": b"UPSTREAM1\n"})])
-    head_rev = pr._commits[0]["sha"]
-    downstream.new_wpt_pr(git_gecko, git_wpt, pr)
-    downstream_sync = set_pr_status(pr.number, "success")
     git_wpt_upstream.head.commit = head_rev
     git_wpt_upstream.git.reset(hard=True)
     with SyncLock.for_process(downstream_sync.process_name) as downstream_lock:
@@ -551,6 +552,12 @@ def create_and_upstream_gecko_bug(
     update_repositories(git_gecko, git_wpt, wait_gecko_commit=rev)
     upstream.gecko_push(git_gecko, git_wpt, "mozilla-central", rev, raise_on_error=True)
 
+    # Set commits PR number so they will be skipped.
+    for commit_item in git_wpt.iter_commits(sync.wpt_commits.head.sha1):
+        wpt_commit = sync_commit.WptCommit(git_wpt, commit_item)
+        if wpt_commit.pr() is None:
+            wpt_commit.notes["wpt_pr"] = 0
+
     return sync
 
 
@@ -773,3 +780,136 @@ def mock_rebase(branch):
 
     assert sync.status != "complete"
     assert downstream_sync.status != "complete"
+
+
+def test_upstream_commit_missing_pr_number(
+    env, git_gecko, git_wpt, git_wpt_upstream, pull_request, set_pr_status, mock_mach
+):
+    trypush.Mach = mock_mach
+
+    pr = pull_request(
+        [
+            (
+                b"Test commit",
+                {"README": b"example_change", "resources/testdriver_vendor.js": b"Some change"},
+            )
+        ]
+    )
+    head_rev = pr._commits[0]["sha"]
+
+    downstream.new_wpt_pr(git_gecko, git_wpt, pr)
+    sync = set_pr_status(pr.number, "success")
+
+    git_wpt_upstream.head.commit = head_rev
+    git_wpt.remotes.origin.fetch()
+    landing.wpt_push(git_gecko, git_wpt, [head_rev], create_missing=False)
+
+    # Reset PR number.
+    for commit_item in git_wpt.iter_commits(sync.wpt_commits.head.sha1):
+        wpt_commit = sync_commit.WptCommit(git_wpt, commit_item)
+        wpt_commit.notes["wpt_pr"] = None
+
+    with SyncLock.for_process(sync.process_name) as downstream_lock:
+        with sync.as_mut(downstream_lock):
+            sync.data["force-metadata-ready"] = True
+
+    landing_sync = None
+    with patch.object(trypush.TryCommit, "read_treeherder", autospec=True) as mock_read:
+        mock_read.return_value = "0000000000000000"
+        with pytest.raises(AbortError):
+            landing_sync = landing.update_landing(git_gecko, git_wpt)
+
+    # Make sure that landing was not created.
+    assert landing_sync is None
+
+    # Set the PR number back.
+    for commit_item in git_wpt.iter_commits(sync.wpt_commits.head.sha1):
+        wpt_commit = sync_commit.WptCommit(git_wpt, commit_item)
+        wpt_commit.notes["wpt_pr"] = pr["number"]
+
+    with patch.object(trypush.TryCommit, "read_treeherder", autospec=True) as mock_read:
+        mock_read.return_value = "0000000000000000"
+        landing_sync = landing.update_landing(git_gecko, git_wpt)
+
+    # Make sure that landing was created this time.
+    assert landing_sync is not None
+
+
+def test_upstream_commit_missing_pr_number_with_has_no_pr(
+    env,
+    git_gecko,
+    git_wpt,
+    git_wpt_upstream,
+    pull_request,
+    set_pr_status,
+    mock_mach,
+):
+    trypush.Mach = mock_mach
+
+    pr = pull_request(
+        [
+            (
+                b"Test commit",
+                {"resources/testdriver_vendor.js": b"Some change"},
+            )
+        ]
+    )
+    head_rev = pr._commits[0]["sha"]
+
+    downstream.new_wpt_pr(git_gecko, git_wpt, pr)
+    sync = set_pr_status(pr.number, "success")
+
+    git_wpt_upstream.head.commit = head_rev
+    git_wpt.remotes.origin.fetch()
+    landing.wpt_push(git_gecko, git_wpt, [head_rev], create_missing=False)
+
+    # Reset PR number.
+    for commit_item in git_wpt.iter_commits(sync.wpt_commits.head.sha1):
+        wpt_commit = sync_commit.WptCommit(git_wpt, commit_item)
+        wpt_commit.notes["wpt_pr"] = None
+
+    with SyncLock.for_process(sync.process_name) as downstream_lock:
+        with sync.as_mut(downstream_lock):
+            sync.data["force-metadata-ready"] = True
+
+    pr_2 = pull_request(
+        [
+            (
+                b"Test commit 2",
+                {"README": b"example_change2"},
+            )
+        ]
+    )
+    head_rev_2 = pr_2._commits[0]["sha"]
+
+    downstream.new_wpt_pr(git_gecko, git_wpt, pr_2)
+    sync_2 = set_pr_status(pr_2.number, "success")
+
+    git_wpt_upstream.head.commit = head_rev_2
+    git_wpt.remotes.origin.fetch()
+    landing.wpt_push(git_gecko, git_wpt, [head_rev_2], create_missing=False)
+
+    with SyncLock.for_process(sync_2.process_name) as downstream_lock:
+        with sync_2.as_mut(downstream_lock):
+            sync_2.data["force-metadata-ready"] = True
+
+    landing_sync = None
+    with patch.object(trypush.TryCommit, "read_treeherder", autospec=True) as mock_read:
+        mock_read.return_value = "0000000000000000"
+        with pytest.raises(AbortError):
+            landing_sync = landing.update_landing(git_gecko, git_wpt)
+
+    # Make sure that landing was not created.
+    assert landing_sync is None
+
+    # Set the PR number to `0` to indicate that it has to be skipped.
+    for commit_item in git_wpt.iter_commits(sync.wpt_commits.head.sha1):
+        wpt_commit = sync_commit.WptCommit(git_wpt, commit_item)
+        wpt_commit.notes["wpt_pr"] = 0
+
+    with patch.object(trypush.TryCommit, "read_treeherder", autospec=True) as mock_read:
+        mock_read.return_value = "0000000000000000"
+        landing_sync = landing.update_landing(git_gecko, git_wpt)
+
+    # Make sure that landing was created this time.
+    assert landing_sync is not None