lint: Fix COMMIT_RANGE issues #29660
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. |
faf9691 to
fa61600
Compare
|
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
|
Where is |
|
The |
There was a problem hiding this comment.
I tested this on top of #29274 in Sjors#30. My fork has an out of date master branch, and the pull request isn't against the master branch. So that seems like a good test case.
IIUC this PR only impacts the Cirrus linter job, which is not running self-hosted on forks, with or without #29274 (not sure why that is, but it's not a big deal).
I pushed once with a single commit and then again with a second commit. The linter passed:
The CI log shows the right commit(s) for git log --no-merges --oneline HEAD~..HEAD.
I don't think I've ever used LOCAL_BRANCH. I tested locally on macOS 14.4 building the rust lint test_runner and calling it with COMMIT_RANGE="HEAD~..HEAD" which seems to work.
fa61600 to
fa1146d
Compare
|
tACK fa1146d Demonstration that the git-commit-check linter still works: https://cirrus-ci.com/task/6732599670865920?logs=lint#L746
When called from |
It may be empty on non-pull requests, when the top commit on the branch is a merge commit. This is preserving the behavior of |
COMMIT_RANGEhas problems on forks or local branches:LOCAL_BRANCHis set, it assumes the presence of amasterbranch, and that themasterbranch is up-to-date. Both of which may be false. (See also discussion in Fix issues with CI on forks #29274 (comment))COMMIT_RANGEisn't set inlint-git-commit-check.py, and--prev-commitsisn't set either, it has the same (broken) assumptions.Fix all issues by simply assuming a merge commit exists. This allows to drop
LOCAL_BRANCH. It also allows to dropSKIP_EMPTY_NOT_A_PR, because scripts will already skip an empty range. Finally, it allows to drop--prev-commits n, because one can simply sayCOMMIT_RANGE='HEAD~n..HEAD'to achieve the same.