Skip to content

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Dec 18, 2025

The single, large main.rs file is fine, but at some point it becomes harder to read.

So reduce the size by pulling functions out into modules.

This can be reviewed with the git option: --color-moved=dimmed-zebra

@DrahtBot DrahtBot added the Tests label Dec 18, 2025
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 18, 2025

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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/34098.

Reviews

See the guideline for information on the review process.

Type Reviewers
ACK l0rinc
Concept ACK rkrux

If your review is incorrectly listed, please copy-paste <!--meta-tag:bot-skip--> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

LLM Linter (✨ experimental)

Possible typos and grammar issues:

  • git-subtree-checksh -> git-subtree-check.sh [missing dot before "sh" in reference to the script name]

2026-01-05

@maflcko
Copy link
Member Author

maflcko commented Dec 22, 2025

IIRC @davidgumberg had a similar patch a year or two ago, but moved each function into a separate file. This patch keeps a grouping into topics. Maybe @davidgumberg can nack or ack this one?

@rkrux
Copy link
Contributor

rkrux commented Dec 23, 2025

Concept ACK fafdeb8 on an initial glance, will take a deeper look.

The context based code separation seems fine to me, also makes the Rust code in the linter slightly more inviting to me, who has limited experience with it.

@l0rinc
Copy link
Contributor

l0rinc commented Jan 5, 2026

Inlined the changes back to see the diff, besides some whitespace, it basically only contained exposing the functions as public and renaming lint_doc to lint_doc_args.

Details
diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs
index 171c98072a..acd36ac2dd 100644
--- a/test/lint/test_runner/src/main.rs
+++ b/test/lint/test_runner/src/main.rs
@@ -2,18 +2,27 @@
 // Distributed under the MIT software license, see the accompanying
 // file COPYING or https://opensource.org/license/mit/.
 
+mod lint_cpp;
+mod lint_docs;
+mod lint_py;
+mod lint_repo_hygiene;
+mod lint_text_format;
+mod util;
+
 use std::env;
-use std::fs::{self, File};
-use std::io::{ErrorKind, Read, Seek, SeekFrom};
-use std::path::PathBuf;
-use std::process::{Command, ExitCode, Stdio};
+use std::fs;
+use std::process::{Command, ExitCode};
 
-/// A possible error returned by any of the linters.
-///
-/// The error string should explain the failure type and list all violations.
-type LintError = String;
-type LintResult = Result<(), LintError>;
-type LintFn = fn() -> LintResult;
+use lint_cpp::{
+    lint_boost_assert, lint_includes_build_config, lint_rpc_assert, lint_std_filesystem,
+};
+use lint_docs::{lint_doc_args, lint_doc_release_note_snippets, lint_markdown};
+use lint_py::lint_py_lint;
+use lint_repo_hygiene::{lint_scripted_diff, lint_subtree};
+use lint_text_format::{
+    lint_commit_msg, lint_tabs_whitespace, lint_trailing_newline, lint_trailing_whitespace,
+};
+use util::{check_output, commit_range, get_git_root, git, LintFn, LintResult};
 
 struct Linter {
     pub description: &'static str,
@@ -26,7 +35,7 @@ fn get_linter_list() -> Vec<&'static Linter> {
         &Linter {
             description: "Check that all command line arguments are documented.",
             name: "doc",
-            lint_fn: lint_doc
+            lint_fn: lint_doc_args
         },
         &Linter {
             description: "Check that no symbol from bitcoin-build-config.h is used without the header being included",
@@ -162,7 +171,7 @@ fn parse_lint_args(args: &[String]) -> Vec<&'static Linter> {
 /// Lint functions should use this command, so that only files tracked by git are considered and
 /// temporary and untracked files are ignored. For example, instead of 'grep', 'git grep' should be
 /// used.
-fn git() -> Command {
+pub fn git() -> Command {
     let mut git = Command::new("git");
     git.arg("--no-pager");
     git
@@ -170,7 +179,7 @@ fn git() -> Command {
 
 /// Return stdout on success and a LintError on failure, when invalid UTF8 was detected or the
 /// command did not succeed.
-fn check_output(cmd: &mut std::process::Command) -> Result<String, LintError> {
+pub fn check_output(cmd: &mut Command) -> Result<String, LintError> {
     let out = cmd.output().expect("command error");
     if !out.status.success() {
         return Err(String::from_utf8_lossy(&out.stderr).to_string());
@@ -184,12 +193,12 @@ fn check_output(cmd: &mut std::process::Command) -> Result<String, LintError> {
 }
 
 /// Return the git root as utf8, or panic
-fn get_git_root() -> PathBuf {
+pub 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 {
+pub fn commit_range() -> String {
     // 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
@@ -204,7 +213,7 @@ fn commit_range() -> String {
 }
 
 /// Return all subtree paths
-fn get_subtrees() -> Vec<&'static str> {
+pub fn get_subtrees() -> Vec<&'static str> {
     // Keep in sync with [test/lint/README.md#git-subtree-checksh]
     vec![
         "src/crc32c",
@@ -217,7 +226,7 @@ fn get_subtrees() -> Vec<&'static str> {
 }
 
 /// Return the pathspecs to exclude by default
-fn get_pathspecs_default_excludes() -> Vec<String> {
+pub fn get_pathspecs_default_excludes() -> Vec<String> {
     get_subtrees()
         .iter()
         .chain(&[
@@ -227,7 +236,7 @@ fn get_pathspecs_default_excludes() -> Vec<String> {
         .collect()
 }
 
-fn lint_subtree() -> LintResult {
+pub fn lint_subtree() -> LintResult {
     // This only checks that the trees are pure subtrees, it is not doing a full
     // check with -r to not have to fetch all the remotes.
     let mut good = true;
@@ -245,7 +254,7 @@ fn lint_subtree() -> LintResult {
     }
 }
 
-fn lint_scripted_diff() -> LintResult {
+pub fn lint_scripted_diff() -> LintResult {
     if Command::new("test/lint/commit-script-check.sh")
         .arg(commit_range())
         .status()
@@ -258,7 +267,7 @@ fn lint_scripted_diff() -> LintResult {
     }
 }
 
-fn lint_commit_msg() -> LintResult {
+pub fn lint_commit_msg() -> LintResult {
     let mut good = true;
     let commit_hashes = check_output(git().args([
         "-c",
@@ -293,7 +302,7 @@ fn lint_commit_msg() -> LintResult {
     }
 }
 
-fn lint_py_lint() -> LintResult {
+pub fn lint_py_lint() -> LintResult {
     let bin_name = "ruff";
     let checks = format!(
         "--select={}",
@@ -353,16 +362,14 @@ fn lint_py_lint() -> LintResult {
         Ok(status) if status.success() => Ok(()),
         Ok(_) => Err(format!("`{bin_name}` found errors!")),
         Err(e) if e.kind() == ErrorKind::NotFound => {
-            println!(
-                "`{bin_name}` was not found in $PATH, skipping those checks."
-            );
+            println!("`{bin_name}` was not found in $PATH, skipping those checks.");
             Ok(())
         }
         Err(e) => Err(format!("Error running `{bin_name}`: {e}")),
     }
 }
 
-fn lint_std_filesystem() -> LintResult {
+pub fn lint_std_filesystem() -> LintResult {
     let found = git()
         .args([
             "grep",
@@ -390,7 +397,7 @@ fs:: namespace, which has unsafe filesystem functions marked as deleted.
     }
 }
 
-fn lint_rpc_assert() -> LintResult {
+pub fn lint_rpc_assert() -> LintResult {
     let found = git()
         .args([
             "grep",
@@ -420,7 +427,7 @@ checks should be used over assert. See: src/util/check.h
     }
 }
 
-fn lint_boost_assert() -> LintResult {
+pub fn lint_boost_assert() -> LintResult {
     let found = git()
         .args([
             "grep",
@@ -446,7 +453,7 @@ include of the boost/assert.hpp dependency.
     }
 }
 
-fn lint_doc_release_note_snippets() -> LintResult {
+pub fn lint_doc_release_note_snippets() -> LintResult {
     let non_release_notes = check_output(git().args([
         "ls-files",
         "--",
@@ -497,7 +504,7 @@ fn get_pathspecs_exclude_whitespace() -> Vec<String> {
     list
 }
 
-fn lint_trailing_whitespace() -> LintResult {
+pub fn lint_trailing_whitespace() -> LintResult {
     let trailing_space = git()
         .args(["grep", "-I", "--line-number", "\\s$", "--"])
         .args(get_pathspecs_exclude_whitespace())
@@ -522,7 +529,7 @@ sourced files to the exclude list.
     }
 }
 
-fn lint_trailing_newline() -> LintResult {
+pub fn lint_trailing_newline() -> LintResult {
     let files = check_output(
         git()
             .args([
@@ -560,7 +567,7 @@ Please add any false positives to the exclude list.
     }
 }
 
-fn lint_tabs_whitespace() -> LintResult {
+pub fn lint_tabs_whitespace() -> LintResult {
     let tabs = git()
         .args(["grep", "-I", "--line-number", "--perl-regexp", "^\\t", "--"])
         .args(["*.cpp", "*.h", "*.md", "*.py", "*.sh"])
@@ -584,7 +591,7 @@ Please add any false positives, such as subtrees, or externally sourced files to
     }
 }
 
-fn lint_includes_build_config() -> LintResult {
+pub fn lint_includes_build_config() -> LintResult {
     let config_path = "./cmake/bitcoin-build-config.h.in";
     let defines_regex = format!(
         r"^\s*(?!//).*({})",
@@ -673,7 +680,7 @@ the header. Consider removing the unused include.
     Ok(())
 }
 
-fn lint_doc() -> LintResult {
+pub fn lint_doc_args() -> LintResult {
     if Command::new("test/lint/check-doc.py")
         .status()
         .expect("command error")
@@ -685,7 +692,7 @@ fn lint_doc() -> LintResult {
     }
 }
 
-fn lint_markdown() -> LintResult {
+pub fn lint_markdown() -> LintResult {
     let bin_name = "mlc";
     let mut md_ignore_paths = get_subtrees();
     md_ignore_paths.push("./doc/README_doxygen.md");
@@ -728,6 +735,7 @@ Markdown link errors found:
     }
 }
 
+
 fn run_all_python_linters() -> LintResult {
     let mut good = true;
     let lint_dir = get_git_root().join("test/lint");

Lightly tested code review ACK fa578d9

@DrahtBot DrahtBot requested a review from rkrux January 5, 2026 17:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants