COMMIT_RANGE variable for non-PRs
#20728
This PR provides the COMMIT_RANGE environment variable, which is used in lint-git-commit-check.py and lint-whitespace.py, for non-PR cases, including our release branches and personal repos.
Fixes https://github.com/bitcoin/bitcoin/runs/8664441400
Has been split out from bitcoin/bitcoin#20697 as requested by MarcoFalke.
🕵️ @practicalswift has been requested to review this pull request as specified in the REVIEWERS file.
Reopened due to the recent message on IRC:
02:52 <DavidBakin> so I am now running CI tests on a branch on my fork.
lintfails inlint-git-commit-check.pywith “Not a valid object name master” - is this something I did in my fork when creating my branch? or what? there isn’t any other useful information printed here, e.g., commit hash. I’ll try to run it locally to see if I can debug it but if you happen to know …
Also rebased.
Doesn’t this also affect non-master branches in our repo?
Doesn’t this also affect non-master branches in our repo?
Not sure. If it does, that should be in the PR description, and the primary reason for making this change.
9@@ -10,6 +10,8 @@ GIT_HEAD=$(git rev-parse HEAD)
10 if [ -n "$CIRRUS_PR" ]; then
11 COMMIT_RANGE="${CIRRUS_BASE_SHA}..$GIT_HEAD"
12 test/lint/commit-script-check.sh "$COMMIT_RANGE"
13+else
14+ COMMIT_RANGE="$(git fetch "$CIRRUS_REPO_CLONE_URL" "${CIRRUS_LAST_GREEN_CHANGE:-$CIRRUS_DEFAULT_BRANCH}" && git merge-base FETCH_HEAD HEAD)..$GIT_HEAD"
master?
I don’t know the code changes needed, but it seems the check currently only runs on pull requests against the base commit.
On master it is presumably skipped because the base commit is equal to the HEAD commit?
So maybe all places where COMMIT_RANGE is used can be guarded by a check on CIRRUS_PR? Or instead skip if the range is empty?
it seems the check currently only runs on pull requests against the base commit.
ci/lint/06_script.sh runs always except cases defined in the FILTER_TEMPLATE.
On master it is presumably skipped because the base commit is equal to the HEAD commit?
According to Cirrus CI docs, the CIRRUS_BASE_SHA variable is defined for PRs only. Therefore, it is natural to have distinct code paths to evaluate COMMIT_RANGE for PRs and non-PRs.
So maybe all places where
COMMIT_RANGEis used can be guarded by a check onCIRRUS_PR?
Given COMMIT_RANGE is used in two python scripts, it’s okay to evaluate it before test/lint/lint-all.py. That is the case for the current ci/lint/06_script.sh file.
Or instead skip if the range is empty?
I cannot figure out such cases.
lint-git-commit-check.py for master and release branches, no?
Well, for a pull request it is trivial to see which commits to check. For a branch not so.
Does it check all commits …
HEAD and the last green push if available or, otherwise, the default branch. I believe, such an approach won’t leave un-linted commits in the commit history.
Why would the default branch be available on cirrus if the ci runs on another branch?
An alternative would be to run it from the last merge git log --merges -1
0fatal: Not a valid object name master
The CIRRUS_DEFAULT_BRANCH is being used in the git fetch command, which should succeed regardless of the current branch in the git tree.
CI fails in the 24.x branch because the COMMIT_RANGE variable is not set at all, and the failed python scripts use the hardcoded “master” name of a branch. For example: https://github.com/bitcoin/bitcoin/blob/ba48fcf4a40c5b9888459511fb4233a1b89184cc/test/lint/lint-whitespace.py#L92-L101
I believe, such an approach won’t leave un-linted commits in the commit history.
yes, maybe.
However, I still don’t understand why this is needed.
If you run the linters on a branch, it is too late, because force pushing is not allowed.
For example, it is not possible to rewrite the commit subject line if test/lint/lint-git-commit-check.py fails on a commit in a non-pull-request branch. (Same for whitespace from test/lint/lint-whitespace.py, or the eval from test/lint/commit-script-check.sh). There are no other uses of COMMIT_RANGE other than those 3.
verify-commits.py on branches, I would have already completely disabled the linters for branches.
If you run the linters on a branch, it is too late, because force pushing is not allowed.
For example, it is not possible to rewrite the commit subject line if
test/lint/lint-git-commit-check.pyfails on a commit in a non-pull-request branch. (Same for whitespace fromtest/lint/lint-whitespace.py, or the eval fromtest/lint/commit-script-check.sh). There are no other uses ofCOMMIT_RANGEother than those 3.
There is still a use case when running CI in personal repos though.
I would have already completely disabled the linters for branches.
That sounds reasonable. But again, this change allows to use the linter CI task in personal repos.
Can you explain that use case? It seems fragile to support that, given that CIRRUS_LAST_GREEN_CHANGE might “accidentally” be green for “broken” commits (for example if the CI was skipped, etc…). Or CIRRUS_DEFAULT_BRANCH might be too far ahead, so that commits are “skipped”.
So to summarize, I fail to see the use case, and even if there was a use case, it would be impossible to support it.
COMMIT_RANGE="EMPTY" and then use that to skip the affected linters
Mind having another look into this PR?
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Concept ACK | decryp2kanon, jonatack |