Tests: Add a lint check for trailing whitespace #11300

meshcollider · 2017-09-11T10:21:47Z

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

practicalswift · 2017-09-11T11:06:00Z

utACK 258094ae4bdb17a1f2063b9f85955f41945e4611

meshcollider · 2017-09-12T00:46:18Z

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

sipa · 2017-09-12T02:25:19Z

contrib/devtools/lint-whitespace.sh

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

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

maflcko · 2017-09-12T02:43:40Z

.travis.yml

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

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

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 · 2017-09-12T23:17:14Z

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 · 2017-09-13T00:38:36Z

@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 · 2017-09-13T08:08:16Z

Thanks, that was it indeed.

ACK b8c0cfa

practicalswift · 2017-09-13T08:16:33Z

utACK b8c0cfae6d6f39fe38dcfec229f3824a5af44b29

maflcko · 2017-09-13T13:04:34Z

utACK b8c0cfae6d6f39fe38dcfec229f3824a5af44b29

jnewbery

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.

jnewbery · 2017-09-13T21:01:17Z

contrib/devtools/lint-whitespace.sh

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"

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.

meshcollider · 2017-09-13T23:40:02Z

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

maflcko · 2017-09-14T09:38:28Z

re-utACK 1f379b1

@practicalswift

1f379b1 Add tab char lint check and exclude imported dependencies (MeshCollider) dd36561 Add a lint check for trailing whitespace. (Evan Klitzke) Pull request description: 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 Tree-SHA512: d2dfbedc8469026f39b0c63d9a71d8b8e2ed3815d69fecaabad10304d977d6345728c4c865ec7600ed539b1f7cabaa826b50312f4d2eef0a1583d4ff9024c36d

@practicalswift

…*dash* no travis *DASH* doesn't implement this check into travis 1f379b1 Add tab char lint check and exclude imported dependencies (MeshCollider) dd36561 Add a lint check for trailing whitespace. (Evan Klitzke) Pull request description: This is a new attempt at bitcoin#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 Tree-SHA512: d2dfbedc8469026f39b0c63d9a71d8b8e2ed3815d69fecaabad10304d977d6345728c4c865ec7600ed539b1f7cabaa826b50312f4d2eef0a1583d4ff9024c36d

fanquake added the Tests label Sep 11, 2017

sipa reviewed Sep 12, 2017

View reviewed changes

maflcko reviewed Sep 12, 2017

View reviewed changes

jnewbery reviewed Sep 13, 2017

View reviewed changes

eklitzke and others added 2 commits September 14, 2017 10:49

Add tab char lint check and exclude imported dependencies

1f379b1

maflcko merged commit 1f379b1 into bitcoin:master Sep 14, 2017

meshcollider deleted the 201709_whitespace_lint branch September 14, 2017 09:43

TheBlueMatt mentioned this pull request Sep 19, 2017

Deduplicate CMerkleBlock construction code, add test coverage #11293

Merged

str4d mentioned this pull request Oct 27, 2020

Backport useful lints from upstream zcash/zcash#4829

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Tests: Add a lint check for trailing whitespace #11300

Tests: Add a lint check for trailing whitespace #11300

Uh oh!

Conversation

meshcollider commented Sep 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Sep 11, 2017

Uh oh!

meshcollider commented Sep 12, 2017

Uh oh!

sipa Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

meshcollider Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

meshcollider Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

maflcko Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

mess110 commented Sep 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mess110 commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

practicalswift commented Sep 13, 2017

Uh oh!

maflcko commented Sep 13, 2017

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

jnewbery Sep 13, 2017

Choose a reason for hiding this comment

Uh oh!

meshcollider commented Sep 13, 2017

Uh oh!

maflcko commented Sep 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

meshcollider commented Sep 11, 2017 •

edited

Loading

sipa Sep 12, 2017 •

edited

Loading

meshcollider Sep 12, 2017 •

edited

Loading

mess110 commented Sep 12, 2017 •

edited

Loading

meshcollider commented Sep 13, 2017 •

edited

Loading

mess110 commented Sep 13, 2017 •

edited

Loading