docs: introduce commit message guideline
Changes PlannedPublic

Authored by romangg on Tue, Aug 6, 3:46 PM.

Details

Reviewers
None
Group Reviewers
KWin
Summary

We did not have an official guideline on how commit messages are supposed to
be formatted.

Inofficially we used something like [compositor] or [platforms/drm] as tags on
the subject line. But the square brackets in the beginning can lead to issues
with certain tools.

This patch specifies an official guideline which is mostly based on the one
developed in the Angular project:
https://github.com/angular/angular/blob/master/CONTRIBUTING.md#commit

Using the existing guideline of another big open source project mostly
unchanged hopefully allows us to set this guideline once and without major
changes afterwards.

Some details had to be changed though to take account of KWin's different
programming language and internal structure.

Diff Detail

Repository
R108 KWin
Branch
commitMessageGuideline
Lint
Lint OK
Unit
No Unit Test Coverage
Build Status
Buildable 14956
Build 14974: arc lint + arc unit
romangg created this revision.Tue, Aug 6, 3:46 PM
Restricted Application added a project: KWin. · View Herald TranscriptTue, Aug 6, 3:46 PM
Restricted Application added a subscriber: kwin. · View Herald Transcript
romangg requested review of this revision.Tue, Aug 6, 3:46 PM

The list of types need to be reconsidered. I believe we need build type but maybe not ci.

ngraham added a subscriber: ngraham.Tue, Aug 6, 4:45 PM

This long list of rules is rather hard to visualize without providing examples. I think a few would help.

zzag added a subscriber: zzag.Wed, Aug 7, 9:45 AM

Perhaps it's also worth to mention special git commit message keywords, e.g. BUG, CCBUG, FEATURE, FIXED-IN, CCMAIL, and so on in the footer section.

HACKING.md
120

Perhaps we could omit ci for now.

147

What's the main argument for keeping the first letter in lower case?

If the first letter is not capitalized, then <subject> blends with <type>(<scope>):.

IMHO, fix: Add more bugs to fix later is much better than fix: add more bugs to fix later.

romangg updated this revision to Diff 63293.Wed, Aug 7, 3:49 PM
  • Remove ci type, note about special keywords
romangg added inline comments.Wed, Aug 7, 3:50 PM
HACKING.md
147

I assume it's because in English language you are not supposed to capitalize after colon. I don't directly prefer one over the other after using the non-capitalized way in English text now for quite some time but preferred the capitalized way in the beginning since I was used to it from German.

zzag added a comment.EditedThu, Aug 8, 9:07 AM

Scopes are a bit tricky. KWin core contains lots of stuff, so it'll be difficult to figure out what one should use as a scope.

We could put component name for example

fix(core): Prevent dangling pointers in the stacking order
feat(colorcorrect): Add gamma and brightness options
fix(core): Calculate correct visible bounds of ShellClient
fix(core): Keep the outline below visual parent
core: Rework frame skipping logic
platfroms-x11: Require XI2
platforms-wayland: Forward axis_source to platform
(platforms: or nothing?) Drop fbdev platform

Alternatively, we could put "topic," e.g.

fix(core): Prevent dangling pointers in the stacking order
feat(nightcolor): Add gamma and brightness options
fix(wayland): Calculate proper visible bound of xdg_surface
fix(core): Keep the outline below visual parent
compositor: Rework frame skipping logic
(x11: or platforms-x11:) Require XI2
(wayland: or platforms-wayland:) Forward axis_source to platform
(platforms: or nothing?) Drop fbdev platform
zzag added inline comments.Thu, Aug 8, 9:11 AM
HACKING.md
133–134

These two are not modules.

romangg added inline comments.Thu, Aug 8, 9:18 AM
HACKING.md
133–134

I haven't looked up the definition of a "module". Is it something specific? Let's just say topic as you recommended in the other comment? Or "area of interest"?

zzag added inline comments.Thu, Aug 8, 9:30 AM
HACKING.md
133–134

Or "area of interest"?

Yep, this sounds good.

romangg added inline comments.Thu, Aug 8, 9:34 AM
HACKING.md
161

The "Breaking Changes" was copied over from the Angular guide lines, but maybe we should remove it here. For once because we don't really have defined what breaking changes are in KWin. Is a change to the Effects API/ABI a breaking change? Is a behavioral change for users one?

anthonyfieroni added inline comments.
HACKING.md
161

If KWin loads all its modules at start it sounds unchanged to users, otherwise breaking API/ABI will result in crash at time to load an incompatible plugin.

yurchor added a subscriber: yurchor.Thu, Aug 8, 5:55 PM

Thanks in advance for fixing these minor typos.

HACKING.md
154

Typo: As first part -> As the first part

156

Typo: ark -> arc

161

Typo: with a space -> with space

romangg updated this revision to Diff 63483.Sat, Aug 10, 1:55 PM
romangg marked 2 inline comments as done.
  • Typos

@yurchor: Thanks for the feedback. Corrected now most of them.

@zzag, @davidedmundson What do you think about the "Breaking Changes" part? Just leave it out for now?

zzag added inline comments.Sat, Aug 10, 4:21 PM
HACKING.md
161

We can drop this section as we check libkwineffects API version.

Is a change to the Effects API/ABI a breaking change?

Third-party effects need to be recompiled if API version of libkwineffects is bumped. It's a good idea to compile your third-party effect against each major release of KWin.

zzag added a comment.Sat, Aug 10, 4:25 PM

The only comment I have is about capitalization. It's better to capitalize <subject> as the prefix serves a bit special purpose.

Other than that, it's +1 from me.

romangg updated this revision to Diff 63495.Sat, Aug 10, 6:42 PM
  • Remove section about breaking changes
In D22973#509840, @zzag wrote:

The only comment I have is about capitalization. It's better to capitalize <subject> as the prefix serves a bit special purpose.

I don't really follow this argument and would like to keep it as in the other project since they probably had some good reason on why they decided to do it like that. Maybe @davidedmundson can chime in as native speaker. There is a difference between British and American English though.

Other than that, it's +1 from me.

zzag added a comment.EditedSat, Aug 10, 7:08 PM

I don't really follow this argument

The prefix is not part of a proper sentence. It has a bit special purpose, we just use a colon to separate it from the rest of subject line. Therefore, we can follow our own rules. Colon is just a stylistic choice I guess.

I prefer to capitalize the "subject" because this way it's much easier to spot where the actual short description of a change begins when browsing git log.

In D22973#509896, @zzag wrote:

I don't really follow this argument

The prefix is not part of a proper sentence. It has a bit special purpose, we just use a colon to separate it from the rest of subject line. Therefore, we can follow our own rules. Colon is just a stylistic choice I guess.

That's the same usage as in Angular project. Still they decided to use lower case.

I prefer to capitalize the "subject" because this way it's much easier to spot where the actual short description of a change begins when browsing git log.

What type of change it is (and scope) is just as important as the subject when browsing the log.

zzag added a comment.Sat, Aug 10, 10:25 PM

What type of change it is (and scope) is just as important as the subject when browsing the log.

It is important but not as the subject. The subject still carries more useful information about the change.

HACKING.md
131

Perhaps slashes are better than hyphens as we are more used to it.

To be honest this discussion about the necessity/preference of capitalizing a single letter doesn't lead to much and I don't want to continue it any more. I don't agree with the provided arguments and see no reason to deviate from the recommendation of Angular project and FWIW https://www.conventionalcommits.org. Either we take the current revision as it is or not at all.

Also please stop adding patches following a not yet agreed upon guideline and diverging from current best practice.

HACKING.md
131

Slashes are used to indicate alternatives in normal text. That's why I opted for some other symbol.

romangg planned changes to this revision.Mon, Aug 12, 3:06 PM

If at all this will be embedded into a push on wider scope in all of KDE or at least Plasma and Frameworks, see https://mail.kde.org/pipermail/plasma-devel/2019-August/101494.html.

Will have to wait till after GitLab transition.