Skip to content

ci: fetch only the number of commits needed for checks#73

Merged
DeepDiver1975 merged 1 commit into
mainfrom
adjust-commit-history
Jul 2, 2026
Merged

ci: fetch only the number of commits needed for checks#73
DeepDiver1975 merged 1 commit into
mainfrom
adjust-commit-history

Conversation

@phil-davis

@phil-davis phil-davis commented Jun 29, 2026

Copy link
Copy Markdown
Contributor

Fixes #55

I discovered that you can't just do:

fetch-depth: ${{ github.event.pull_request.commits + 1 }}

Calculations do not work there.

You have to do a whole little job before the real job, which gets the number of commits in the PR, does the calculation, and passes it to the next real job.

Do we care enough to add this extra code?

@phil-davis phil-davis force-pushed the adjust-commit-history branch 2 times, most recently from c9dc3b3 to 77c6f10 Compare June 29, 2026 05:50
@phil-davis phil-davis force-pushed the adjust-commit-history branch from 77c6f10 to cd32e5c Compare June 29, 2026 05:52
@phil-davis

Copy link
Copy Markdown
Contributor Author

@DeepDiver1975 DeepDiver1975 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed as a maintainer — the approach here is correct, and I want to confirm the key worry from the description is handled.

No invalid Actions arithmetic. ${{ github.event.pull_request.commits + 1 }} indeed doesn't work (Actions expressions have no +). This PR sidesteps that cleanly: the calculate-depth job substitutes the plain integer ${{ github.event.pull_request.commits }} into a shell variable, does the math in bash ($((PR_COMMITS + 1)), valid), and passes it on via $GITHUB_OUTPUTfetch-depth: ${{ needs.calculate-depth.outputs.calculated_depth }}. 👍

+1 is right — no off-by-one. Validating the PR's N commits requires fetching those N commits plus their merge-base parent, so N+1 is the correct depth (a bare N would risk truncating the range). Keeping the +1 is the safe choice. The empty/non-PR fallback to depth 1 is sensible.

CI proves it end-to-end: Calculate commits in the PR and Validate semantic commits both ran under the new N+1 logic and passed — so the semantic-commit check still has enough history. All checks green, MERGEABLE.

On your question — "Do we care enough to add this extra code?" — the extra job is modest and the payoff (avoiding fetch-depth: 0 full-history clones on every PR) is real, so I think it's worth it. One minor thought, no change requested: the calculate-depth job is duplicated across semantic-git-message.yml and translation-sync.yml; if it spreads further, a tiny composite/reusable step could DRY it up — but two copies is fine for now.

Changelog: this is an internal CI-workflow optimization (no behavior change in the reusable workflows themselves), and recent infra PRs here didn't carry a changelog/unreleased/ fragment, so I don't think one is required — your call.

LGTM. Leaving the merge to you since it's your PR and you raised an open design question in the description.

🤖 Generated with Claude Code

@phil-davis phil-davis requested a review from DeepDiver1975 June 29, 2026 08:04
@DeepDiver1975 DeepDiver1975 merged commit 353376d into main Jul 2, 2026
22 checks passed
@DeepDiver1975 DeepDiver1975 deleted the adjust-commit-history branch July 2, 2026 20:15
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.

Is full history needed in semantic git message workflow?

3 participants