lint: Fix COMMIT_RANGE issues #29660

1. 
   maflcko commented at 1:49 pm on March 15, 2024: member

   COMMIT_RANGE has problems on forks or local branches:

   • When LOCAL_BRANCH is set, it assumes the presence of a master branch, and that the master branch is up-to-date. Both of which may be false. (See also discussion in #29274 (review))
   • When COMMIT_RANGE isn’t set in lint-git-commit-check.py, and --prev-commits isn’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 drop SKIP_EMPTY_NOT_A_PR, because scripts will already skip an empty range. Finally, it allows to drop --prev-commits n, because one can simply say COMMIT_RANGE='HEAD~n..HEAD' to achieve the same.

2. 
   DrahtBot commented at 1:49 pm on March 15, 2024: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	Sjors

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

3. DrahtBot added the label Tests on Mar 15, 2024
4. maflcko force-pushed on Mar 15, 2024
5. DrahtBot added the label CI failed on Mar 15, 2024
6. 
   DrahtBot commented at 1:53 pm on March 15, 2024: contributor

   🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

   Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

   Leave a comment here, if you need help tracking down a confusing failure.

   Debug: https://github.com/bitcoin/bitcoin/runs/22707944823

7. DrahtBot removed the label CI failed on Mar 15, 2024
8. fanquake requested review from Sjors on Mar 19, 2024
9. 
   Sjors commented at 5:53 pm on March 19, 2024: member

   Where is lint-git-commit-check.py called from?
10. 
    maflcko commented at 6:01 pm on March 19, 2024: member

    The lint-*.py scripts were always called by iterating over all python files in the folder and executing any that match the lint-*.py pattern. They are called by the lint test_runner.
11. in ci/lint/06_script.sh:11 in fa61600367 outdated

     7@@ -8,21 +8,19 @@ export LC_ALL=C
     8 
     9 set -ex
    10 
    11-if [ -n "$LOCAL_BRANCH" ]; then
    12-  # To faithfully recreate CI linting locally, specify all commits on the current
    13-  # branch.
    14-  COMMIT_RANGE="$(git merge-base HEAD master)..HEAD"
    15-elif [ -n "$CIRRUS_PR" ]; then
    16+if [ -n "$CIRRUS_PR" ]; then
    

    ---

    ---

    

    Sjors commented at 11:09 am on March 21, 2024:

    Maybe explain why we want no more than 2 commits for a PR here. It seems incorrect for commit-script-check.sh.

    ---

    

    maflcko commented at 11:38 am on March 21, 2024:

    The Cirrus stuff is explained in .cirrus.yml, just hop to COMMIT_RANGE in the file.

    HEAD~ refers to the commit prior to the pull request merge commit and HEAD to the pull request merge commit. Thus, HEAD~..HEAD, means “all changes in the pull request”.

    ---

    

    Sjors commented at 12:45 pm on March 21, 2024:

    Ah, a useful comment could be:

    0# Cirrus jobs are run on the 'pull/${CIRRUS_PR}/merge' branch
    

    And for safety against regressions maybe add:

    0if ! git show --summary HEAD | grep -q ^Merge; then
    1  echo "Error: expected a merge commit"
    2  exit 1;
    3fi 
    

    ---

    

    Sjors commented at 12:56 pm on March 21, 2024:

    (could also check for the presence of HEAD^2)

    ---

    

    maflcko commented at 7:16 pm on March 21, 2024:

    (could also check for the presence of HEAD^2)

    Why?

    ---

    

    maflcko commented at 7:19 pm on March 21, 2024:

    And for safety against regressions maybe add:

    Thanks, done.

    ---

    

    Sjors commented at 9:30 pm on March 21, 2024:

    I mean checking HEAD^2 could be an alternative for looking for a commit message regex. I see you found another approach.

    ---

    

    maflcko commented at 8:31 am on March 22, 2024:

    I still don’t understand how the presence of HEAD^2 says anything. HEAD^2 isn’t valid syntax. Locally I just get fatal: ambiguous argument 'HEAD^2': unknown revision or path not in the working tree.. Maybe you meant HEAD~2? However, this will either be another merge commit, or a commit in the PR, depending on how the merge was done. So I am not sure what to check.

    ---

    

    Sjors commented at 8:56 am on March 22, 2024:

    git show HEAD^2 works if you’re on a merge commit. It shows the second parent. It fails otherwise, because non-merge commits only have one parent: https://gist.github.com/zhangzhhz/77850a5d6d64568b5ea50ffc7b6c57ce

    ---

    

    Sjors commented at 8:57 am on March 22, 2024:

    (but your solution is more readable)

    ---

    

    maflcko commented at 9:12 am on March 22, 2024:

    Ah, I see now. May change if I have to re-touch for other reasons.
12. 
    Sjors commented at 11:20 am on March 21, 2024: member

    I tested this on top of #29274 in https://github.com/Sjors/bitcoin/pull/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:

    • https://cirrus-ci.com/task/6752324784226304
    • https://cirrus-ci.com/task/4594589848305664

    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.

13. lint: Fix COMMIT_RANGE issues fa1146d01b
14. maflcko force-pushed on Mar 21, 2024
15. 
    Sjors commented at 9:00 am on March 22, 2024: member

    tACK fa1146d01b148dd60fcada36a3b37ed37532ce2b

    Demonstration that the git-commit-check linter still works: https://cirrus-ci.com/task/6732599670865920?logs=lint#L746

    It also allows to drop SKIP_EMPTY_NOT_A_PR, because scripts will already skip an empty range.

    When called from 06_script.sh then COMMIT_RANGE is never empty I think? I didn’t test how this behaves with (not PR) branch pushes.

16. 
    maflcko commented at 9:10 am on March 22, 2024: member

    When called from 06_script.sh then COMMIT_RANGE is never empty I think? I didn’t test how this behaves with (not PR) branch pushes.

    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 SKIP_EMPTY_NOT_A_PR.

17. maflcko requested review from fanquake on Mar 25, 2024
18. fanquake merged this on Mar 25, 2024
19. fanquake closed this on Mar 25, 2024

    ---

20. maflcko deleted the branch on Mar 25, 2024

Contributors
maflcko DrahtBot Sjors

Review Requested
fanquake

Labels
Tests