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.
Example of what a lint failure looks like in Travis: https://travis-ci.org/eklitzke/bitcoin/jobs/262052953
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.
10 | +if [ -z "${TRAVIS_COMMIT_RANGE}" ]; then 11 | + exit 0 12 | +fi 13 | + 14 | +showdiff() { 15 | + if ! git diff "${TRAVIS_COMMIT_RANGE}" --; then
I assume you need to set the context lines to 0 to prevent false positives?
-U0
That doesn't hurt, although I think it's correct as is. Context lines are shown with a leading space when they have no changes, so a context line that starts with + will actually have a leading space. I'll make this change anyway for safety/clarity.
Concept ACK, as trailing white space causes issues with some editors that remove it automatically on save from the whole file, even from untouched lines.
17 | + exit 1 18 | + fi 19 | +} 20 | + 21 | +# Do a first pass, and if no trailing whitespace was found then exit early. 22 | +if ! showdiff | egrep -q '^\+.*\s+$'; then
Nit: Use grep -E instead of egrep which is deprecated :-)
Serious nit, haha.
41 | + echo "$FILENAME" 42 | + SEEN=1 43 | + fi 44 | + echo "$line" 45 | + fi 46 | +done < <(showdiff | egrep '^(diff --git |\+.*\s+$)')
Nit: Use grep -E instead of egrep which is deprecated :-)
Concept ACK
@MarcoFalke @practicalswift Thanks for the suggestions, I've made these changes.
If this is merged, I'll submit some whitespace-only PRs to clean up various files in the project (probably starting with markdown files, and then moving onto code if that goes well). I feel like it's not worth sending out any whitespace PRs though until this has landed, since otherwise there could be new regressions.
@eklitzke Whitespace only PRs aren't likely to be accepted. Basically they disrupt other work and patches, and create rebasing issues, without providing much value. See the first point in the dev notes. "Do not submit patches solely to modify the style of existing code."
We also have clang-format rules that can be used against new/modified code, which should handle formatting.
@fanquake Noted. Although I did have problems with clang-format, since many of the existing files are not formatted properly, so running those files through clang-format caused needless code churn. In any event, this check will prevent the introduction of new trailing whitespace.
Please squash your commits according to https://github.com/bitcoin/bitcoin/blob/master/CONTRIBUTING.md#squashing-commits
Can we ignore violations introduced in the directories of our imported dependencies?
I'm thinking about:
src/leveldb/src/secp256k1/src/univalue/Also, what about adding a similar check for introductions of new \t:s in files with suffixes *.cpp, *.h, *.md, *.py and *.sh? Again excluding the imported dependencies.
@eklitzke Are you still working on this?
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 | + exit 0
Why exit as if the check was successful instead of showing an error?
utACK d8a7add0a1f31388eb1235ae11315f00dcf43be3 if the the right files / directories are excluded. It looks like git has a pathspec feature that let you easily exclude paths with :! syntax.
Other things that could be done in future prs:
contrib/devtools/commit-script-check.sh into this linting framework instead of running it separately.
Closing due to inactivity. Please ping me to get this reopened.
This is up for grab by other contributors.
