lint: Fix lint-whitespace issues #29487

pull maflcko wants to merge 2 commits into bitcoin:master from maflcko:2402-lint-fix-ws- changing 2 files +84 −137
  1. maflcko commented at 12:05 PM on February 27, 2024: member

    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 #29274 (review)
    • 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 ....

  2. DrahtBot commented at 12:05 PM on February 27, 2024: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK TheCharlatan
    Concept ACK Empact

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #29479 (test: Refactor subtree exclusion in lint tests by BrandonOdiwuor)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  3. DrahtBot added the label Tests on Feb 27, 2024
  4. Sjors commented at 12:15 PM on February 27, 2024: member

    Does this PR allow dropping COMMIT_RANGE from ci/lint/06_script.sh?

  5. maflcko commented at 12:45 PM on February 27, 2024: member

    I think it should be dropped, but that can be done in a follow-up, unrelated to the bugfixes here.

  6. Empact commented at 4:48 PM on February 27, 2024: contributor

    Concept ACK

  7. in test/lint/test_runner/src/main.rs:103 in fa68024bcb outdated
      94 | @@ -95,6 +95,85 @@ fs:: namespace, which has unsafe filesystem functions marked as deleted.
      95 |      }
      96 |  }
      97 |  
      98 | +/// Return the pathspecs for whitespace related excludes
      99 | +fn get_pathspecs_exclude_whitespace() -> Vec<String> {
     100 | +    let mut list = get_pathspecs_exclude_subtrees();
     101 | +    list.extend(
     102 | +        [
     103 | +            // Permanant excludes
    


    flack commented at 10:34 AM on March 3, 2024:

    Permanant => Permanent


    maflcko commented at 11:33 AM on March 11, 2024:

    Thanks, fixed typo.

  8. maflcko force-pushed on Mar 11, 2024
  9. lint: Fix lint-whitespace issues fa5729436c
  10. maflcko force-pushed on Mar 12, 2024
  11. in test/lint/test_runner/src/main.rs:136 in fa5729436c outdated
     131 | +        .args(get_pathspecs_exclude_whitespace())
     132 | +        .status()
     133 | +        .expect("command error")
     134 | +        .success();
     135 | +    if trailing_space {
     136 | +        Err(r#"
    


    TheCharlatan commented at 1:31 PM on March 13, 2024:

    How about displaying the offending lines with something like: EDIT: Diff was generated on a dirty commit by mistake.

    diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs
    index f9df576239..687d11c4e0 100644
    --- a/test/lint/test_runner/src/main.rs
    +++ b/test/lint/test_runner/src/main.rs
    @@ -129 +129 @@ fn lint_trailing_whitespace() -> LintResult {
    -    let trailing_space = git()
    +    let trailing_whitespace = check_output(git()
    @@ -131,6 +131,4 @@ fn lint_trailing_whitespace() -> LintResult {
    -        .args(get_pathspecs_exclude_whitespace())
    -        .status()
    -        .expect("command error")
    -        .success();
    -    if trailing_space {
    -        Err(r#"
    +        .args(get_pathspecs_exclude_whitespace()));
    +    if let Ok(message) = trailing_whitespace {
    +        Err(format!(r#"
    +{}
    @@ -146,2 +144,2 @@ sourced files to the exclude list.
    -            "#
    -        .to_string())
    +            "#,
    +        message))
    @@ -154 +152 @@ fn lint_tabs_whitespace() -> LintResult {
    -    let tabs = git()
    +    let tabs = check_output(git()
    @@ -157,6 +155,4 @@ fn lint_tabs_whitespace() -> LintResult {
    -        .args(get_pathspecs_exclude_whitespace())
    -        .status()
    -        .expect("command error")
    -        .success();
    -    if tabs {
    -        Err(r#"
    +        .args(get_pathspecs_exclude_whitespace()));
    +    if let Ok(message) = tabs {
    +        Err(format!(r#"
    +{}
    @@ -170,2 +166,2 @@ Please add any false positives, such as subtrees, or externally sourced files to
    -            "#
    -        .to_string())
    +            "#,
    +        message))
    

    maflcko commented at 1:39 PM on March 13, 2024:

    They are already printed, no? If not, it would be good if you shared the output of my pull vs your suggestion, so that the difference in output can be observed.


    maflcko commented at 1:47 PM on March 13, 2024:

    For reference, the output on my system, with trailing whitespace added is:

    test/functional/wallet_disable.py:23:         import time;self.nodes[1].setmocktime(int(time.time()-10*60)) 
    
    ^^^
    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.
                
    ^---- ⚠️ Failure generated from trailing whitespace check!
    

    TheCharlatan commented at 2:03 PM on March 13, 2024:

    Ah, this is git paging the output on my system if the output is not piped to somewhere else. I'll just switch the paging off for git grep then, it's annoying anyway.


    maflcko commented at 2:37 PM on March 13, 2024:

    Ah, interesting. One could consider to disable the git pager for the lint tests with the -c option. Not sure what to pass to it, and I haven't tested it, but the diff would look a bit like:

    diff --git a/test/lint/test_runner/src/main.rs b/test/lint/test_runner/src/main.rs
    index b97e822484..d6c1d39155 100644
    --- a/test/lint/test_runner/src/main.rs
    +++ b/test/lint/test_runner/src/main.rs
    @@ -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.args(["-c", "..."]);
    +    git
     }
     
     /// Return stdout
    

    TheCharlatan commented at 2:39 PM on March 13, 2024:

    Or just git --no-pager?


    maflcko commented at 4:07 PM on March 13, 2024:

    Thanks, added unrelated commit to fix this as well for all lint checks.

  12. in test/lint/test_runner/src/main.rs:156 in fa5729436c outdated
     151 | +}
     152 | +
     153 | +fn lint_tabs_whitespace() -> LintResult {
     154 | +    let tabs = git()
     155 | +        .args(["grep", "-I", "--line-number", "--perl-regexp", "^\\t", "--"])
     156 | +        .args(["*.cpp", "*.h", "*.md", "*.py", "*.sh"])
    


    TheCharlatan commented at 1:32 PM on March 13, 2024:

    This list is pretty limited. Would adding .rs to it make sense?


    maflcko commented at 1:41 PM on March 13, 2024:

    Sure, it can be added. I tried to keep everything as-is, except for the bugfixes. I may add it, if I have to re-touch.

  13. TheCharlatan approved
  14. TheCharlatan commented at 2:25 PM on March 13, 2024: contributor

    Nice, ACK fa5729436ca12b20cfa2cd1f0c6f54af7192f0a6

  15. DrahtBot requested review from Empact on Mar 13, 2024
  16. lint: Use git --no-pager to print any output in one go 5555395c15
  17. TheCharlatan approved
  18. TheCharlatan commented at 4:20 PM on March 13, 2024: contributor

    Re-ACK 5555395c15e896230a55c131fc3cbfd9d116adf8

  19. fanquake merged this on Mar 15, 2024
  20. fanquake closed this on Mar 15, 2024

  21. maflcko deleted the branch on Mar 15, 2024
  22. bitcoin locked this on Mar 15, 2025

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-24 09:14 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me