From fa673cf3449f4e71501814bf99c2e2bbb49b8fcb Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Thu, 23 Jan 2025 10:38:50 +0100 Subject: [PATCH 1/3] lint: Call lint_scripted_diff from test_runner Allowing to call the check from the test_runner allows for consistent error messages and better UX by having a single test_runner for all checks. This requires the env var to be set for now. The next commit makes the commit range optional. --- ci/lint/06_script.sh | 1 - test/lint/test_runner/src/main.rs | 23 +++++++++++++++++++++++ 2 files changed, 23 insertions(+), 1 deletion(-) diff --git a/ci/lint/06_script.sh b/ci/lint/06_script.sh index cdf0f60147d..2824356506b 100755 --- a/ci/lint/06_script.sh +++ b/ci/lint/06_script.sh @@ -25,7 +25,6 @@ export COMMIT_RANGE echo git log --no-merges --oneline "$COMMIT_RANGE" echo -test/lint/commit-script-check.sh "$COMMIT_RANGE" RUST_BACKTRACE=1 "${LINT_RUNNER_PATH}/test_runner" if [ "$CIRRUS_REPO_FULL_NAME" = "bitcoin/bitcoin" ] && [ "$CIRRUS_PR" = "" ] ; then diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index 8479fd2d64a..36f82f14b1a 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -68,6 +68,11 @@ fn get_linter_list() -> Vec<&'static Linter> { name: "subtree", lint_fn: lint_subtree }, + &Linter { + description: "Check scripted-diffs", + name: "scripted_diff", + lint_fn: lint_scripted_diff + }, &Linter { description: "Check that tabs are not used as whitespace", name: "tabs_whitespace", @@ -173,6 +178,11 @@ fn get_git_root() -> PathBuf { PathBuf::from(check_output(git().args(["rev-parse", "--show-toplevel"])).unwrap()) } +/// Return the commit range, or panic +fn commit_range() -> String { + env::var("COMMIT_RANGE").unwrap() +} + /// Return all subtree paths fn get_subtrees() -> Vec<&'static str> { vec![ @@ -210,6 +220,19 @@ fn lint_subtree() -> LintResult { } } +fn lint_scripted_diff() -> LintResult { + if Command::new("test/lint/commit-script-check.sh") + .arg(commit_range()) + .status() + .expect("command error") + .success() + { + Ok(()) + } else { + Err("".to_string()) + } +} + fn lint_py_lint() -> LintResult { let bin_name = "ruff"; let checks = format!( From fa99728b0c8b3cac7056fa554fab7a8a4624a2de Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 14 Jan 2025 14:02:26 +0100 Subject: [PATCH 2/3] lint: Move commit range printing to test_runner Having a single test_runner for all logic improves the consistency and UX. --- ci/lint/06_script.sh | 13 ++----------- test/lint/test_runner/src/main.rs | 16 +++++++++++++++- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/ci/lint/06_script.sh b/ci/lint/06_script.sh index 2824356506b..7e27197024e 100755 --- a/ci/lint/06_script.sh +++ b/ci/lint/06_script.sh @@ -1,6 +1,6 @@ #!/usr/bin/env bash # -# Copyright (c) 2018-2022 The Bitcoin Core developers +# Copyright (c) 2018-present The Bitcoin Core developers # Distributed under the MIT software license, see the accompanying # file COPYING or http://www.opensource.org/licenses/mit-license.php. @@ -9,22 +9,13 @@ export LC_ALL=C set -ex if [ -n "$CIRRUS_PR" ]; then - COMMIT_RANGE="HEAD~..HEAD" + export COMMIT_RANGE="HEAD~..HEAD" if [ "$(git rev-list -1 HEAD)" != "$(git rev-list -1 --merges HEAD)" ]; then echo "Error: The top commit must be a merge commit, usually the remote 'pull/${PR_NUMBER}/merge' branch." false fi -else - # Otherwise, assume that a merge commit exists. This merge commit is assumed - # to be the base, after which linting will be done. If the merge commit is - # HEAD, the range will be empty. - COMMIT_RANGE="$( git rev-list --max-count=1 --merges HEAD )..HEAD" fi -export COMMIT_RANGE -echo -git log --no-merges --oneline "$COMMIT_RANGE" -echo RUST_BACKTRACE=1 "${LINT_RUNNER_PATH}/test_runner" if [ "$CIRRUS_REPO_FULL_NAME" = "bitcoin/bitcoin" ] && [ "$CIRRUS_PR" = "" ] ; then diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index 36f82f14b1a..dacdff8a828 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -180,7 +180,17 @@ fn get_git_root() -> PathBuf { /// Return the commit range, or panic fn commit_range() -> String { - env::var("COMMIT_RANGE").unwrap() + // Use the env var, if set. E.g. COMMIT_RANGE='HEAD~n..HEAD' for the last 'n' commits. + env::var("COMMIT_RANGE").unwrap_or_else(|_| { + // Otherwise, assume that a merge commit exists. This merge commit is assumed + // to be the base, after which linting will be done. If the merge commit is + // HEAD, the range will be empty. + format!( + "{}..HEAD", + check_output(git().args(["rev-list", "--max-count=1", "--merges", "HEAD"])) + .expect("check_output failed") + ) + }) } /// Return all subtree paths @@ -673,6 +683,10 @@ fn main() -> ExitCode { }; let git_root = get_git_root(); + let commit_range = commit_range(); + let commit_log = check_output(git().args(["log", "--no-merges", "--oneline", &commit_range])) + .expect("check_output failed"); + println!("Checking commit range ({commit_range}):\n{commit_log}\n"); let mut test_failed = false; for linter in linters_to_run { From faf8fc5487d409eeff7b7b260eabb6929a7b7a5f Mon Sep 17 00:00:00 2001 From: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz> Date: Tue, 14 Jan 2025 14:05:18 +0100 Subject: [PATCH 3/3] lint: Call lint_commit_msg from test_runner Allowing to call the check from the test_runner allows for consistent error messages. Also, manually setting the commit range is no longer needed. --- test/lint/lint-git-commit-check.py | 52 ------------------------------ test/lint/test_runner/src/main.rs | 41 +++++++++++++++++++++++ 2 files changed, 41 insertions(+), 52 deletions(-) delete mode 100755 test/lint/lint-git-commit-check.py diff --git a/test/lint/lint-git-commit-check.py b/test/lint/lint-git-commit-check.py deleted file mode 100755 index f5636f22ec8..00000000000 --- a/test/lint/lint-git-commit-check.py +++ /dev/null @@ -1,52 +0,0 @@ -#!/usr/bin/env python3 -# -# Copyright (c) 2020-2022 The Bitcoin Core developers -# Distributed under the MIT software license, see the accompanying -# file COPYING or http://www.opensource.org/licenses/mit-license.php. -# -# Linter to check that commit messages have a new line before the body -# or no body at all - -import argparse -import os -import sys - -from subprocess import check_output - - -def parse_args(): - """Parse command line arguments.""" - parser = argparse.ArgumentParser( - description=""" - Linter to check that commit messages have a new line before - the body or no body at all. - """, - epilog=f""" - You can manually set the commit-range with the COMMIT_RANGE - environment variable (e.g. "COMMIT_RANGE='HEAD~n..HEAD' - {sys.argv[0]}") for the last 'n' commits. - """) - return parser.parse_args() - - -def main(): - parse_args() - exit_code = 0 - - assert os.getenv("COMMIT_RANGE") # E.g. COMMIT_RANGE='HEAD~n..HEAD' - commit_range = os.getenv("COMMIT_RANGE") - - commit_hashes = check_output(["git", "-c", "log.showSignature=false", "log", commit_range, "--format=%H"], text=True, encoding="utf8").splitlines() - - for hash in commit_hashes: - commit_info = check_output(["git", "-c", "log.showSignature=false", "log", "--format=%B", "-n", "1", hash], text=True, encoding="utf8").splitlines() - if len(commit_info) >= 2: - if commit_info[1]: - print(f"The subject line of commit hash {hash} is followed by a non-empty line. Subject lines should always be followed by a blank line.") - exit_code = 1 - - sys.exit(exit_code) - - -if __name__ == "__main__": - main() diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs index dacdff8a828..0d785682bb2 100644 --- a/test/lint/test_runner/src/main.rs +++ b/test/lint/test_runner/src/main.rs @@ -73,6 +73,11 @@ fn get_linter_list() -> Vec<&'static Linter> { name: "scripted_diff", lint_fn: lint_scripted_diff }, + &Linter { + description: "Check that commit messages have a new line before the body or no body at all.", + name: "commit_msg", + lint_fn: lint_commit_msg + }, &Linter { description: "Check that tabs are not used as whitespace", name: "tabs_whitespace", @@ -243,6 +248,42 @@ fn lint_scripted_diff() -> LintResult { } } +fn lint_commit_msg() -> LintResult { + let mut good = true; + let commit_hashes = check_output(git().args(&[ + "-c", + "log.showSignature=false", + "log", + &commit_range(), + "--format=%H", + ]))?; + for hash in commit_hashes.lines() { + let commit_info = check_output(git().args([ + "-c", + "log.showSignature=false", + "log", + "--format=%B", + "-n", + "1", + hash, + ]))?; + if let Some(line) = commit_info.lines().nth(1) { + if !line.is_empty() { + println!( + "The subject line of commit hash {} is followed by a non-empty line. Subject lines should always be followed by a blank line.", + hash + ); + good = false; + } + } + } + if good { + Ok(()) + } else { + Err("".to_string()) + } +} + fn lint_py_lint() -> LintResult { let bin_name = "ruff"; let checks = format!(