diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index bfea2f6a02..41ff9f7e3c 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -130,6 +130,9 @@ jobs: BASE_SHA: ${{ github.event.pull_request.base.sha }} HEAD_SHA: ${{ github.event.pull_request.head.sha }} PUSH_SHA: ${{ github.sha }} + PR_NUMBER: ${{ github.event.pull_request.number }} + HEAD_REPO: ${{ github.event.pull_request.head.repo.full_name }} + HEAD_REF: ${{ github.event.pull_request.head.ref }} REPO: ${{ github.repository }} APPROVED: ${{ contains(github.event.pull_request.labels.*.name, 'approved-for-ci') }} GH_TOKEN: ${{ github.token }} @@ -236,7 +239,8 @@ jobs: # touched the pull request's files, and each restarted the whole run. carried= if [ "$EVENT_NAME" = pull_request ] && [ "$native" = true ]; then - carried=$(python3 -I scripts/ci/main_merge.py --base "$BASE_SHA" --head "$HEAD_SHA" --repo "$REPO" | sed -n 's/^carried=//p') + carried=$(python3 -I scripts/ci/main_merge.py --base "$BASE_SHA" --head "$HEAD_SHA" --repo "$REPO" \ + --pr "$PR_NUMBER" --head-repo "$HEAD_REPO" --head-ref "$HEAD_REF" | sed -n 's/^carried=//p') if [ -n "$carried" ]; then echo "Only a merge of main into a change that passed in $carried: repository checks only." native=false diff --git a/scripts/ci/main_merge.py b/scripts/ci/main_merge.py index 825701dfa4..ca8644aae1 100644 --- a/scripts/ci/main_merge.py +++ b/scripts/ci/main_merge.py @@ -1,14 +1,18 @@ #!/usr/bin/env python3 """Is this pull request push only a merge of main into a change that already passed CI? - python3 -I scripts/ci/main_merge.py --base BASE_SHA --head HEAD_SHA --repo OWNER/NAME + python3 -I scripts/ci/main_merge.py --base BASE_SHA --head HEAD_SHA --repo OWNER/NAME \ + --pr NUMBER --head-repo OWNER/NAME --head-ref BRANCH Prints `carried=` when all of these hold, else `carried=`: - HEAD is a merge commit whose second parent is on main (an ancestor of BASE): "Merge branch 'main'", or GitHub's Update branch button; - - the pull request's own change is the same before and after the merge (equal `git patch-id`), - so the merge resolved no conflict and brought nothing into the change itself; - - a ci.yml run of the change before the merge passed with the test shards. When that change was + - the pull request's own change is the same before and after the merge (equal `git patch-id + --verbatim`, whitespace included), so the merge resolved no conflict and brought nothing into + the change itself; + - a ci.yml run of this pull request (by number, or for a fork, where GitHub lists no pull + request on the run, by head repository and branch) for the change before the merge passed + with the test shards. When that change was itself only a merge of main (a run that carried, its shards skipped), the walk goes on down the first parents, through at most MAX_CHAIN merges, to the run where the shards ran. @@ -38,7 +42,8 @@ def patch_id(a, b): diff = subprocess.run(["git", "diff", "--no-renames", a, b], capture_output=True, check=True).stdout if not diff.strip(): return "" - out = subprocess.run(["git", "patch-id", "--stable"], input=diff, capture_output=True, check=True).stdout + # --verbatim: --stable ignores whitespace, which matters in strings and indentation. + out = subprocess.run(["git", "patch-id", "--verbatim"], input=diff, capture_output=True, check=True).stdout return out.split()[0].decode() if out.split() else "" @@ -74,10 +79,20 @@ def get(url, token): return json.load(r) -def passing_run(repo, sha, token): - """URL of a successful pull request ci.yml run of `sha` whose test shards all passed.""" +def same_pull_request(run, pr, head_repo, head_ref): + """Whether a workflow run belongs to this pull request, not another one with the same head.""" + numbers = [p.get("number") for p in run.get("pull_requests") or []] + if numbers: + return pr in numbers + return (run.get("head_repository") or {}).get("full_name") == head_repo and run.get("head_branch") == head_ref + + +def passing_run(repo, sha, token, pr, head_repo, head_ref): + """URL of a successful ci.yml run of this pull request at `sha` whose test shards all passed.""" runs = get(f"{API}/repos/{repo}/actions/workflows/ci.yml/runs?head_sha={sha}&event=pull_request&status=success&per_page=10", token) for run in runs.get("workflow_runs", []): + if not same_pull_request(run, pr, head_repo, head_ref): + continue jobs = get(f"{API}/repos/{repo}/actions/runs/{run['id']}/jobs?per_page=100", token).get("jobs", []) shards = [j for j in jobs if j.get("name", "").startswith(SHARD_PREFIX)] if shards and all(j.get("conclusion") == "success" for j in shards): @@ -90,12 +105,15 @@ def main(): ap.add_argument("--base", required=True) ap.add_argument("--head", required=True) ap.add_argument("--repo", required=True) + ap.add_argument("--pr", required=True, type=int) + ap.add_argument("--head-repo", required=True) + ap.add_argument("--head-ref", required=True) a = ap.parse_args() carried = "" try: token = os.environ.get("GH_TOKEN", "") for change in merge_chain(a.base, a.head) if token else []: - carried = passing_run(a.repo, change, token) + carried = passing_run(a.repo, change, token, a.pr, a.head_repo, a.head_ref) if carried: print(f"{a.head} merges main into {change}, which passed: {carried}", file=sys.stderr) break diff --git a/scripts/ci/test_main_merge.py b/scripts/ci/test_main_merge.py index 0fde86d16e..163f726488 100644 --- a/scripts/ci/test_main_merge.py +++ b/scripts/ci/test_main_merge.py @@ -39,6 +39,18 @@ class Repo: LINES = "".join(f"line {i}\n" for i in range(40)) +class SamePullRequest(unittest.TestCase): + def test_listed_pull_request_must_match(self): + self.assertTrue(main_merge.same_pull_request({"pull_requests": [{"number": 7}]}, 7, "x/y", "b")) + self.assertFalse(main_merge.same_pull_request({"pull_requests": [{"number": 8}]}, 7, "x/y", "b")) + + def test_fork_run_matches_by_head_repository_and_branch(self): + run = {"pull_requests": [], "head_repository": {"full_name": "fork/solidcraft"}, "head_branch": "fix"} + self.assertTrue(main_merge.same_pull_request(run, 7, "fork/solidcraft", "fix")) + self.assertFalse(main_merge.same_pull_request(run, 7, "fork/solidcraft", "other")) + self.assertFalse(main_merge.same_pull_request(run, 7, "someone/solidcraft", "fix")) + + class MergeOfMain(unittest.TestCase): def setUp(self): self.tmp = tempfile.TemporaryDirectory() @@ -98,6 +110,16 @@ class MergeOfMain(unittest.TestCase): head = self.merge_main() self.assertIsNone(main_merge.merge_of_main(base, head)) + def test_whitespace_change_in_the_change_is_not_carried(self): + # A conflict resolution that only respaces the change's own line still changes the change. + base = self.main_commit(**{"a.txt": LINES.replace("line 5\n", "line 5 on main\n")}) + subprocess.run(["git", "merge", "-q", "main"], cwd=self.tmp.name, capture_output=True) + self.r.write("a.txt", LINES.replace("line 5\n", "line five\n")) + self.r.git("add", "-A") + self.r.git("commit", "-q", "--no-edit") + head = self.r.git("rev-parse", "HEAD") + self.assertIsNone(main_merge.merge_of_main(base, head)) + def test_ordinary_commit_is_not_carried(self): head = self.r.commit("more", **{"a.txt": LINES.replace("line 5\n", "line 5b\n")}) base = self.r.git("rev-parse", "main")