[#1593] Make commit message verification more robust

[orecham]

Notes for Reviewer

1. Add just script for commit verification; remove corresponding shell script
2. Properly retrieve commits in branch
3. Ensure subject begin with the issue number [#XYZ]
4. Ensure message begins with a capital letter
5. Utilize imperative crate to check for imperative mood
   1. As per recommendation in the commit guidelines
   2. Does not catch everything but catches common offenders

The imperative mood check can be omitted if there is worry about false positives.

Pre-Review Checklist for the PR Author

• Add sensible notes for the reviewer
• PR title is short, expressive and meaningful
• Consider switching the PR to a draft (Convert to draft)
  • as draft PR, the CI will be skipped for pushes
• Relevant issues are linked in the References section
• Branch follows the naming format (iox2-123-introduce-posix-ipc-example)
• Commits messages are according to this guideline
  • Commit messages have the issue ID ([#123] Add posix ipc example)
  • Keep in mind to use the same email that was used to sign the Eclipse Contributor Agreement
• Tests follow the best practice for testing
• Changelog updated in the unreleased section including API breaking changes

PR Reviewer Reminders

• Commits are properly organized and messages are according to the guideline
• Unit tests have been written for new behavior
• Public API is documented
• PR title describes the changes

References

Closes #1593

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 76.26%. Comparing base (6db1e5a) to head (73dd4e1).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1594      +/-   ##
==========================================
- Coverage   76.29%   76.26%   -0.03%     
==========================================
  Files         415      415              
  Lines       39772    39772              
  Branches     1321     1321              
==========================================
- Hits        30345    30334      -11     
- Misses       8353     8364      +11     
  Partials     1074     1074              

Flag	Coverage Δ
CPP	66.44% <ø> (ø)
Rust	76.14% <ø> (-0.03%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.
see 10 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[elfenpiff]

I do not like those guidelines at all, and also the imperative mood.

If the rest of the crew:
@elBoberido @ComicSansMS @dkroenke @FerdinandSpitzschnueffler @budrus

Like it, I will not intervene here. But the gains are, in my opinion, nearly nonexistent. And multi line commit messages with detailed lists and explanations are just wrong (why add a guideline for something that clearly belongs into the code and not hidden in a commit message)

When I am looking for a specific commit with lazygit, I first take a look at the changes, and if this is not sufficient, I will take a look at the commit message.

But I do not care for wording or spelling; it must be short and summarize what happened in this commit. If it is a capital case, a comma is missing, the line length is 50 or 120, I don't care.
But I care that the CI is not adding another check that lets my PR fail because my commit message didn't start with a capital letter, or was in a non-imperative mood.

[orecham]

@elfenpiff I like the guideline. The imperative mood check encourages grouping changes into atomic units (whether the content of the commit actually follows this is another question) and if done correctly makes cherry picking easier.

The capitialiation I agree is nitpicky but I enjoy a clean commit log. The friction it adds is not greater than enforicing the issue number is tagged - just reword the commit.

[FerdinandSpitzschnueffler]

I agree with @orecham and already follow this guideline (apart from the line length - 50 is sometimes too short). Additionally, the checklist in our PR template requires this, so it makes sense to check it. If we decide against, we should also remove this point from the checklist.

[elfenpiff]

If you like it, then I am fine with it. @orecham would it be possible to set the line length to 110. This is also the line length we have in Rust, C++ and Python. I see no reason why git should be here an exception.

[orecham]

There is no line check at the moment. We can leave it out to allow some flexibility. The check only fails on the things I deemed worth being strict about.

[orecham]

@elBoberido This is a matter of taste, so I guess we will never all agree but we will need to settle on a convention.

I am in favor of a commit history that reads like steps building upon each other and especially value cherry-pickability (the convention doesn't guarantee this but it does encourage it). Commits like: [#XYZ] first attempt are more effort for me to reason about and work with.

[elBoberido]

@orecham fully agree and I also do this and try to have atomic commits which are building. The issue is just with casual contributors and all the things they have to do to be able to merge a PR. The ECA is already a big hurdle and I fear we scare some of them away if we are too strict with some stuff that is not that important. We have to find a good balance.

[elfenpiff]

@orecham @elBoberido can we wrap up the discussion here and merge it? The [#123] prefix check is definitely valuable.

[orecham]

@elfenpiff @elBoberido I propose this:

1. Keep the issue number check
2. Keep the imperative mood check
3. Drop the requirement to start first word with capital letter

[elBoberido]

I'm all for the issue number check. Because of the reasons mentioned above, I'm skeptical about the other things but I leave it to you two to bikeshed this out.

[orecham]

TODO: document commits linting check