Skip to content

fix(ci): use full git history on PRs and fail loudly on diff errors#3

Merged
jlc488 merged 1 commit into
mainfrom
fix/ci-pr-fetch-depth-and-loud-diff
May 23, 2026
Merged

fix(ci): use full git history on PRs and fail loudly on diff errors#3
jlc488 merged 1 commit into
mainfrom
fix/ci-pr-fetch-depth-and-loud-diff

Conversation

@jlc488
Copy link
Copy Markdown
Contributor

@jlc488 jlc488 commented May 23, 2026

Summary

Two bugs in the CI workflow from #1 made path-filtered builds silently skip on every PR. Discovered while opening #2 — the matrix went empty and the build job was reported as "skipping" even though `easy-paging-demo/` was clearly the changed directory.

Two bugs, one symptom

Bug 1 — GitHub Actions ternary trap on `fetch-depth`

```yaml
fetch-depth: ${{ github.event_name == 'pull_request' && 0 || 1 }}
```

The intent was "0 (full history) on PRs, 1 (shallow) otherwise". But in Actions expressions, `0` is falsy, so `0 || 1` short-circuits to `1`. The expression evaluates to `1` on both PRs and main pushes. PR checkouts had no history beyond the merge commit.

Bug 2 — silent failure on `git diff`

```bash
mapfile -t changed_files < <(git diff --name-only "$base" HEAD)
```

With bug 1 in effect, `git diff` exited with `fatal: bad object` because the base SHA was never fetched. But process substitution does not propagate the subshell's exit status under `set -e` — so the script kept going with an empty `changed_files` array. The matrix went empty. The build job was marked as "skipping". CI looked green but had built nothing.

Fix

  • `fetch-depth: 0` unconditionally. Cost is negligible for this repo and removes the ternary footgun entirely. A code comment now flags the trap so the next person doesn't reintroduce it.
  • Two-stage `git diff` with explicit error check, so `set -e` actually catches a non-zero exit. `::error::` annotation points at the likely cause (shallow checkout) for the next debugger.

Verification

Will be confirmed when #2 is rebased on top of this fix:

  • On that PR's CI run, the `detect` job should output `demos: ["easy-paging-demo"]` and `any: true`.
  • The `build (easy-paging-demo)` matrix job should actually run `./gradlew build` rather than skipping.

Cannot be verified by this PR's own CI run — this PR doesn't touch any demo directory, so the matrix will (correctly) be empty and `build` will (correctly) skip. The fix only takes effect when there's a demo change to detect.

Test plan

  • CI on this PR: `detect` job passes (with the new defensive diff handling), `build` skipped (no demos changed — expected)
  • After merge + rebase of feat: add easy-paging-demo (first runnable example) #2: `detect` job emits `easy-paging-demo` in the matrix, `build` job runs end-to-end and turns green

Two bugs in the workflow from #1 conspired to make path-filtered builds
silently skip on every PR. Discovered while opening #2, the first demo PR,
where the build job was reported as 'skipping' even though
easy-paging-demo/ was clearly the changed directory.

Bug 1 — GitHub Actions ternary trap on fetch-depth
    fetch-depth: ${{ github.event_name == 'pull_request' && 0 || 1 }}
The intent was "0 (full history) on PRs, 1 (shallow) otherwise". But in
Actions expressions, `0` is falsy, so `0 || 1` short-circuits to `1`.
The expression evaluated to `1` on both PRs and main pushes, leaving PR
checkouts with no history beyond the merge commit.

Bug 2 — silent failure on git diff
    mapfile -t changed_files < <(git diff --name-only "$base" HEAD)
With bug 1 in effect, `git diff` exited with "fatal: bad object" because
the base SHA wasn't fetched. But process substitution does not propagate
the subshell's exit status under `set -e`, so the script kept going with
an empty `changed_files` array — and therefore an empty matrix and a
"skipped" build job. CI looked green, but it had built nothing.

Fix:
- fetch-depth: 0 unconditionally. Cost is negligible for this repo and
  removes the ternary footgun entirely.
- Run git diff in a separate statement so `set -e` actually catches a
  non-zero exit, with an explicit ::error:: pointer to the likely cause
  (shallow checkout) so the next person debugging this gets a head start.

Verification will happen on the next PR (the rebase of #2) — the matrix
should populate with easy-paging-demo and the build job should actually
execute ./gradlew build instead of skipping.
@jlc488 jlc488 merged commit f506fd1 into main May 23, 2026
3 checks passed
@jlc488 jlc488 deleted the fix/ci-pr-fetch-depth-and-loud-diff branch May 23, 2026 05:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant