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' \