On 5/28/19 9:42 AM, Liam Girdwood wrote:

On Tue, 2019-05-28 at 17:26 +0300, Daniel Baluta wrote:

...

Hello all,

I have few comments about commits in SOF FW which I think might improve the review quality.

1. I've seen commit messages that do not explain WHY the

change is needed but mostly saying WHAT the change is doing.

Explaining WHAT the change is doing is good when the code is fairly complicated. Anyhow, I would prefer to see more of WHY do we need that change.

Lets try to keep this in mind.

2. Commit messages do not contain a verb.

Commits should be seen as actions thus should contain a verb. More than that we should use imperative mood.

Not added, fixed, etc but Add, Fix, etc!

3. Capitalize the subject line

First letter from commit subject after the subsystem tag should be capital.

e.g. buffer: add new interface -> buffer: Add new interface

I think we should turn our attention to:

https://chris.beams.io/posts/git-commit/#seven-rules

thanks, Daniel. _______________________________________________

Ack, good points for everyone to use when creating new PRs and patches.

Yep, more that 50% of my comments for kernel patches are "fix your commit messages" and it's even worse for SOF proper.

<snip>

Personally I capitalise the subject line only if it doesn't contain any prefixes, e.g.

37624b5 Linux 5.1-rc7

or

b2a20fd Merge branch 'bnxt_en-Misc-bug-fixes'

etc. I.e. really only special cases. When there are prefixes I personally optically prefer

8118b82 mm/page_alloc.c: fix never set ALLOC_NOFRAGMENT flag

to

ef61eb4 USB: yurex: Fix protection fault after device removal

As far as I know in English a colon should be followed by lower case, it's in German that it is followed by upper case. So, I would appreciate it if we didn't try to enforce this, at least not until there's a corresponding entry in CodingStyle ;-) We also could collect some statistics. E.g. of the last 5000 commits in my current Linux tree, that contain a colon in the subject, 2692 begin with a lower case after the last colon and 1825 with upper case.

I see your point on this and I agree! No enforcement for capital letter after prefix strings.

Anyhow, I am stressing again on writing good commit messages which explain WHY the change is needed.

thanks, Daniel.