From a36974585f67171bafed13a6a972364371530d1b Mon Sep 17 00:00:00 2001 From: Valentin Rothberg Date: Mon, 29 Oct 2018 10:50:03 +0100 Subject: CONTRIBUTING.md: add section about describing changes Add a section about describing changes in commit messages. GitHub tends to drive the large part of discussions and change descriptions to the corresponding pull requests and issues, but such information is lost in the git history. Not providing sufficient information in commit messages is painful for reviewing and can cause issues while debugging. It also complicates studying source code, where reading commit messages and the code's git history is a common approach to better understand the code. Following the descriptions should be enforced by the maintainers of the libpod project. Pull requests containing commits without proper descriptions should not be merged. This change bases on the documentation of the Linux kernel v4.17: https://www.kernel.org/doc/html/v4.17/process/submitting-patches.html Signed-off-by: Valentin Rothberg --- CONTRIBUTING.md | 73 +++++++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 71 insertions(+), 2 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index fa95bfe3a..c4e208894 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -52,11 +52,80 @@ too since in the end the entire PR will be reviewed anyway. When in doubt, squash. PRs that fix issues should include a reference like `Closes #XXXX` in the -commit message so that github will automatically close the referenced issue +commit message so that GitHub will automatically close the referenced issue when the PR is merged. PRs will be approved by an [approver][owners] listed in [`OWNERS`](OWNERS). +### Describe your Changes in Commit Messages + +Describe your problem. Whether your patch is a one-line bug fix or 5000 lines +of a new feature, there must be an underlying problem that motivated you to do +this work. Convince the reviewer that there is a problem worth fixing and that +it makes sense for them to read past the first paragraph. + +Describe user-visible impact. Straight up crashes and lockups are pretty +convincing, but not all bugs are that blatant. Even if the problem was spotted +during code review, describe the impact you think it can have on users. Keep in +mind that the majority of users run packages provided by distributions, so +include anything that could help route your change downstream. + +Quantify optimizations and trade-offs. If you claim improvements in +performance, memory consumption, stack footprint, or binary size, include +numbers that back them up. But also describe non-obvious costs. Optimizations +usually aren’t free but trade-offs between CPU, memory, and readability; or, +when it comes to heuristics, between different workloads. Describe the expected +downsides of your optimization so that the reviewer can weigh costs against +benefits. + +Once the problem is established, describe what you are actually doing about it +in technical detail. It’s important to describe the change in plain English for +the reviewer to verify that the code is behaving as you intend it to. + +Solve only one problem per patch. If your description starts to get long, +that’s a sign that you probably need to split up your patch. + +If the patch fixes a logged bug entry, refer to that bug entry by number and +URL. If the patch follows from a mailing list discussion, give a URL to the +mailing list archive. + +However, try to make your explanation understandable without external +resources. In addition to giving a URL to a mailing list archive or bug, +summarize the relevant points of the discussion that led to the patch as +submitted. + +If you want to refer to a specific commit, don’t just refer to the SHA-1 ID of +the commit. Please also include the oneline summary of the commit, to make it +easier for reviewers to know what it is about. Example: + +``` +Commit f641c2d9384e ("fix bug in rm -fa parallel deletes") [...] +``` + +You should also be sure to use at least the first twelve characters of the +SHA-1 ID. The libpod repository holds a lot of objects, making collisions with +shorter IDs a real possibility. Bear in mind that, even if there is no +collision with your six-character ID now, that condition may change five years +from now. + +If your patch fixes a bug in a specific commit, e.g. you found an issue using +git bisect, please use the ‘Fixes:’ tag with the first 12 characters of the +SHA-1 ID, and the one line summary. For example: + +``` +Fixes: f641c2d9384e ("fix bug in rm -fa parallel deletes") +``` + +The following git config settings can be used to add a pretty format for +outputting the above style in the git log or git show commands: + +``` +[core] + abbrev = 12 +[pretty] + fixes = Fixes: %h (\"%s\") +``` + ### Sign your PRs The sign-off is a line at the end of the explanation for the patch. Your @@ -127,7 +196,7 @@ Integration Tests [README.md](test/README.md). For general questions and discussion, please use the IRC `#podman` channel on `irc.freenode.net`. -For discussions around issues/bugs and features, you can use the github +For discussions around issues/bugs and features, you can use the GitHub [issues](https://github.com/containers/libpod/issues) and [PRs](https://github.com/containers/libpod/pulls) -- cgit v1.2.3-54-g00ecf