Establishing Commit Message Guidelines

[iamniting]

We want to keep our commit messages and git history neat and organized. I usually follow the guidelines in [1], except for the title length, which is suggested to be 50 characters. Personally, I find 72 characters more practical. I've noticed differing opinions on how to write the commit messages. I'd love to hear from everyone about how they approach writing commit messages and what they think is the best way. Let's discuss and decide on the most effective approach together!

[1] https://cbea.ms/git-commit/

[subhamkrai]

@iamniting In rook we use commitlint github action which use this configuration https://github.com/rook/rook/blob/master/.commitlintrc.json for commit title and description

[nb-ohad]

I have no issues with limitations on commits title. I do find it limiting and concerning to limit the line length for commit message lines.

The way I see it is that the reason behind limiting anything is to try to get to a descriptive and concise explanation of the commit content/changes.

For contributors who are very articulate and for changes that need lengthy explanations, an artificial limitation on line length will only result in an artificial line breaks with no real intention, or need, to break the sentence itself. The results, at least in English, are not natural to a reader who is expecting either a period or a line separation to separate sentences and paragraphs.

In addition, these kinds of semantic breaks in sentences (as opposed to visual breaks) do not play nicely with different resolutions, especially when consuming the content on smaller screens (like a phone screen)

As an example, lets take the following commit message:

The author meant to convey a single sentence This commit adds spec changes to StorageCluster to support additional parameters for Noobaa to use an external Postgres server

But was forced to break it into 3 different lines (which still looks fine here)

When looking at the same screen via a phone's narrower screen you will get

Which is definitely presented as 3 different sentences as opposed to only one. The cause is the artificial break of the original line into 3 different ones.

I will also highlight the use case of consuming commit descriptions via Github/Git APIs to be processed by an external tool. In that case, most tools will treat hard-line separation (new lines) as different sentences. This can hinder the compatibility of our repo with analysis tools.

My recommendation and vote is to remove any limitations on the length of description lines and address the concern of confusing or non-coherent descriptions as part of the PR review process.

[iamniting]

@nbalacha @jarrpa @umangachapagain @travisn @obnoxxx @agarwal-mudit @raghavendra-talur @ShyamsundarR

Can you pls provide your thoughts as well?

[travisn]

I would summarize my main concerns about commit messages into two points:

1. Have a good, brief subject, that describes the purpose for the change as much as possible in a single line.
   • Avoid subjects that only say it's a general "bug fix" or "update" to a file, since those comments are not helpful context at all.
2. The description paragraph should give as much context as necessary to understand the purpose for the change.
   • If the commit message is too long, that detail should be added as comments in the code, or at least duplicated to the code for better discoverability of those details.

Beyond those two points which really need to be enforced by maintainers rather than CI tools, the formatting restrictions are less important to me.

[nbalacha]

Most of the 72 char restrictions for the length of the description lines appear to date back several years and were introduced to work with the tools available at that time. If the tools used now no longer have such restrictions, we could do away with the max 72 char requirement for description lines. The summary line should be restricted in length to 50 chars as is the current practice. If consistency is a requirement, then the 72 description line char limit can be kept.

[ShyamsundarR]

I tend to agree with @travisn

IMHO a commit message linter is needed only when there are restrictions to be applied for example:

• Commit message subject shall start with certain keywords
• Commit message body shall contain an issue reference
• Backport commit messages shall contain a cherry-picked from commit reference
• etc.

If such rules need to be enforced a linter is useful, such that the feedback is from a tool rather than a person and also a reviewer need not (mostly) worry about the rules as such.

Beyond that a simple subject/body for EACH commit should be reviewed for correctness by the reviewer anyway, and if there are commit message guidelines (like the post you mention above @iamniting ) just make sure it is part of the CONTRIBUTING or such guide, such that the reviewer can point to the same for an acceptable practice.

(my bigger nit is smaller commits and a commit chain that is bisect-able, but that is a different topic)

[obnoxxx]

I like the idea of writing down commit message requirements. the contributing guide of this repo has some description of requirements for commit messages: https://github.com/red-hat-storage/ocs-operator/blob/main/CONTRIBUTING.md

But reviewing it now, I thought that we had added more details when we originally created the guide. somehow, it seems to have been lost in the cracks of time ... So let's expand it!

for comparison, the samba-operator project has more details: https://github.com/samba-in-kubernetes/samba-operator/blob/master/docs/CONTRIBUTING.md

for linting the commit message formatting, the project tried commitlint first, as recommended by the rook project, but it was found to be too picky for no good reason, and then samba-operator chose gitlint for commit message linting instead. This is very successful so far.

Apart from formatting requirements, for me the content of the commit message is usually more important.

I prefer to see the following points covered:

• on a high level, what is the effect of the commit ( e.g. change in behavior)
• why is it needed (e. g. which misbehavior does it fix)
• how is the change done (only if not obvious, i. e. don't repeat the patch content))
• why is it done this way (if there are alterative solutions)

these points can hardly be covered by automatic linters but have to be checked by reviewers.

FWIW, I usually apply similar criteria to code commits to avoid pointless comments

Just a few cents of mine ...

[raghavendra-talur]

Others have already chimed in with the suggestions I would have had.

About the char limits for the subject and body, I think we don't need to restrict it to a very low number like 50(helpful when patches are reviewed in a email client). However, it is always good to have a agreed upon number which suits the tools currently used. A higher char limit like 120 or 150(which suits github reviews on a typical resolution monitor) is good.