From 9d64ebc2db64b874ef7aa7153bc4bcf790db7ec9 Mon Sep 17 00:00:00 2001 From: Siddartha Pothapragada Date: Tue, 11 Aug 2026 17:58:16 -0700 Subject: [PATCH] Lint only what a PR adds: diff the link checks from the merge base The URL, xref, and file size linters diffed base..head, where base is the tip of the base branch rather than the point the branch left it. A branch cut before recent commits still carries the lines those commits replaced, so against the newer tip its old copies read as additions and the branch is blamed for links someone else already repaired. #21729 failed exactly that way, on nine links #21694 had already fixed or ignored. The workflow resolves the merge base and passes it down. That is the half that matters for branches already open: on a pull request the reusable workflow resolves from the merge commit, so it carries this fix even though scripts/ still comes from the branch itself. The scripts switch to three dot ranges so running them by hand behaves the same way. Computing a merge base needs history, so the pull request path checks out in full, restoring what #17682 traded away for speed at a measured 25s of wall clock. Pushes and the nightly whole tree scan cannot use a merge base and stay shallow. A missing merge base leaves the range unset for a whole tree scan rather than substituting the base tip, which two of the three scripts accept and then quietly pass. Authored with Claude Code (Claude Opus 5). --- .../tests/test_link_check_diff_selection.py | 161 ++++++++++++++++++ .github/workflows/_link_check.yml | 61 ++++++- scripts/lint_file_size.sh | 4 +- scripts/lint_urls.sh | 6 +- scripts/lint_xrefs.sh | 6 +- 5 files changed, 225 insertions(+), 13 deletions(-) create mode 100644 .ci/scripts/tests/test_link_check_diff_selection.py diff --git a/.ci/scripts/tests/test_link_check_diff_selection.py b/.ci/scripts/tests/test_link_check_diff_selection.py new file mode 100644 index 00000000000..c79b9f26236 --- /dev/null +++ b/.ci/scripts/tests/test_link_check_diff_selection.py @@ -0,0 +1,161 @@ +# Copyright (c) Meta Platforms, Inc. and affiliates. +# All rights reserved. +# +# This source code is licensed under the BSD-style license found in the +# LICENSE file in the root directory of this source tree. + +import os +import shutil +import subprocess +import sys +import tempfile +import unittest +from pathlib import Path + +import pytest + +REPO_ROOT = Path(__file__).resolve().parents[3] + +# Fixture content, not real references. The linters under test would otherwise find +# their own bait here and check it against this repo. +BAD = "[broken](sub/missing.md)\nhttps://example.invalid/dead\n" # @lint-ignore +GOOD = "[fine](sub/present.md)\nhttps://example.invalid/live\n" # @lint-ignore + +OVERSIZED = "x" * (1024 * 1024 + 1) + +CURL_STUB = """#!/bin/sh +for arg in "$@"; do url=$arg; done +case "$url" in *dead*) echo 404 ;; *) echo 200 ;; esac +""" + + +@pytest.mark.skipif(sys.platform == "win32", reason="The scripts under test need bash") +class TestLinkCheckDiffSelection(unittest.TestCase): + """A branch cut before recent base commits keeps the base branch's old lines. Those + are not the branch's to answer for, so the linters must diff from the merge base. + + Each fixture file pins a different part of that: base_only.md the file selection, + both_sides.md the per-file diff, big.bin the file size range, feature_only.md that + the branch's own additions are still checked at all. + """ + + def setUp(self): + self.repo = Path(tempfile.mkdtemp()) + self.addCleanup(shutil.rmtree, self.repo, ignore_errors=True) + self.git("init", "-q") + self.git("symbolic-ref", "HEAD", "refs/heads/main") + self.git("config", "user.email", "test@example.com") + self.git("config", "user.name", "test") + + self.write("sub/present.md", "target\n") + self.write("base_only.md", BAD) + self.write("both_sides.md", BAD) + self.write("big.bin", OVERSIZED) + self.commit("initial") + self.git("branch", "feature") + + self.write("base_only.md", GOOD) + self.write("both_sides.md", GOOD) + self.write("big.bin", "small\n") + self.commit("main repairs the links and shrinks the file") + + self.git("checkout", "-q", "feature") + self.write("both_sides.md", BAD + GOOD) + self.write("feature_only.md", GOOD) + self.commit("feature edits one file and adds another") + + def env(self, **extra): + # A developer's git config and any inherited GIT_DIR must not reach the fixture. + env = {k: v for k, v in os.environ.items() if not k.startswith("GIT_")} + env["GIT_CONFIG_GLOBAL"] = os.devnull + env["GIT_CONFIG_SYSTEM"] = os.devnull + env.update(extra) + return env + + def git(self, *args): + subprocess.run( + ["git", *args], + cwd=self.repo, + check=True, + capture_output=True, + env=self.env(), + ) + + def write(self, relpath, text): + path = self.repo / relpath + path.parent.mkdir(parents=True, exist_ok=True) + path.write_text(text) + + def commit(self, message): + self.git("add", "-A") + self.git("commit", "-q", "-m", message) + + def add_on_feature(self, relpath, text): + self.write(relpath, text) + self.commit(f"feature adds {relpath}") + + def lint(self, script, **env): + return subprocess.run( + ["bash", str(REPO_ROOT / "scripts" / script), "main", "feature"], + cwd=self.repo, + capture_output=True, + text=True, + env=self.env(**env), + ) + + def assert_scoped_to_feature(self, result): + output = result.stdout + result.stderr + self.assertEqual(result.returncode, 0, output) + self.assertIn("feature_only.md", result.stdout) + self.assertIn("both_sides.md", result.stdout) + self.assertNotIn("base_only.md", output) + + def curl_stub(self): + stub_dir = self.repo / "stub" + stub_dir.mkdir() + stub = stub_dir / "curl" + stub.write_text(CURL_STUB) + stub.chmod(0o755) + return {"PATH": f"{stub_dir}{os.pathsep}{os.environ['PATH']}"} + + def test_lint_urls_ignores_lines_the_branch_only_lacks(self): + self.assert_scoped_to_feature(self.lint("lint_urls.sh", **self.curl_stub())) + + def test_lint_xrefs_ignores_lines_the_branch_only_lacks(self): + self.assert_scoped_to_feature(self.lint("lint_xrefs.sh")) + + def test_lint_file_size_ignores_files_the_branch_only_lacks(self): + result = self.lint("lint_file_size.sh") + output = result.stdout + result.stderr + self.assertEqual(result.returncode, 0, output) + self.assertIn("feature_only.md", result.stdout) + self.assertNotIn("big.bin", output) + + def test_lint_urls_still_catches_a_url_the_branch_adds(self): + self.add_on_feature("feature_bad.md", BAD) + result = self.lint("lint_urls.sh", **self.curl_stub()) + output = result.stdout + result.stderr + self.assertEqual(result.returncode, 1, output) + self.assertIn("example.invalid/dead", output) + + def test_lint_xrefs_still_catches_a_reference_the_branch_adds(self): + self.add_on_feature("feature_bad.md", BAD) + result = self.lint("lint_xrefs.sh") + output = result.stdout + result.stderr + self.assertEqual(result.returncode, 1, output) + self.assertIn("sub/missing.md", output) + + def test_lint_file_size_still_catches_a_file_the_branch_adds(self): + self.add_on_feature("feature_big.bin", OVERSIZED) + result = self.lint("lint_file_size.sh") + output = result.stdout + result.stderr + self.assertEqual(result.returncode, 1, output) + self.assertIn("feature_big.bin", output) + + def test_lint_urls_survives_a_colorizing_git_config(self): + config = self.repo / "colorful.gitconfig" + config.write_text("[color]\n\tui = always\n") + result = self.lint( + "lint_urls.sh", GIT_CONFIG_GLOBAL=str(config), **self.curl_stub() + ) + self.assert_scoped_to_feature(result) diff --git a/.github/workflows/_link_check.yml b/.github/workflows/_link_check.yml index a713de1c0dc..97f99b9f139 100644 --- a/.github/workflows/_link_check.yml +++ b/.github/workflows/_link_check.yml @@ -5,7 +5,9 @@ on: type: string required: true base_ref: - description: Commit to diff against. Empty, or all zeros, means check the whole tree. + description: > + Base branch commit. Pull requests lint from its merge base with ref, pushes + lint from it directly. Empty, or all zeros, means check the whole tree. type: string required: false default: '' @@ -21,16 +23,31 @@ jobs: - uses: actions/checkout@v4 with: ref: ${{ inputs.ref }} + # Only the merge base path needs history, and it costs ~25s. Quote the + # branches: bare 0 is falsy, so `&& 0 || 1` would always yield 1. + fetch-depth: ${{ github.event_name == 'pull_request' && '0' || '1' }} - name: Lint URLs env: BASE_REF: ${{ inputs.base_ref }} HEAD_REF: ${{ inputs.ref }} + FROM_MERGE_BASE: ${{ github.event_name == 'pull_request' }} run: | args=() # A push creating a tag or branch reports an all zero SHA no remote can serve. if [ -n "$BASE_REF" ] && [ "$BASE_REF" != "0000000000000000000000000000000000000000" ]; then - git fetch --no-tags --depth=1 origin "$BASE_REF" - args=("$BASE_REF" "$HEAD_REF") + if [ "$FROM_MERGE_BASE" = "true" ]; then + git fetch --no-tags origin "$BASE_REF" + # Where the branch left the base branch, not the base branch tip, or a + # branch cut before recent commits is blamed for every line it merely + # lacks. No merge base means no usable range, so leave args empty for a + # whole tree scan: the scripts treat a bad range as nothing to check. + if merge_base=$(git merge-base "$BASE_REF" "$HEAD_REF"); then + args=("$merge_base" "$HEAD_REF") + fi + else + git fetch --no-tags --depth=1 origin "$BASE_REF" + args=("$BASE_REF" "$HEAD_REF") + fi fi ./scripts/lint_urls.sh "${args[@]}" || { echo @@ -50,16 +67,31 @@ jobs: - uses: actions/checkout@v4 with: ref: ${{ inputs.ref }} + # Only the merge base path needs history, and it costs ~25s. Quote the + # branches: bare 0 is falsy, so `&& 0 || 1` would always yield 1. + fetch-depth: ${{ github.event_name == 'pull_request' && '0' || '1' }} - name: Lint cross-references env: BASE_REF: ${{ inputs.base_ref }} HEAD_REF: ${{ inputs.ref }} + FROM_MERGE_BASE: ${{ github.event_name == 'pull_request' }} run: | args=() # A push creating a tag or branch reports an all zero SHA no remote can serve. if [ -n "$BASE_REF" ] && [ "$BASE_REF" != "0000000000000000000000000000000000000000" ]; then - git fetch --no-tags --depth=1 origin "$BASE_REF" - args=("$BASE_REF" "$HEAD_REF") + if [ "$FROM_MERGE_BASE" = "true" ]; then + git fetch --no-tags origin "$BASE_REF" + # Where the branch left the base branch, not the base branch tip, or a + # branch cut before recent commits is blamed for every line it merely + # lacks. No merge base means no usable range, so leave args empty for a + # whole tree scan: the scripts treat a bad range as nothing to check. + if merge_base=$(git merge-base "$BASE_REF" "$HEAD_REF"); then + args=("$merge_base" "$HEAD_REF") + fi + else + git fetch --no-tags --depth=1 origin "$BASE_REF" + args=("$BASE_REF" "$HEAD_REF") + fi fi ./scripts/lint_xrefs.sh "${args[@]}" || { echo @@ -79,16 +111,31 @@ jobs: - uses: actions/checkout@v4 with: ref: ${{ inputs.ref }} + # Only the merge base path needs history, and it costs ~25s. Quote the + # branches: bare 0 is falsy, so `&& 0 || 1` would always yield 1. + fetch-depth: ${{ github.event_name == 'pull_request' && '0' || '1' }} - name: Lint file sizes env: BASE_REF: ${{ inputs.base_ref }} HEAD_REF: ${{ inputs.ref }} + FROM_MERGE_BASE: ${{ github.event_name == 'pull_request' }} run: | args=() # A push creating a tag or branch reports an all zero SHA no remote can serve. if [ -n "$BASE_REF" ] && [ "$BASE_REF" != "0000000000000000000000000000000000000000" ]; then - git fetch --no-tags --depth=1 origin "$BASE_REF" - args=("$BASE_REF" "$HEAD_REF") + if [ "$FROM_MERGE_BASE" = "true" ]; then + git fetch --no-tags origin "$BASE_REF" + # Where the branch left the base branch, not the base branch tip, or a + # branch cut before recent commits is blamed for every line it merely + # lacks. No merge base means no usable range, so leave args empty for a + # whole tree scan: the scripts treat a bad range as nothing to check. + if merge_base=$(git merge-base "$BASE_REF" "$HEAD_REF"); then + args=("$merge_base" "$HEAD_REF") + fi + else + git fetch --no-tags --depth=1 origin "$BASE_REF" + args=("$BASE_REF" "$HEAD_REF") + fi fi chmod +x ./scripts/lint_file_size.sh ./scripts/lint_file_size.sh "${args[@]}" || { diff --git a/scripts/lint_file_size.sh b/scripts/lint_file_size.sh index ec2256453fa..a579254c936 100644 --- a/scripts/lint_file_size.sh +++ b/scripts/lint_file_size.sh @@ -40,8 +40,8 @@ is_exception() { if [ $# -eq 2 ]; then base=$1 head=$2 - echo "Checking changed files between $base..$head" - files=$(git diff --name-only "$base..$head") + echo "Checking changed files between $base...$head" + files=$(git diff --no-color --name-only "$base...$head") else echo "Checking all files in repository" files=$(git ls-files) diff --git a/scripts/lint_urls.sh b/scripts/lint_urls.sh index 92b57171bc1..a49a52b011a 100755 --- a/scripts/lint_urls.sh +++ b/scripts/lint_urls.sh @@ -70,8 +70,10 @@ done < <( ':(exclude,glob)**/third_party/**' ) if [ $# -eq 2 ]; then - for filename in $(git diff --name-only --unified=0 "$1..$2"); do - git diff --unified=0 "$1..$2" -- "$filename" "${excludes[@]}" \ + # Three dots, not two. Against the base branch tip, a branch cut before recent + # commits looks like it is adding back every line those commits touched. + for filename in $(git diff --no-color --name-only --unified=0 "$1...$2"); do + git diff --no-color --unified=0 "$1...$2" -- "$filename" "${excludes[@]}" \ | grep -E '^\+' \ | grep -Ev '^\+\+\+' \ | perl -nle 'print for m#'"$pattern"'#g' \ diff --git a/scripts/lint_xrefs.sh b/scripts/lint_xrefs.sh index 54917b26d8e..4845da0d27f 100755 --- a/scripts/lint_xrefs.sh +++ b/scripts/lint_xrefs.sh @@ -35,8 +35,10 @@ done < <( ':(exclude,glob)**/third_party/**' ) if [ $# -eq 2 ]; then - for filename in $(git diff --name-only --unified=0 "$1..$2"); do - git diff --unified=0 "$1..$2" -- "$filename" "${excludes[@]}" \ + # Three dots, not two. Against the base branch tip, a branch cut before recent + # commits looks like it is adding back every line those commits touched. + for filename in $(git diff --no-color --name-only --unified=0 "$1...$2"); do + git diff --no-color --unified=0 "$1...$2" -- "$filename" "${excludes[@]}" \ | grep -E '^\+' \ | grep -Ev '^\+\+\+' \ | perl -nle 'print for m#'"$pattern"'#g' \