foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-08 10:31:50 -05:00
Code Issues Activity

Merge bitcoin/bitcoin#29487: lint: Fix lint-whitespace issues

Browse source 
5555395c15 lint: Use git --no-pager to print any output in one go (MarcoFalke)
fa5729436c lint: Fix lint-whitespace issues (MarcoFalke)

Pull request description:

  The lint check has many issues:

  * It uses `COMMIT_RANGE`, which is brittle code, apparently making it harder to run the CI locally, or self-hosted. See https://github.com/bitcoin/bitcoin/pull/29274#discussion_r1457739319
  * The result depends on `COMMIT_RANGE`, or the number of commits passed to the script, which can cause false negatives or false positives.
  * It is based on the diff output, parsing it, and printing it again, which is brittle as well.
  * The output does not include line number, making it harder to act on a lint error.

  Fix all issues by removing the script and replacing it with a simple call to `git grep -I --line-number ...`.

ACKs for top commit:
  TheCharlatan:
    Re-ACK 5555395c15

Tree-SHA512: 71ea8b6382af064beb72fb17f21a0ae9e9238c97e7fa43c2ec353fd1dd73a7bbd696ba0f0a9f65d1eff7c86cbf6cc104a992cb5450a3d50f122955e835270065
This commit is contained in:
fanquake 2024-03-15 12:03:42 +00:00
commit 015ac13dcc
No known key found for this signature in database
GPG key ID: 2EEB9F5CC09526C1
2 changed files with 84 additions and 137 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

136
test/lint/lint-whitespace.py
View file

@ -1,136 +0,0 @@
#!/usr/bin/env python3
#
# Copyright (c) 2017-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.
#
# Check for new lines in diff that introduce trailing whitespace or
# tab characters instead of spaces.
# We can't run this check unless we know the commit range for the PR.
import argparse
import os
import re
import sys
from subprocess import check_output
EXCLUDED_DIRS = ["depends/patches/",
"contrib/guix/patches/",
"src/leveldb/",
"src/crc32c/",
"src/secp256k1/",
"src/minisketch/",
"doc/release-notes/",
"src/qt/locale"]
def parse_args():
"""Parse command line arguments."""
parser = argparse.ArgumentParser(
description="""
Check for new lines in diff that introduce trailing whitespace
or tab characters instead of spaces in unstaged changes, the
previous n commits, or a commit-range.
""",
epilog=f"""
You can manually set the commit-range with the COMMIT_RANGE
environment variable (e.g. "COMMIT_RANGE='47ba2c3...ee50c9e'
{sys.argv[0]}"). Defaults to current merge base when neither
prev-commits nor the environment variable is set.
""")
parser.add_argument("--prev-commits", "-p", required=False, help="The previous n commits to check")
return parser.parse_args()
def report_diff(selection):
filename = ""
seen = False
seenln = False
print("The following changes were suspected:")
for line in selection:
if re.match(r"^diff", line):
filename = line
seen = False
elif re.match(r"^@@", line):
linenumber = line
seenln = False
else:
if not seen:
# The first time a file is seen with trailing whitespace or a tab character, we print the
# filename (preceded by a newline).
print("")
print(filename)
seen = True
if not seenln:
print(linenumber)
seenln = True
print(line)
def get_diff(commit_range, check_only_code):
exclude_args = [":(exclude)" + dir for dir in EXCLUDED_DIRS]
if check_only_code:
what_files = ["*.cpp", "*.h", "*.md", "*.py", "*.sh"]
else:
what_files = ["."]
diff = check_output(["git", "diff", "-U0", commit_range, "--"] + what_files + exclude_args, text=True, encoding="utf8")
return diff
def main():
args = parse_args()
if not os.getenv("COMMIT_RANGE"):
if args.prev_commits:
commit_range = "HEAD~" + args.prev_commits + "...HEAD"
else:
# This assumes that the target branch of the pull request will be master.
merge_base = check_output(["git", "merge-base", "HEAD", "master"], text=True, encoding="utf8").rstrip("\n")
commit_range = merge_base + "..HEAD"
else:
commit_range = os.getenv("COMMIT_RANGE")
if commit_range == "SKIP_EMPTY_NOT_A_PR":
sys.exit(0)
whitespace_selection = []
tab_selection = []
# Check if trailing whitespace was found in the diff.
for line in get_diff(commit_range, check_only_code=False).splitlines():
if re.match(r"^(diff --git|\@@|^\+.*\s+$)", line):
whitespace_selection.append(line)
whitespace_additions = [i for i in whitespace_selection if i.startswith("+")]
# Check if tab characters were found in the diff.
for line in get_diff(commit_range, check_only_code=True).splitlines():
if re.match(r"^(diff --git|\@@|^\+.*\t)", line):
tab_selection.append(line)
tab_additions = [i for i in tab_selection if i.startswith("+")]
ret = 0
if len(whitespace_additions) > 0:
print("This diff appears to have added new lines with trailing whitespace.")
report_diff(whitespace_selection)
ret = 1
if len(tab_additions) > 0:
print("This diff appears to have added new lines with tab characters instead of spaces.")
report_diff(tab_selection)
ret = 1
sys.exit(ret)
if __name__ == "__main__":
main()

85
test/lint/test_runner/src/main.rs
View file

@ -14,7 +14,9 @@ type LintFn = fn() -> LintResult;
/// Return the git command
fn git() -> Command {
Command::new("git")
let mut git = Command::new("git");
git.arg("--no-pager");
git
}
/// Return stdout
@ -95,6 +97,85 @@ fs:: namespace, which has unsafe filesystem functions marked as deleted.
}
}
/// Return the pathspecs for whitespace related excludes
fn get_pathspecs_exclude_whitespace() -> Vec<String> {
let mut list = get_pathspecs_exclude_subtrees();
list.extend(
[
// Permanent excludes
"*.patch",
"src/qt/locale",
"contrib/windeploy/win-codesign.cert",
"doc/README_windows.txt",
// Temporary excludes, or existing violations
"doc/release-notes/release-notes-0.*",
"contrib/init/bitcoind.openrc",
"contrib/macdeploy/macdeployqtplus",
"src/crypto/sha256_sse4.cpp",
"src/qt/res/src/*.svg",
"test/functional/test_framework/crypto/ellswift_decode_test_vectors.csv",
"test/functional/test_framework/crypto/xswiftec_inv_test_vectors.csv",
"contrib/qos/tc.sh",
"contrib/verify-commits/gpg.sh",
"src/univalue/include/univalue_escapes.h",
"src/univalue/test/object.cpp",
"test/lint/git-subtree-check.sh",
]
.iter()
.map(|s| format!(":(exclude){}", s)),
);
list
}
fn lint_trailing_whitespace() -> LintResult {
let trailing_space = git()
.args(["grep", "-I", "--line-number", "\\s$", "--"])
.args(get_pathspecs_exclude_whitespace())
.status()
.expect("command error")
.success();
if trailing_space {
Err(r#"
^^^
Trailing whitespace is problematic, because git may warn about it, or editors may remove it by
default, forcing developers in the future to either undo the changes manually or spend time on
review.
Thus, it is best to remove the trailing space now.
Please add any false positives, such as subtrees, Windows-related files, patch files, or externally
sourced files to the exclude list.
"#
.to_string())
} else {
Ok(())
}
}
fn lint_tabs_whitespace() -> LintResult {
let tabs = git()
.args(["grep", "-I", "--line-number", "--perl-regexp", "^\\t", "--"])
.args(["*.cpp", "*.h", "*.md", "*.py", "*.sh"])
.args(get_pathspecs_exclude_whitespace())
.status()
.expect("command error")
.success();
if tabs {
Err(r#"
^^^
Use of tabs in this codebase is problematic, because existing code uses spaces and tabs will cause
display issues and conflict with editor settings.
Please remove the tabs.
Please add any false positives, such as subtrees, or externally sourced files to the exclude list.
"#
.to_string())
} else {
Ok(())
}
}
fn lint_includes_build_config() -> LintResult {
let config_path = "./src/config/bitcoin-config.h.in";
let include_directive = "#include <config/bitcoin-config.h>";
@ -232,6 +313,8 @@ fn main() -> ExitCode {
let test_list: Vec<(&str, LintFn)> = vec![
("subtree check", lint_subtree),
("std::filesystem check", lint_std_filesystem),
("trailing whitespace check", lint_trailing_whitespace),
("no-tabs check", lint_tabs_whitespace),
("build config includes check", lint_includes_build_config),
("-help=1 documentation check", lint_doc),
("lint-*.py scripts", lint_all),