script: Linter to check commit message formatting by am1r021 · Pull Request #19439 · bitcoin/bitcoin

am1r021 · 2020-07-03T21:20:57Z

Write linter to check that commit messages have a new line before the body or no body at all. fixes issue #19091.

adamjonas

Warm welcome @ghorbanian and excited to see more contributions for you in the future!

adamjonas · 2020-07-06T23:45:28Z

test/lint/lint-git-commit-check.sh

This line is failing the linter for having a trailing whitespace.

adamjonas · 2020-07-06T23:50:06Z

test/lint/lint-git-commit-check.sh

I think you can just put 2020 here.

adamjonas · 2020-07-07T00:25:40Z

test/lint/lint-git-commit-check.sh

$ git log origin..HEAD --format=%s%b+ isn't working for me while testing locally. I get:

fatal: ambiguous argument 'origin..HEAD': unknown revision or path not in the working tree. Use '--' to separate paths from revisions, like this: 'git <command> [<revision>...] -- [<file>...]'

I changed it to git log origin/master..HEAD. My intent is to get the last N commits that have not yet been pushed in a portable way. I did some research and I believe the change that I have made achieves this, but please let me know if not.

adamjonas · 2020-07-07T00:25:44Z

test/lint/lint-git-commit-check.sh

If successfully passed, you'd want this to exit silently. Following the format of the other linters, I'd suggest replacing this line:

Suggested change

echo "No commit message formatting errors detected."

exit ${EXIT_CODE}

troygiorshev

Hi @ghorbanian thanks for picking this up!

When manually testing this script, I don't think it actually works :/

I've suggested below why I think this may be happening.

troygiorshev · 2020-07-07T16:51:19Z

test/lint/lint-git-commit-check.sh

This "+" idea is clever! However, I don't think it's one we can live with. The "+" character is actually used surprisingly often in commit messages.

git log --grep='+' --oneline | wc -l
> 706

Two suggested solutions, if you're looking for somewhere to start

Try and find a list of valid characters in commit messages, and see if you can use one that isn't in the list. Maybe "#" would work? I know that emojis work in commit messages, so maybe this isn't a viable option, but check it out :)

Try iterate over each commit, as opposed to getting them all at once.

After looking at the script some more, I decided to iterate over each commit by hash value in order to circumvent this delimiter problem. Thank you for the suggestion.

troygiorshev · 2020-07-07T16:53:19Z

test/lint/lint-git-commit-check.sh

Check out what this outputs, I don't think it's what you want.
Possibly you're looking for --format=%B+, and then see if you can think of a way to deal with the "+" issue.

troygiorshev · 2020-07-07T16:55:08Z

test/lint/lint-git-commit-check.sh

This message isn't quite right. Maybe something like one of these is better.

Suggested change

echo "Your subject line follows a non-empty line. Subject lines should always be followed by a blank line."

echo "Your body follows a non-empty line. Subject lines should always be followed by a blank line."

Suggested change

echo "Your subject line follows a non-empty line. Subject lines should always be followed by a blank line."

echo "Your subject line is followed by a non-empty line. Subject lines should always be followed by a blank line."

troygiorshev · 2020-07-07T16:55:39Z

test/lint/lint-git-commit-check.sh

Thanks for including this reference! Let's keep it out of the code though, can you move it to the body of your commit message?

am1r021

@troygiorshev Thank you for the code review!

maflcko · 2020-07-08T15:26:37Z

test/lint/lint-git-commit-check.sh

Could make sense to default this to TRAVIS_COMMIT_RANGE if available?

troygiorshev

Below \/

troygiorshev

Reviewed, manually tested. Works great!

Just need the message changed.

test/lint/lint-git-commit-check.sh

Write linter to check that commit messages have a new line before the body or no body at all. reference: gist.github.com/agnivade/67b42d664ece2d4210c7 Fixes issue bitcoin#19091.

troygiorshev · 2020-07-16T13:53:29Z

ACK 284a969 Reviewed, manually tested. Works great!

fjahr · 2020-07-17T12:46:39Z

tested ACK 284a969

Test

$ touch foo
$ git add foo
$ git commit -am "Foo
→ Bar"
$ test/lint/lint-git-commit-check.sh
The subject line of commit hash 1b6e02448bcced186160815a986d65ca38144940 is followed by a non-empty line. Subject lines should always be followed by a blank line.
$ test/lint/lint-git-commit-check.sh 0
$ test/lint/lint-git-commit-check.sh 1
The subject line of commit hash 1b6e02448bcced186160815a986d65ca38144940 is followed by a non-empty line. Subject lines should always be followed by a blank line.

adamjonas · 2020-07-23T14:43:31Z

utACK 284a969

284a969 Linter to check commit message formatting (Amir Ghorbanian) Pull request description: Write linter to check that commit messages have a new line before the body or no body at all. fixes issue bitcoin#19091. ACKs for top commit: troygiorshev: ACK 284a969 Reviewed, manually tested. Works great! fjahr: tested ACK 284a969 adamjonas: utACK 284a969 Tree-SHA512: fa278f090780b54e4fa6e2967a62b4c1a4da55d112ec1ad6dd7e1181ac490c5c1af0165524b5781b463fdd6d0f79fd3d95b5160184e6eca432ccff1189f77390

fjahr · 2020-08-02T22:25:40Z

The linter seems to fail in the CI for commit messages without a body, at least it did for me here https://travis-ci.org/github/bitcoin/bitcoin/builds/714296380 and I can not reproduce it locally. Can somebody else confirm this or is able to reproduce this locally?

fjahr · 2020-08-04T08:57:15Z

The linter seems to fail in the CI for commit messages without a body, at least it did for me here https://travis-ci.org/github/bitcoin/bitcoin/builds/714296380 and I can not reproduce it locally. Can somebody else confirm this or is able to reproduce this locally?

My initial analysis of what caused the failure was wrong so ignore that part. But I think I have found what the issue was and proposed a fix here: #19654

…ssage linter 7235178 lint: Remove travis env var from commit linter (Fabian Jahr) Pull request description: bitcoin#19439 was recently merged and seemed to work fine but I now noticed strange behavior when it was running in Travis, which I could not reproduce locally. It turns out `TRAVIS_COMMIT_RANGE` which is used in Travis to get the commits for the linter, uses all the commits that were in a push, which includes all rebase commits for example. This means that the linter can fail on a commit that the developer has never even seen before, which can be very confusing. See an example here which caused me to look into this: https://travis-ci.org/github/bitcoin/bitcoin/jobs/714296381 The commit that is reported as failing in my PR is not part of my PR. I think we rather want to use something like `git merge-base` to get the commit range by default and in Travis. I am leaving the env variable functionality in place with a different name but this is not a variable that can be expected to be present in the CI environments so the `merge-base` range should be used there by default. ACKs for top commit: hebasto: ACK 7235178, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: afb27bb386855cb8d5cf84fd3a6c11ef1160b25af6175ed0aa146bf04b9a26eb77298df70df0a855f8c46f19f08b3f62c49872c12974fcfa5526a15ee05b3c10

284a969 Linter to check commit message formatting (Amir Ghorbanian) Pull request description: Write linter to check that commit messages have a new line before the body or no body at all. fixes issue bitcoin#19091. ACKs for top commit: troygiorshev: ACK 284a969 Reviewed, manually tested. Works great! fjahr: tested ACK 284a969 adamjonas: utACK 284a969 Tree-SHA512: fa278f090780b54e4fa6e2967a62b4c1a4da55d112ec1ad6dd7e1181ac490c5c1af0165524b5781b463fdd6d0f79fd3d95b5160184e6eca432ccff1189f77390

…ssage linter 7235178 lint: Remove travis env var from commit linter (Fabian Jahr) Pull request description: bitcoin#19439 was recently merged and seemed to work fine but I now noticed strange behavior when it was running in Travis, which I could not reproduce locally. It turns out `TRAVIS_COMMIT_RANGE` which is used in Travis to get the commits for the linter, uses all the commits that were in a push, which includes all rebase commits for example. This means that the linter can fail on a commit that the developer has never even seen before, which can be very confusing. See an example here which caused me to look into this: https://travis-ci.org/github/bitcoin/bitcoin/jobs/714296381 The commit that is reported as failing in my PR is not part of my PR. I think we rather want to use something like `git merge-base` to get the commit range by default and in Travis. I am leaving the env variable functionality in place with a different name but this is not a variable that can be expected to be present in the CI environments so the `merge-base` range should be used there by default. ACKs for top commit: hebasto: ACK 7235178, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: afb27bb386855cb8d5cf84fd3a6c11ef1160b25af6175ed0aa146bf04b9a26eb77298df70df0a855f8c46f19f08b3f62c49872c12974fcfa5526a15ee05b3c10

284a969 Linter to check commit message formatting (Amir Ghorbanian) Pull request description: Write linter to check that commit messages have a new line before the body or no body at all. fixes issue bitcoin#19091. ACKs for top commit: troygiorshev: ACK 284a969 Reviewed, manually tested. Works great! fjahr: tested ACK 284a969 adamjonas: utACK 284a969 Tree-SHA512: fa278f090780b54e4fa6e2967a62b4c1a4da55d112ec1ad6dd7e1181ac490c5c1af0165524b5781b463fdd6d0f79fd3d95b5160184e6eca432ccff1189f77390

…ssage linter 7235178 lint: Remove travis env var from commit linter (Fabian Jahr) Pull request description: bitcoin#19439 was recently merged and seemed to work fine but I now noticed strange behavior when it was running in Travis, which I could not reproduce locally. It turns out `TRAVIS_COMMIT_RANGE` which is used in Travis to get the commits for the linter, uses all the commits that were in a push, which includes all rebase commits for example. This means that the linter can fail on a commit that the developer has never even seen before, which can be very confusing. See an example here which caused me to look into this: https://travis-ci.org/github/bitcoin/bitcoin/jobs/714296381 The commit that is reported as failing in my PR is not part of my PR. I think we rather want to use something like `git merge-base` to get the commit range by default and in Travis. I am leaving the env variable functionality in place with a different name but this is not a variable that can be expected to be present in the CI environments so the `merge-base` range should be used there by default. ACKs for top commit: hebasto: ACK 7235178, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: afb27bb386855cb8d5cf84fd3a6c11ef1160b25af6175ed0aa146bf04b9a26eb77298df70df0a855f8c46f19f08b3f62c49872c12974fcfa5526a15ee05b3c10

DrahtBot added the Tests label Jul 3, 2020

fanquake added Scripts and tools and removed Tests labels Jul 3, 2020

am1r021 force-pushed the linter-addition branch from ef28c5e to acc86bd Compare July 6, 2020 13:41

adamjonas reviewed Jul 7, 2020

View reviewed changes

troygiorshev reviewed Jul 7, 2020

View reviewed changes

am1r021 force-pushed the linter-addition branch from acc86bd to 6e8feaa Compare July 8, 2020 15:11

am1r021 commented Jul 8, 2020

View reviewed changes

maflcko reviewed Jul 8, 2020

View reviewed changes

am1r021 force-pushed the linter-addition branch 2 times, most recently from cf714e4 to a62b825 Compare July 12, 2020 20:17

troygiorshev suggested changes Jul 15, 2020

View reviewed changes

test/lint/lint-git-commit-check.sh Outdated Show resolved Hide resolved

Linter to check commit message formatting

284a969

Write linter to check that commit messages have a new line before the body or no body at all. reference: gist.github.com/agnivade/67b42d664ece2d4210c7 Fixes issue bitcoin#19091.

am1r021 force-pushed the linter-addition branch from b2e8a42 to 284a969 Compare July 15, 2020 14:02

maflcko merged commit edec7f7 into bitcoin:master Jul 30, 2020

yahiheb mentioned this pull request Jul 30, 2020

Write linter to check that commit messages have a new line before the body (or no body) #19091

Closed

fjahr mentioned this pull request Aug 4, 2020

lint: Don't use TRAVIS_COMMIT_RANGE in commit message linter #19654

Merged

bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022

	echo "No commit message formatting errors detected."
	exit ${EXIT_CODE}

	echo "Your subject line follows a non-empty line. Subject lines should always be followed by a blank line."
	echo "Your body follows a non-empty line. Subject lines should always be followed by a blank line."

	echo "Your subject line follows a non-empty line. Subject lines should always be followed by a blank line."
	echo "Your subject line is followed by a non-empty line. Subject lines should always be followed by a blank line."

Conversation

am1r021 commented Jul 3, 2020 • edited by maflcko Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjonas left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

troygiorshev left a comment

Choose a reason for hiding this comment

Uh oh!

troygiorshev Jul 7, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

am1r021 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

troygiorshev left a comment • 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!

troygiorshev left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

troygiorshev commented Jul 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fjahr commented Jul 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adamjonas commented Jul 23, 2020

Uh oh!

fjahr commented Aug 2, 2020

Uh oh!

fjahr commented Aug 4, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

am1r021 commented Jul 3, 2020 •

edited by maflcko

Loading

troygiorshev Jul 7, 2020 •

edited

Loading

troygiorshev left a comment •

edited

Loading

troygiorshev commented Jul 16, 2020 •

edited

Loading

fjahr commented Jul 17, 2020 •

edited

Loading