NixOS / nixpkgs

Nix Packages collection & NixOS
MIT License
18.18k stars 14.19k forks source link

"Enforce" better commit messages for Nixpkgs #160745

Open primeos opened 2 years ago

primeos commented 2 years ago

I increasingly notice PRs with important but non-trivial changes that lack an appropriate commit message and it's getting kinda annoying to tell every author. It also often happens that the author did write a description for the PR but the commit message has only a subject and no body (a bad pattern that emerged on GitHub).

IMO it'd be great if we could document the need for a good commit message more prominently.

E.g., the following is already good: https://github.com/NixOS/nixpkgs/blob/3ddc71525fd0d12083620b8d2b181f62e73ffff0/CONTRIBUTING.md#writing-good-commit-messages:

In addition to writing properly formatted commit messages, it's important to include relevant information so other developers can later understand why a change was made. While this information usually can be found by digging code, mailing list/Discourse archives, pull request discussions or upstream changes, it may require a lot of work.

A link to https://cbea.ms/git-commit/ or another source would also be a good addition IMO (possibly even copying "The seven rules of a great Git commit message"). We might also want to explicitly mention that the commit messages for non-trivial changes should have a body that explains what, why, and how.

Depending on how often too brief / bad commit messages are a problem we might also want to add an item to the PR template: https://github.com/NixOS/nixpkgs/blob/3ddc71525fd0d12083620b8d2b181f62e73ffff0/.github/PULL_REQUEST_TEMPLATE.md. Or e.g.:

-- [ ] Fits [CONTRIBUTING.md](https://github.com/NixOS/nixpkgs/blob/master/CONTRIBUTING.md).
+- [ ] Fits [CONTRIBUTING.md](../CONTRIBUTING.md) and has a [good commit message](../CONTRIBUTING.md#writing-good-commit-messages).

For package version upgrades and such a one-line commit message is usually sufficient.

That is of course fine (assuming it's a trivial change that doesn't need any explanation).

It looks like the requirement for an appropriate commit message also isn't mentioned here: https://nixos.org/manual/nixpkgs/stable/#chap-reviewing-contributions

rnhmjoj commented 2 years ago

We might also want to explicitly mention that the commit messages for non-trivial changes should have a body that explains what, why, and how.

Yes! This is what I hope to find in a commit message, particularly if it comes up from a git bisect. Most of the time the message is very brief and the bulk of the explanation is in the pull request. This is not a good practice because that information can be lost forever, for example if the author deletes their comments or github bans the account (it has happened at least once in Nixpkgs).

PS: I'm also a fan of commit message tales, hope to see some more.

Mindavi commented 2 years ago

I also like the recent change to the contributing guidelines to link the release notes in a commit message, making it easier to see what changed and possibly spot why a package regressed or doesn't do what you expect anymore. https://github.com/NixOS/nixpkgs/pull/167264

Adding it explicitly to the contributing checkmark makes most sense to me to prevent adding more checkmarks.

rnhmjoj commented 7 months ago

I think it's fair to say that trust or dealing with malicious committers has nothing to do with this proposal:

In addition to writing properly formatted commit messages, it's important to include relevant information so other developers can later understand why a change was made. While this information usually can be found by digging code, mailing list/Discourse archives, pull request discussions or upstream changes, it may require a lot of work.

This is about making life easier for people coming about a commit (e.g. after a bisection or blame). It's invaluable if the commit message contains a clear explanation of what the patch does and why it was done (perhaps a link to an issue or PR).