]>
Commit | Line | Data |
---|---|---|
7c673cae FG |
1 | ========================== |
2 | Submitting Patches to Ceph | |
3 | ========================== | |
4 | ||
5 | This is based on Documentation/SubmittingPatches from the Linux kernel, | |
6 | but has pared down significantly and updated based on the Ceph project's | |
7 | best practices. | |
8 | ||
9 | The patch signing procedures and definitions are unmodified. | |
10 | ||
11 | ||
12 | SIGNING CONTRIBUTIONS | |
13 | ===================== | |
14 | ||
15 | In order to keep the record of code attribution clean within the | |
16 | source repository, please follow these guidelines for signing | |
17 | patches submitted to the project. These definitions are taken | |
18 | from those used by the Linux kernel and many other open source | |
19 | projects. | |
20 | ||
21 | ||
22 | 1. Sign your work | |
23 | ----------------- | |
24 | ||
25 | To improve tracking of who did what, especially with patches that can | |
26 | percolate to their final resting place in the kernel through several | |
27 | layers of maintainers, we've introduced a "sign-off" procedure on | |
28 | patches that are being emailed around. | |
29 | ||
30 | The sign-off is a simple line at the end of the explanation for the | |
31 | patch, which certifies that you wrote it or otherwise have the right to | |
32 | pass it on as a open-source patch. The rules are pretty simple: if you | |
33 | can certify the below: | |
34 | ||
35 | Developer's Certificate of Origin 1.1 | |
36 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |
37 | ||
38 | By making a contribution to this project, I certify that: | |
39 | ||
40 | (a) The contribution was created in whole or in part by me and I | |
41 | have the right to submit it under the open source license | |
42 | indicated in the file; or | |
43 | ||
44 | (b) The contribution is based upon previous work that, to the best | |
45 | of my knowledge, is covered under an appropriate open source | |
46 | license and I have the right under that license to submit that | |
47 | work with modifications, whether created in whole or in part | |
48 | by me, under the same open source license (unless I am | |
49 | permitted to submit under a different license), as indicated | |
50 | in the file; or | |
51 | ||
52 | (c) The contribution was provided directly to me by some other | |
53 | person who certified (a), (b) or (c) and I have not modified | |
54 | it. | |
55 | ||
56 | (d) I understand and agree that this project and the contribution | |
57 | are public and that a record of the contribution (including all | |
58 | personal information I submit with it, including my sign-off) is | |
59 | maintained indefinitely and may be redistributed consistent with | |
60 | this project or the open source license(s) involved. | |
61 | ||
62 | then you just add a line saying :: | |
63 | ||
64 | Signed-off-by: Random J Developer <random@developer.example.org> | |
65 | ||
66 | ||
67 | using your real name (sorry, no pseudonyms or anonymous contributions.) | |
68 | ||
69 | Some people also put extra tags at the end. They'll just be ignored for | |
70 | now, but you can do this to mark internal company procedures or just | |
71 | point out some special detail about the sign-off. | |
72 | ||
73 | If you are a subsystem or branch maintainer, sometimes you need to slightly | |
74 | modify patches you receive in order to merge them, because the code is not | |
75 | exactly the same in your tree and the submitters'. If you stick strictly to | |
76 | rule (c), you should ask the submitter to rediff, but this is a totally | |
77 | counter-productive waste of time and energy. Rule (b) allows you to adjust | |
78 | the code, but then it is very impolite to change one submitter's code and | |
79 | make them endorse your bugs. To solve this problem, it is recommended that | |
80 | you add a line between the last Signed-off-by header and yours, indicating | |
81 | the nature of your changes. While there is nothing mandatory about this, it | |
82 | seems like prepending the description with your mail and/or name, all | |
83 | enclosed in square brackets, is noticeable enough to make it obvious that | |
84 | you are responsible for last-minute changes. Example :: | |
85 | ||
86 | Signed-off-by: Random J Developer <random@developer.example.org> | |
87 | [lucky@maintainer.example.org: struct foo moved from foo.c to foo.h] | |
88 | Signed-off-by: Lucky K Maintainer <lucky@maintainer.example.org> | |
89 | ||
90 | This practise is particularly helpful if you maintain a stable branch and | |
91 | want at the same time to credit the author, track changes, merge the fix, | |
92 | and protect the submitter from complaints. Note that under no circumstances | |
93 | can you change the author's identity (the From header), as it is the one | |
94 | which appears in the changelog. | |
95 | ||
96 | Special note to back-porters: It seems to be a common and useful practise | |
97 | to insert an indication of the origin of a patch at the top of the commit | |
98 | message (just after the subject line) to facilitate tracking. For instance, | |
99 | here's what we see in 2.6-stable :: | |
100 | ||
101 | Date: Tue May 13 19:10:30 2008 +0000 | |
102 | ||
103 | SCSI: libiscsi regression in 2.6.25: fix nop timer handling | |
104 | ||
105 | commit 4cf1043593db6a337f10e006c23c69e5fc93e722 upstream | |
106 | ||
107 | And here's what appears in 2.4 :: | |
108 | ||
109 | Date: Tue May 13 22:12:27 2008 +0200 | |
110 | ||
111 | wireless, airo: waitbusy() won't delay | |
112 | ||
113 | [backport of 2.6 commit b7acbdfbd1f277c1eb23f344f899cfa4cd0bf36a] | |
114 | ||
115 | Whatever the format, this information provides a valuable help to people | |
116 | tracking your trees, and to people trying to trouble-shoot bugs in your | |
117 | tree. | |
118 | ||
119 | ||
120 | 2. When to use ``Acked-by:`` and ``Cc:`` | |
121 | ---------------------------------------- | |
122 | ||
123 | The ``Signed-off-by:`` tag indicates that the signer was involved in the | |
124 | development of the patch, or that he/she was in the patch's delivery path. | |
125 | ||
126 | If a person was not directly involved in the preparation or handling of a | |
127 | patch but wishes to signify and record their approval of it then they can | |
128 | arrange to have an ``Acked-by:`` line added to the patch's changelog. | |
129 | ||
130 | ``Acked-by:`` is often used by the maintainer of the affected code when that | |
131 | maintainer neither contributed to nor forwarded the patch. | |
132 | ||
133 | ``Acked-by:`` is not as formal as ``Signed-off-by:``. It is a record that the acker | |
134 | has at least reviewed the patch and has indicated acceptance. Hence patch | |
135 | mergers will sometimes manually convert an acker's "yep, looks good to me" | |
136 | into an ``Acked-by:``. | |
137 | ||
138 | ``Acked-by:`` does not necessarily indicate acknowledgement of the entire patch. | |
139 | For example, if a patch affects multiple subsystems and has an ``Acked-by:`` from | |
140 | one subsystem maintainer then this usually indicates acknowledgement of just | |
141 | the part which affects that maintainer's code. Judgement should be used here. | |
142 | When in doubt people should refer to the original discussion in the mailing | |
143 | list archives. | |
144 | ||
145 | If a person has had the opportunity to comment on a patch, but has not | |
146 | provided such comments, you may optionally add a "Cc:" tag to the patch. | |
147 | This is the only tag which might be added without an explicit action by the | |
148 | person it names. This tag documents that potentially interested parties | |
149 | have been included in the discussion | |
150 | ||
151 | ||
152 | 3. Using ``Reported-by:``, ``Tested-by:`` and ``Reviewed-by:`` | |
153 | -------------------------------------------------------------- | |
154 | ||
155 | If this patch fixes a problem reported by somebody else, consider adding a | |
156 | Reported-by: tag to credit the reporter for their contribution. Please | |
157 | note that this tag should not be added without the reporter's permission, | |
158 | especially if the problem was not reported in a public forum. That said, | |
159 | if we diligently credit our bug reporters, they will, hopefully, be | |
160 | inspired to help us again in the future. | |
161 | ||
162 | A ``Tested-by:`` tag indicates that the patch has been successfully tested (in | |
163 | some environment) by the person named. This tag informs maintainers that | |
164 | some testing has been performed, provides a means to locate testers for | |
165 | future patches, and ensures credit for the testers. | |
166 | ||
167 | ``Reviewed-by:``, instead, indicates that the patch has been reviewed and found | |
168 | acceptable according to the Reviewer's Statement: | |
169 | ||
170 | Reviewer's statement of oversight | |
171 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |
172 | ||
173 | By offering my ``Reviewed-by:`` tag, I state that: | |
174 | ||
175 | (a) I have carried out a technical review of this patch to | |
176 | evaluate its appropriateness and readiness for inclusion into | |
177 | the mainline kernel. | |
178 | ||
179 | (b) Any problems, concerns, or questions relating to the patch | |
180 | have been communicated back to the submitter. I am satisfied | |
181 | with the submitter's response to my comments. | |
182 | ||
183 | (c) While there may be things that could be improved with this | |
184 | submission, I believe that it is, at this time, (1) a | |
185 | worthwhile modification to the kernel, and (2) free of known | |
186 | issues which would argue against its inclusion. | |
187 | ||
188 | (d) While I have reviewed the patch and believe it to be sound, I | |
189 | do not (unless explicitly stated elsewhere) make any | |
190 | warranties or guarantees that it will achieve its stated | |
191 | purpose or function properly in any given situation. | |
192 | ||
193 | A ``Reviewed-by`` tag is a statement of opinion that the patch is an | |
194 | appropriate modification of the kernel without any remaining serious | |
195 | technical issues. Any interested reviewer (who has done the work) can | |
196 | offer a ``Reviewed-by`` tag for a patch. This tag serves to give credit to | |
197 | reviewers and to inform maintainers of the degree of review which has been | |
198 | done on the patch. ``Reviewed-by:`` tags, when supplied by reviewers known to | |
199 | understand the subject area and to perform thorough reviews, will normally | |
200 | increase the likelihood of your patch getting into the kernel. | |
201 | ||
202 | ||
203 | PREPARING AND SENDING PATCHES | |
204 | ============================= | |
205 | ||
206 | The upstream repository is managed by Git. You will find that it | |
207 | is easiest to work on the project and submit changes by using the | |
208 | git tools, both for managing your own code and for preparing and | |
209 | sending patches. | |
210 | ||
211 | The project will generally accept code either by pulling code directly from | |
212 | a published git tree (usually on github), or via patches emailed directly | |
213 | to the email list (ceph-devel@vger.kernel.org). For the kernel client, | |
214 | patches are expected to be reviewed in the email list. And for everything | |
215 | else, github is preferred due to the convenience of the 'pull request' | |
216 | feature. | |
217 | ||
218 | ||
219 | 1. Github pull request | |
220 | ---------------------- | |
221 | ||
222 | The preferred way to submit code is by publishing your patches in a branch | |
223 | in your github fork of the ceph repository and then submitting a github | |
224 | pull request. | |
225 | ||
226 | For example, prepare your changes | |
227 | ||
228 | .. code-block:: bash | |
229 | ||
230 | # ...code furiously... | |
231 | $ git commit # git gui is also quite convenient | |
232 | $ git push origin mything | |
233 | ||
234 | Then submit a pull request at | |
235 | ||
236 | https://github.com/ceph/ceph/pulls | |
237 | ||
238 | and click 'New pull request'. See :ref:`_title_of_commit` for our naming | |
239 | convention of pull requests. The 'hub' command-line tool, available from | |
240 | ||
241 | https://github.com/github/hub | |
242 | ||
243 | allows you to submit pull requests directly from the command line | |
244 | ||
245 | .. code-block:: bash | |
246 | ||
247 | $ hub pull-request -b ceph:master -h you:mything | |
248 | ||
249 | Pull requests appear in the review queue at | |
250 | ||
251 | https://github.com/organizations/ceph/dashboard/pulls | |
252 | ||
253 | You may want to ping a developer in #ceph-devel on irc.oftc.net or on the | |
254 | email list to ensure your submission is noticed. | |
255 | ||
256 | When addressing review comments, can should either add additional patches to | |
257 | your branch or (better yet) squash those changes into the relevant commits so | |
258 | that the sequence of changes is "clean" and gets things right the first time. | |
259 | The 'git rebase -i' command is very helpful in this process. Once you have | |
260 | updated your local branch, you can simply force-push to the existing branch | |
261 | in your public repository that is referenced by the pull request with | |
262 | ||
263 | .. code-block:: bash | |
264 | ||
265 | $ git push -f origin mything | |
266 | ||
267 | and your changes will be visible from the existing pull-request. You may want | |
268 | to ping the reviewer again or comment on the pull request to ensure the updates | |
269 | are noticed. | |
270 | ||
271 | Sometimes your change could be based on an outdated parent commit and has | |
272 | conflicts with the latest target branch, then you need to fetch the updates | |
273 | from the remote branch, rebase your change onto it, and resolve the conflicts | |
274 | before doing the force-push | |
275 | ||
276 | .. code-block:: bash | |
277 | ||
278 | $ git pull --rebase origin target-branch | |
279 | ||
280 | So that the pull request does not contain any "merge" commit. Instead of "merging" | |
281 | the target branch, we expect a linear history in a pull request where you | |
282 | commit on top of the remote branch. | |
283 | ||
284 | Q: Which branch should I target in my pull request? | |
285 | ||
286 | A: The target branch depends on the nature of your change: | |
287 | ||
288 | If you are adding a feature, target the "master" branch in your pull | |
289 | request. | |
290 | ||
291 | If you are fixing a bug, target the named branch corresponding to the | |
292 | major version that is currently in development. For example, if | |
293 | Infernalis is the latest stable release and Jewel is development, target | |
294 | the "jewel" branch for bugfixes. The Ceph core developers will | |
295 | periodically merge this named branch into "master". When this happens, | |
296 | the master branch will contain your fix as well. | |
297 | ||
298 | If you are fixing a bug (see above) *and* the bug exists in older stable | |
299 | branches (for example, the "hammer" or "infernalis" branches), then you | |
300 | should file a Redmine ticket describing your issue and fill out the | |
301 | "Backport: <branchname>" form field. This will notify other developers that | |
302 | your commit should be cherry-picked to one or more stable branches. Then, | |
303 | target the "master" branch in your pull request. | |
304 | ||
305 | For example, you should set "Backport: jewel, kraken" in your Redmine ticket | |
306 | to indicate that you are fixing a bug that exists on the "jewel" and | |
307 | "kraken" branches and that you desire that your change be cherry-picked to | |
308 | those branches after it is merged into master. | |
309 | ||
310 | Q: How to include ``Reviewed-by: tag(s)`` in my pull request? | |
311 | ||
312 | A: You don't. If someone reviews your pull request, they should indicate they | |
313 | have done so by commenting on it with "+1", "looks good to me", "LGTM", | |
314 | and/or the entire "Reviewed-by: ..." line with their name and email address. | |
315 | ||
316 | The developer merging the pull request should note positive reviews and | |
317 | include the appropriate Reviewed-by: lines in the merge commit. | |
318 | ||
319 | ||
320 | 2. Patch submission via ceph-devel@vger.kernel.org | |
321 | -------------------------------------------------- | |
322 | ||
323 | The best way to generate a patch for manual submission is to work from | |
324 | a Git checkout of the Ceph source code. You can then generate patches | |
325 | with the 'git format-patch' command. For example, | |
326 | ||
327 | .. code-block:: bash | |
328 | ||
329 | $ git format-patch HEAD^^ -o mything | |
330 | ||
331 | will take the last two commits and generate patches in the mything/ | |
332 | directory. The commit you specify on the command line is the | |
333 | 'upstream' commit that you are diffing against. Note that it does | |
334 | not necesarily have to be an ancestor of your current commit. You | |
335 | can do something like | |
336 | ||
337 | .. code-block:: bash | |
338 | ||
339 | $ git checkout -b mything | |
340 | # ... do lots of stuff ... | |
341 | $ git fetch | |
342 | # ...find out that origin/unstable has also moved forward... | |
343 | $ git format-patch origin/unstable -o mything | |
344 | ||
345 | and the patches will be against origin/unstable. | |
346 | ||
347 | The ``-o`` dir is optional; if left off, the patch(es) will appear in | |
348 | the current directory. This can quickly get messy. | |
349 | ||
350 | You can also add ``--cover-letter`` and get a '0000' patch in the | |
351 | mything/ directory. That can be updated to include any overview | |
352 | stuff for a multipart patch series. If it's a single patch, don't | |
353 | bother. | |
354 | ||
355 | Make sure your patch does not include any extra files which do not | |
356 | belong in a patch submission. Make sure to review your patch -after- | |
357 | generated it with ``diff(1)``, to ensure accuracy. | |
358 | ||
359 | If your changes produce a lot of deltas, you may want to look into | |
360 | splitting them into individual patches which modify things in | |
361 | logical stages. This will facilitate easier reviewing by other | |
362 | kernel developers, very important if you want your patch accepted. | |
363 | There are a number of scripts which can aid in this. | |
364 | ||
365 | The ``git send-email`` command make it super easy to send patches | |
366 | (particularly those prepared with git format patch). It is careful to | |
367 | format the emails correctly so that you don't have to worry about your | |
368 | email client mangling whitespace or otherwise screwing things up. It | |
369 | works like so: | |
370 | ||
371 | .. code-block:: bash | |
372 | ||
373 | $ git send-email --to ceph-devel@vger.kernel.org my.patch | |
374 | ||
375 | for a single patch, or | |
376 | ||
377 | .. code-block:: bash | |
378 | ||
379 | $ git send-email --to ceph-devel@vger.kernel.org mything | |
380 | ||
381 | to send a whole patch series (prepared with, say, git format-patch). | |
382 | ||
383 | ||
384 | 3. Describe your changes | |
385 | ------------------------ | |
386 | ||
387 | Describe the technical detail of the change(s) your patch includes. | |
388 | ||
389 | .. _title_of_commit: | |
390 | ||
391 | Title of pull requests and title of commits | |
392 | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ | |
393 | ||
394 | The text up to the first empty line in a commit message is the commit | |
395 | title. Ideally it is a single short line less than 50 characters, | |
396 | summarizing the change. It is required to prefix it with the | |
397 | subsystem or module you are changing. For instance, the prefix | |
398 | could be "doc:", "osd:", or "common:". One can use:: | |
399 | ||
400 | git log | |
401 | ||
402 | for more examples. Please use this convention for naming pull requests | |
403 | (subsystem: short description) also, as it feeds directly into the script | |
404 | that generates release notes and it's tedious to clean up at release time. | |
405 | ||
406 | Commit message | |
407 | ^^^^^^^^^^^^^^ | |
408 | ||
409 | Be as specific as possible. The WORST descriptions possible include | |
410 | things like "update driver X", "bug fix for driver X", or "this patch | |
411 | includes updates for subsystem X. Please apply." | |
412 | ||
413 | If your description starts to get long, that's a sign that you probably | |
414 | need to split up your patch. See :ref:`split_changes`. | |
415 | ||
416 | When you submit or resubmit a patch or patch series, include the | |
417 | complete patch description and justification for it. Don't just | |
418 | say that this is version N of the patch (series). Don't expect the | |
419 | patch merger to refer back to earlier patch versions or referenced | |
420 | URLs to find the patch description and put that into the patch. | |
421 | I.e., the patch (series) and its description should be self-contained. | |
422 | This benefits both the patch merger(s) and reviewers. Some reviewers | |
423 | probably didn't even receive earlier versions of the patch. | |
424 | ||
425 | Tag the commit | |
426 | ^^^^^^^^^^^^^^ | |
427 | ||
428 | If the patch fixes a logged bug entry, refer to that bug entry by | |
429 | URL. In particular, if this patch fixes one or more issues | |
430 | tracked by http://tracker.ceph.com, consider adding a ``Fixes:`` tag to | |
431 | connect this change to addressed issue(s). So a line saying :: | |
432 | ||
433 | Fixes: http://tracker.ceph.com/issues/12345 | |
434 | ||
435 | is added before the ``Signed-off-by:`` line stating that this commit | |
436 | addresses http://tracker.ceph.com/issues/12345. It helps the reviewer to | |
437 | get more context of this bug, so she/he can hence update the issue on | |
438 | the bug tracker accordingly. | |
439 | ||
440 | So a typical commit message for revising the document could look like:: | |
441 | ||
442 | doc: add "--foo" option to bar | |
443 | ||
444 | * update the man page for bar with the newly added "--foo" option. | |
445 | * fix a typo | |
446 | ||
447 | Fixes: http://tracker.ceph.com/issues/12345 | |
448 | Signed-off-by: Random J Developer <random@developer.example.org> | |
449 | ||
450 | .. _split_changes: | |
451 | ||
452 | 4. Separate your changes | |
453 | ------------------------ | |
454 | ||
455 | Separate *logical changes* into a single patch file. | |
456 | ||
457 | For example, if your changes include both bug fixes and performance | |
458 | enhancements for a single driver, separate those changes into two | |
459 | or more patches. If your changes include an API update, and a new | |
460 | driver which uses that new API, separate those into two patches. | |
461 | ||
462 | On the other hand, if you make a single change to numerous files, | |
463 | group those changes into a single patch. Thus a single logical change | |
464 | is contained within a single patch. | |
465 | ||
466 | If one patch depends on another patch in order for a change to be | |
467 | complete, that is OK. Simply note "this patch depends on patch X" | |
468 | in your patch description. | |
469 | ||
470 | If you cannot condense your patch set into a smaller set of patches, | |
471 | then only post say 15 or so at a time and wait for review and integration. | |
472 | ||
c07f9fc5 FG |
473 | 5. Document your changes |
474 | ------------------------ | |
475 | ||
476 | If you have added or modified any user-facing functionality, such | |
477 | as CLI commands or their output, then the patch series or pull request | |
478 | must include appropriate updates to documentation. | |
479 | ||
480 | It is the submitter's responsibility to make the changes, and the reviewer's | |
481 | responsibility to make sure they are not merging changes that do not | |
482 | have the needed updates to documentation. | |
483 | ||
484 | Where there are areas that have absent documentation, or there is no | |
485 | clear place to note the change that is being made, the reviewer should | |
486 | contact the component lead, who should arrange for the missing section | |
487 | to be created with sufficient detail for the patch submitter to | |
488 | document their changes. | |
7c673cae | 489 | |
c07f9fc5 | 490 | 6. Style check your changes |
7c673cae FG |
491 | --------------------------- |
492 | ||
493 | Check your patch for basic style violations, details of which can be | |
494 | found in CodingStyle. | |
495 | ||
496 | ||
c07f9fc5 | 497 | 7. No MIME, no links, no compression, no attachments. Just plain text |
7c673cae FG |
498 | ---------------------------------------------------------------------- |
499 | ||
500 | Developers need to be able to read and comment on the changes you are | |
501 | submitting. It is important for a kernel developer to be able to | |
502 | "quote" your changes, using standard e-mail tools, so that they may | |
503 | comment on specific portions of your code. | |
504 | ||
505 | For this reason, all patches should be submitting e-mail "inline". | |
506 | WARNING: Be wary of your editor's word-wrap corrupting your patch, | |
507 | if you choose to cut-n-paste your patch. | |
508 | ||
509 | Do not attach the patch as a MIME attachment, compressed or not. | |
510 | Many popular e-mail applications will not always transmit a MIME | |
511 | attachment as plain text, making it impossible to comment on your | |
512 | code. A MIME attachment also takes Linus a bit more time to process, | |
513 | decreasing the likelihood of your MIME-attached change being accepted. | |
514 | ||
515 | Exception: If your mailer is mangling patches then someone may ask | |
516 | you to re-send them using MIME. | |
517 |