1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
|
# Contributing Guidelines
This document attempts to outline some basic rules to follow when contributing
to OpenBMC's IPMI stack. It does *not* outline coding style; we follow the
[OpenBMC C++ style guide](https://github.com/openbmc/docs/blob/master/cpp-style-and-conventions)
for that.
## Organizing Commits
A good commit does exactly one thing. We prefer many small, atomic commits to
one large commit which makes many functional changes.
- Too large: "convert foo to new API; also fix CVE 1234 in bar"
- Too small: "move abc.h to top of include list" and "move xyz.h to bottom of
include list"
- Just right: "convert foo to new API" and "convert foo from tab to space"
Often, creating small commits this way results in a number of commits which are
dependent on prior commits; Gerrit handles this situation well, so feel free to
push commits which are based on your change still in review. However, when
possible, your commit should stand alone on top of master - "Fix whitespace in
bar()" does not need to depend on "refactor foo()". Said differently, ensure
that topics which are not related to each other semantically are also not
related to each other in Git until they are merged into master.
When pushing a stack of patches (current branch is >1 commits ahead of
origin/master), these commits will show up with that same relationship in
gerrit. This means that each patch must be merged in order of that relationship.
So if one of the patches in the middle needs to be changed, all the patches from
that point on would need to be pushed to maintain the relationship. This will
effectively rebase the unchanged patches, which would in turn trigger a new CI
build. Ideally, changes from the entire patchset could be done all in one go to
reduce unnecessary rebasing.
When someone makes a comment on your commit in Gerrit, modify that commit and
send it again to Gerrit. This typically involves `git rebase --interactive` or
`git commit --amend`, for which there are many guides online. As mentioned in
the paragraph above, when possible you should make changes to multiple patches
in the same stack before you push, in order to minimize CI and notification
churn from the rebase operations.
Commits which include changes that can be tested by a unit test should also
include a unit test to exercise that change, within the same commit. Unit tests
should be clearly written - even moreso than production code, unit tests are
meant primarily to be read by humans - and should test both good and bad
behaviors. Refer to the
[testing documentation](https://github.com/openbmc/phosphor-host-ipmid/blob/master/docs/testing.md)
for help writing tests, as well as best practices.
## Formatting Commit Messages
Your commit message should explain:
- Concisely, *what* change are you making? e.g. "docs: convert from US to UK
spelling" This first line of your commit message is the subject line.
- Comprehensively, *why* are you making that change? In some cases, like a
small refactor, the why is fairly obvious. But in cases like the inclusion of
a new feature, you should explain why the feature is needed.
- Concisely, *how* did you test? This comes in the form of a "Tested:" footer
in your commit message and is required for all code changes in the IPMI
stack. It typically consists of copy-pasted ipmitool requests and responses.
When possible, use the high-level ipmitool commands (e.g. "ipmitool sensor
read 0x1"). In cases where that's not possible, or when testing edge or error
cases, it is acceptable to use "ipmitool raw" - but an explanation of your
output is appreciated. If the change can be validated entirely by running
unit tests, say so in the "Tested:" tag.
Try to include the component you are changing at the front of your subject line;
this typically comes in the form of the class, module, handler, or directory you
are modifying. e.g. "apphandler: refactor foo to new API"
Loosely, we try to follow the 50/72 rule for commit messages - that is, the
subject line should not exceed 50 characters and the body should not exceed 72
characters. This is common practice in many projects which use Git.
All commit messages must include a Signed-off-by line, which indicates that you
the contributor have agreed to the Developer Certificate of Origin. This line
must include the name you commonly use, often a given name and a family name or
surname. (ok: A. U. Thor, Sam Samuelsson, robert a. heinlein; not ok:
xXthorXx, Sam, RAH)
## Sending Patches
Like most projects in OpenBMC, we use Gerrit to review patches. Please check
the MAINTAINERS file to determine who needs to approve your review in order for
your change to be merged. Submitters will need to manually add their reviewers
in Gerrit; reviewers are not currently added automatically. Maintainers may not
see the commit if they have not been added to the review!
## Pace of Review
Contributors who are used to code reviews by their team internal to their own
company, or who are not used to code reviews at all, are sometimes surprised by
the pace of code reviews in open source projects. Try to keep in mind that those
reviewing your patch may be contributing to OpenBMC in a volunteer or
partial-time capacity, may be in a timezone far removed from your own, and may
have very deep review queues already of patches which have been waiting longer
than yours.
If you feel your patch has been missed entirely, of course it's
alright to email the maintainers (addresses available in MAINTAINERS file) - but
a reasonable timeframe to do so is on the order of a week, not on the order of
hours.
Additionally, the IPMI stack has a set of policies for when and how changes can
be approved; please check the MAINTAINERS file for the full list ("Change
approval rules").
The maintainers' job is to ensure that incoming patches are as correct and easy
to maintain as possible. Part of the nature of open source is attrition -
contributors can come and go easily - so maintainers tend not to put stock in
promises such as "I will add unit tests in a later patch" or "I will be
implementing this proposal by the end of next month." This often manifests as
reviews which may seem harsh or exacting; please keep in mind that the community
is trying to collaborate with you to build a patch that will benefit the project
on its own.
## Code of Conduct
We enthusiastically adhere to the same
[Code of Conduct](https://github.com/openbmc/docs/blob/master/code-of-conduct.md)
as the rest of OpenBMC. If you have any concerns, please check that document for
guidelines on who can help you resolve them.
|
