Fix link check on pushes that have no base commit (#21644)

## Problem

`.github/workflows/_link_check.yml` decides which commits to diff by
inspecting the event that triggered its *caller*. That is the wrong
place to make the decision. A reusable workflow is called from more than
one context, and the value it reaches for is not present in all of them.

For a pull request it uses the pull request base. For everything else it
uses `github.event.before`, which is only meaningful for a push to a
branch that already existed.

| trigger | `github.event.before` | before this change |
| --- | --- | --- |
| pull request | not used, PR base instead | changed lines checked |
| push to an existing branch | previous branch tip | changed lines
checked |
| push creating a tag | all zeros | job fails, exit 128 |
| push creating a branch | all zeros | job fails, exit 128 |
| scheduled run | absent | whole tree checked, by accident |
| manual dispatch | absent | whole tree checked, by accident |

The all-zero rows fail outright, which is what every `ciflow/nightly/*`
tag push hits:

```
git fetch --no-tags --depth=1 origin 0000000000000000000000000000000000000000
fatal: remote error: upload-pack: not our ref 0000000000000000000000000000000000000000
##[error]Process completed with exit code 128
```

The two "by accident" rows are worth spelling out, because the behavior
there is correct but nothing makes it so. With `before` absent, the
fetch becomes `git fetch --no-tags --depth=1 origin ""`, which quietly
succeeds by fetching the default head. Then in

```yaml
./scripts/lint_urls.sh $(
  ...
  echo "${{ github.event.before }}" "${{ github.sha }}"
)
```

the unquoted command substitution word splits the empty first field
away, so the script gets one argument rather than two, fails its `[ $#
-eq 2 ]` test, and falls through to whole tree mode. The right thing
happens for the wrong reason, and only for as long as that substitution
stays unquoted.

## Fix

Take the base as an input instead:

```yaml
      base_ref:
        description: Commit to diff against. Empty, or all zeros, means check the whole tree.
        type: string
        required: false
        default: ''
```

Empty means there is nothing to diff against. The three lint scripts
already support that: given two arguments they diff a range, given none
they scan the whole tree. So the empty case needs no fallback logic, it
just calls them with no arguments. The all-zero SHA maps to empty,
because a caller forwarding `github.event.before` has no way to avoid
producing it.

With the base known up front, the separate `Fetch base ref` step has
nothing left to decide, so it folds into the lint step. Each job loses a
step and the file gets shorter, 107 lines to 100:

```yaml
          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")
          fi
          ./scripts/lint_urls.sh "${args[@]}" || {
```

`lint.yml` passes the pull request base on pull requests and
`github.event.before` otherwise, one added line.

The job level `if:` conditions still read `github.event_name` and the
pull request labels. Those decide whether a job runs at all, which is a
different question and out of scope here. What changes is how the diff
base is computed.

One incidental correctness gain: the old steps checked out `inputs.ref`
but diffed against `github.sha` or `head.sha` read from the caller's
event. Now the checked out commit and the diff head are the same input,
so they cannot drift apart. That mismatch was not hypothetical, see
below.

## What `nightly.yml` stops running, and why that is also a fix

`nightly.yml` now runs the check only on the schedule and on a manual
dispatch. That drops two triggers: pull requests touching
`.github/workflows/nightly.yml`, and `ciflow/nightly/*` tag pushes.

**The pull request trigger was not providing coverage.** `nightly.yml`
passes `ref: ${{ github.sha }}`, which on a pull request is the merge
commit. The head commit is therefore never checked out and never
fetched, so `git diff base..head` cannot resolve:

```
Checking changed files between bcceab1..7e66c47
fatal: Invalid revision range bcceab1..7e66c47
```

`lint_urls.sh` and `lint_xrefs.sh` consume that diff through a process
substitution, so the failure is swallowed and they report success having
checked zero URLs. `lint_file_size.sh` reads it into a plain assignment,
which `set -e` turns into a hard failure, and there is no
`skip-file-size-lint` label to escape it. Run 31125514863 shows all
three: two green jobs that examined nothing, and one red one.

So this removes two no-ops and one permanently red, unskippable check.
Pull requests keep the real check from `lint.yml`, against their own
base, on the path where the head commit is actually checked out.

**The tag push trigger is a policy call, not a bug fix.** To be precise
about causation: the exit 128 there is removed by the zero-SHA guard
plus the empty default, not by the `if:`. Left alone, a
`ciflow/nightly/*` push would now run a clean whole tree scan. It is
skipped because there were 19 such tag pushes in the last day alone, a
whole tree URL scan takes about six minutes, and it would be red on the
pre-existing dead links every time.

**One narrow gap remains.** A pull request targeting the `nightly`
branch gets no link check at all, since `lint.yml` excludes that branch
through `branches-ignore`. Before this change it got the two no-ops and
the red file-size job, so nothing that worked is lost. Happy to add a
correct `pull_request` arm to `nightly.yml` if reviewers would rather
close it.

## Result

| trigger | after this change |
| --- | --- |
| pull request, via `lint.yml` | changed lines only, unchanged |
| push to `main` or an existing `release/*` | changed lines only,
unchanged |
| push creating a `release/*` branch | whole tree, instead of exit 128 |
| push of a `ciflow/nightly/*` tag | skipped, instead of exit 128 |
| pull request touching `nightly.yml` | skipped, was two no-ops and one
red job |
| scheduled nightly | whole tree, unchanged, now by design |
| manual dispatch, either workflow | whole tree, unchanged |

`lint.yml` declares no `tags:` in its push trigger, so the tag case only
ever reached `_link_check.yml` through `nightly.yml`.

## Testing

- All three workflow files parse, and every `run` block in
`_link_check.yml` is clean under `shellcheck -S style`.
- Ran the rewritten lint step as a standalone script for every shape of
input, across all three jobs: a real base SHA fetches and passes two
arguments; an all-zero SHA and an empty value skip the fetch and pass
none; an unservable base exits 128 with git's own message and never
reaches the lint script. Repeated with and without `set -u`, and with
stray positional parameters already set, so the argument list cannot
depend on how the runner invokes the step.
- Ran `scripts/lint_file_size.sh` with no arguments to confirm the whole
tree path works end to end: 9033 files checked, no failures.
- The whole tree paths of `lint_urls.sh` and `lint_xrefs.sh` use `git
grep -P`, which needs a git built with PCRE support that I did not have
locally, so I did not run those myself. They are already exercised in CI
though, by the current scheduled nightly, which reaches whole tree mode
through the accident described above. Its most recent `lint-urls` log
checks about 2148 URLs and reports 2126 OK, 15 WARN, 7 FAIL. Those 7 are
genuine dead links, unrelated to this change, and it neither fixes nor
hides them, so the scheduled job stays red until they are updated.
- On this pull request, `Lint` runs the changed workflows against
themselves and all three link check jobs pass with `BASE_REF` and
`HEAD_REF` taken from the inputs. The `nightly` run on the
`ciflow/nightly` tag, the case that used to exit 128, is skipped.

## Not addressed here

`lint_urls.sh` and `lint_xrefs.sh` end their input pipeline with `||
true`, needed because `git grep` exits 1 when it finds nothing. It also
means that if `git grep` fails outright the lint reports zero findings
and passes. That is the same swallowing described above, it is
pre-existing, and it deserves its own change.

Author: shoumikhin

.github/workflows/_link_check.yml

@@ -4,6 +4,11 @@ on:
       ref:
         type: string
         required: true
+      base_ref:
+        description: Commit to diff against. Empty, or all zeros, means check the whole tree.
+        type: string
+        required: false
+        default: ''
 
 jobs:
   lint-urls:
@@ -16,22 +21,18 @@ jobs:
       - uses: actions/checkout@v4
         with:
           ref: ${{ inputs.ref }}
-      - name: Fetch base ref
-        run: |
-          if [ "${{ github.event_name }}" = "pull_request" ]; then
-            git fetch --no-tags --depth=1 origin "${{ github.event.pull_request.base.sha }}"
-          else
-            git fetch --no-tags --depth=1 origin "${{ github.event.before }}"
-          fi
       - name: Lint URLs
+        env:
+          BASE_REF: ${{ inputs.base_ref }}
+          HEAD_REF: ${{ inputs.ref }}
         run: |
-          ./scripts/lint_urls.sh $(
-            if [ "${{ github.event_name }}" = "pull_request" ]; then
-              echo "${{ github.event.pull_request.base.sha }}" "${{ github.event.pull_request.head.sha }}"
-            else
-              echo "${{ github.event.before }}" "${{ github.sha }}"
-            fi
-          ) || {
+          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")
+          fi
+          ./scripts/lint_urls.sh "${args[@]}" || {
             echo
             echo "URL lint failed."
             echo "If this is a transient outage, you can bypass it by adding the \`skip-url-lint\` label to your PR."
@@ -49,22 +50,18 @@ jobs:
       - uses: actions/checkout@v4
         with:
           ref: ${{ inputs.ref }}
-      - name: Fetch base ref
-        run: |
-          if [ "${{ github.event_name }}" = "pull_request" ]; then
-            git fetch --no-tags --depth=1 origin "${{ github.event.pull_request.base.sha }}"
-          else
-            git fetch --no-tags --depth=1 origin "${{ github.event.before }}"
-          fi
       - name: Lint cross-references
+        env:
+          BASE_REF: ${{ inputs.base_ref }}
+          HEAD_REF: ${{ inputs.ref }}
         run: |
-          ./scripts/lint_xrefs.sh $(
-            if [ "${{ github.event_name }}" = "pull_request" ]; then
-              echo "${{ github.event.pull_request.base.sha }}" "${{ github.event.pull_request.head.sha }}"
-            else
-              echo "${{ github.event.before }}" "${{ github.sha }}"
-            fi
-          ) || {
+          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")
+          fi
+          ./scripts/lint_xrefs.sh "${args[@]}" || {
             echo
             echo "Xref lint failed."
             echo "If this is a transient outage, you can bypass it by adding the \`skip-xref-lint\` label to your PR."
@@ -82,23 +79,19 @@ jobs:
       - uses: actions/checkout@v4
         with:
           ref: ${{ inputs.ref }}
-      - name: Fetch base ref
-        run: |
-          if [ "${{ github.event_name }}" = "pull_request" ]; then
-            git fetch --no-tags --depth=1 origin "${{ github.event.pull_request.base.sha }}"
-          else
-            git fetch --no-tags --depth=1 origin "${{ github.event.before }}"
-          fi
       - name: Lint file sizes
+        env:
+          BASE_REF: ${{ inputs.base_ref }}
+          HEAD_REF: ${{ inputs.ref }}
         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")
+          fi
           chmod +x ./scripts/lint_file_size.sh
-          ./scripts/lint_file_size.sh $(
-            if [ "${{ github.event_name }}" = "pull_request" ]; then
-              echo "${{ github.event.pull_request.base.sha }}" "${{ github.event.pull_request.head.sha }}"
-            else
-              echo "${{ github.event.before }}" "${{ github.sha }}"
-            fi
-          ) || {
+          ./scripts/lint_file_size.sh "${args[@]}" || {
             echo
             echo "File size lint failed: some files exceed the 1 MB limit."
             echo "If you really need large files, consider using Git LFS or storing them elsewhere."

.github/workflows/lint.yml

@@ -125,3 +125,4 @@ jobs:
     uses: ./.github/workflows/_link_check.yml
     with:
       ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.head.sha || github.sha }}
+      base_ref: ${{ github.event_name == 'pull_request' && github.event.pull_request.base.sha || github.event.before }}

.github/workflows/nightly.yml

@@ -32,6 +32,8 @@ jobs:
           pytorchbot-token: ${{ secrets.GH_PYTORCHBOT_TOKEN }}
 
   link-check:
+    # Whole tree scans only. Pull requests get the same check from lint.yml.
+    if: ${{ github.event_name == 'schedule' || github.event_name == 'workflow_dispatch' }}
     needs: update-pytorch-commit-hash
     uses: ./.github/workflows/_link_check.yml
     with: