Tests: Add a lint check for trailing whitespace #11300

meshcollider commented at 10:21 AM on September 11, 2017: contributor

This is a new attempt at #11005

Addressed nits, excluded imported dependencies, squashed the original commits, and added a test for tab characters in the *.cpp *.h *.md *.py *.sh files too as per @practicalswift suggestion

fanquake added the label Tests on Sep 11, 2017

practicalswift commented at 11:06 AM on September 11, 2017: contributor

utACK 258094ae4bdb17a1f2063b9f85955f41945e4611

meshcollider commented at 12:46 AM on September 12, 2017: contributor

Fixed a mis-copied comment in the tab character check which was referring to whitespace again

in contrib/devtools/lint-whitespace.sh:57 in 98ff6e4d4d outdated

  52 | +  RET=1
  53 | +fi
  54 | +
  55 | +# Check if tab characters were found in the diff.
  56 | +if showcodediff | grep -P -q '^\+.*\t'; then
  57 | +  echo "This diff appears to have added new lines with tab characters not spaces."

sipa commented at 2:25 AM on September 12, 2017:

This doesn't sound like correct English to me (but I'm not a native speaker myself). Use "instead of spaces" perhaps?

meshcollider commented at 2:32 AM on September 12, 2017:

Yeah, I don't think it's technically wrong but it doesn't sound quite right, yours sounds better. Fixed.

in .travis.yml:52 in b98279d992 outdated

  45 | @@ -46,6 +46,7 @@ install:
  46 |  before_script:
  47 |      - if [ "$CHECK_DOC" = 1 -a "$TRAVIS_EVENT_TYPE" = "pull_request" ]; then contrib/devtools/commit-script-check.sh $TRAVIS_COMMIT_RANGE; fi
  48 |      - if [ "$CHECK_DOC" = 1 ]; then contrib/devtools/check-doc.py; fi
  49 | +    - if [ "$CHECK_DOC" = 1 ]; then contrib/devtools/lint-all.sh; fi

MarcoFalke commented at 2:43 AM on September 12, 2017:

This needs to be guarded by TRAVIS_COMMIT_RANGE if you fail on -z $TRAVIS_COMMIT_RANGE, no?

meshcollider commented at 3:16 AM on September 12, 2017:

I'm not even sure if I should have changed it to fail, should it just exit quietly if there's no commit range?

MarcoFalke commented at 4:03 AM on September 12, 2017:

Actually this should be fine, as TRAVIS_COMMIT_RANGE should never be empty according to the travis doc. (It is only empty for branches with a single commit, which we don't have)

mess110 commented at 11:17 PM on September 12, 2017: contributor

I can't manage to run the script. Am I calling it wrong?

TRAVIS_COMMIT_RANGE=50fae68d416b4b8ec4ca192923dfd5ae9ea42773...b3d6fc654770e3b4d2f82e8d77e531df9e522982 ./contrib/devtools/lint-all.sh
fatal: There is nothing to exclude from by :(exclude) patterns.
Perhaps you forgot to add either ':/' or '.' ?

I got the commit range from https://travis-ci.org/bitcoin/bitcoin/jobs/272143045

I get the same if I run

TRAVIS_COMMIT_RANGE=50fae68d416b4b8ec4ca192923dfd5ae9ea42773...b3d6fc654770e3b4d2f82e8d77e531df9e522982 git diff -U0 "${TRAVIS_COMMIT_RANGE}" -- ":(exclude)src/leveldb/" ":(exclude)src/secp256k1/" ":(exclude)src/univalue/"

This works:

TRAVIS_COMMIT_RANGE=50fae68d416b4b8ec4ca192923dfd5ae9ea42773...b3d6fc654770e3b4d2f82e8d77e531df9e522982 git diff -U0 "${TRAVIS_COMMIT_RANGE}"

meshcollider commented at 12:38 AM on September 13, 2017: contributor

@mess110 my guess is that you have a different version of git which doesn't assume all files to be included if only excludes are given, but I've updated the commit to (hopefully) fix it for you

mess110 commented at 8:08 AM on September 13, 2017: contributor

Thanks, that was it indeed.

ACK https://github.com/bitcoin/bitcoin/pull/11300/commits/b8c0cfae6d6f39fe38dcfec229f3824a5af44b29

practicalswift commented at 8:16 AM on September 13, 2017: contributor

utACK b8c0cfae6d6f39fe38dcfec229f3824a5af44b29

MarcoFalke commented at 1:04 PM on September 13, 2017: member

utACK b8c0cfae6d6f39fe38dcfec229f3824a5af44b29

in contrib/devtools/lint-whitespace.sh:11 in b8c0cfae6d outdated

   6 | +#
   7 | +# Check for new lines in diff that introduce trailing whitespace.
   8 | +
   9 | +# We can't run this check unless we know the commit range for the PR.
  10 | +if [ -z "${TRAVIS_COMMIT_RANGE}" ]; then
  11 | +  echo "Cannot run lint-whitespace.sh without commit range"

jnewbery commented at 9:01 PM on September 13, 2017:

Perhaps add help text here explaining how to run locally:

  echo "To run locally, use:"
  echo "TRAVIS_COMMIT_RANGE='<commit range>' .lint-whitespace.sh"
  echo "eg:"
  echo "TRAVIS_COMMIT_RANGE='47ba2c3...ee50c9e' .lint-whitespace.sh"

jnewbery commented at 9:06 PM on September 13, 2017: member

Tested ACK. I've tested locally that both checks work, and that the src/leveldb/ exclusion also works.

It'd be helpful to also print out the @@ line showing the line number where the whitespace was introduced. Something like:

diff --git a/contrib/devtools/lint-whitespace.sh b/contrib/devtools/lint-whitespace.sh
index 5bea86a..8a065fe 100755
--- a/contrib/devtools/lint-whitespace.sh
+++ b/contrib/devtools/lint-whitespace.sh
@@ -40,0 +41,2 @@ if showdiff | grep -E -q '^\+.*\s+$'; then
+    elif [[ "$line" =~ ^@@ ]]; then
+      LINENUMBER="$line"
@@ -46,0 +49 @@ if showdiff | grep -E -q '^\+.*\s+$'; then
+        echo "$LINENUMBER"
@@ -51 +54 @@ if showdiff | grep -E -q '^\+.*\s+$'; then
-  done < <(showdiff | grep -E '^(diff --git |\+.*\s+$)')
+  done < <(showdiff | grep -E '^(diff --git |@@|\+.*\s+$)')
@@ -64,0 +68,2 @@ if showcodediff | grep -P -q '^\+.*\t'; then
+    elif [[ "$line" =~ ^@@ ]]; then
+      LINENUMBER="$line"
@@ -70,0 +76 @@ if showcodediff | grep -P -q '^\+.*\t'; then
+        echo "$LINENUMBER"
@@ -75 +81 @@ if showcodediff | grep -P -q '^\+.*\t'; then
-  done < <(showcodediff | grep -P '^(diff --git |\+.*\t)')
+  done < <(showcodediff | grep -P '^(diff --git |@@|\+.*\t)')

should do it. This only prints out the line number for the first match in each file, but I think that's ok.

Add a lint check for trailing whitespace.

This adds a new CHECK_DOC check that looks for newly introduced trailing
whitespace. Existing trailing whitespace (of which there is plenty!)
will not trigger an error.

This is written in a generic way so that new lint-*.sh scripts can be
added to contrib/devtools/, as I'd like to contribute additional lint
checks in the future.

dd365612fd

Add tab char lint check and exclude imported dependencies 1f379b1f06

meshcollider commented at 11:40 PM on September 13, 2017: contributor

Rebased on master to fix merge conflict and addressed @jnewbery's comments, thanks!

MarcoFalke commented at 9:38 AM on September 14, 2017: member

re-utACK 1f379b1

MarcoFalke merged this on Sep 14, 2017

MarcoFalke closed this on Sep 14, 2017

MarcoFalke referenced this in commit f4ed44ab4a on Sep 14, 2017

meshcollider deleted the branch on Sep 14, 2017

PastaPastaPasta referenced this in commit ef10797760 on Sep 24, 2019

PastaPastaPasta referenced this in commit d57d950bc7 on Dec 21, 2019

PastaPastaPasta referenced this in commit cb5408a218 on Jan 2, 2020

PastaPastaPasta referenced this in commit cba81aa8bd on Jan 4, 2020

PastaPastaPasta referenced this in commit a01d3fc7b4 on Jan 4, 2020

PastaPastaPasta referenced this in commit d71fd97b51 on Jan 10, 2020

PastaPastaPasta referenced this in commit d7b26c696a on Jan 10, 2020

PastaPastaPasta referenced this in commit 80f4de55fd on Jan 10, 2020

PastaPastaPasta referenced this in commit d94853e4d1 on Jan 12, 2020

zkbot referenced this in commit 43ac2062f9 on Oct 28, 2020

zkbot referenced this in commit 84a5830aaa on Nov 9, 2020

ckti referenced this in commit 83ec5a464f on Mar 28, 2021

DrahtBot locked this on Sep 8, 2021