Contribution Checklist
Contents
- One Patch Per Independent Change
- Contribution Email Subject Line
- Detailed Explanation of the Patch
- Documentation
- Testing
- FSF copyright Assignment
- Copyright Header
- Attribution
- Properly Formatted Changes
- Submitting patches
- Ping and keep pinging
- Receiving positive reviews
- Properly formatted commit messages
- Fixing commit dates
- Bugzilla
A high-quality contribution includes the following things:
1. One Patch Per Independent Change
It is very important that independent changes be split into separate patches, as it has many advantages:
- It helps the review process, since each patch is therefore smaller, increasing the chances of being able to accept each patch faster;
- When investigating which patch introduced a change of behavior, once the patch is found, it is easier to determine why it changes the behavior if the patch is smaller;
- Very seldomly, a patch causes a regression severe enough that, when unable to fix the regression immediately, a revert is needed. When that happens, smaller patches means we end up reverting only the part that causes the problem.
Typical examples where splitting patches up will be requested:
- Patches that make unrelated formatting changes while also fixing a problem;
- Patches that combine fixes for multiple problems;
- Patches that combine multiple enhancements;
Also, for fixes/enhancements which you implemented in multiple logical steps, we encourage you to also submit your changes in multiple commits, each commit corresponding to each logical step. For instance, if implementing a given enhancement made you...
- move a routine from one .c file to another;
- then modify that routine and a few callers;
... submitting your changes as two patches, the first one just doing the move, and the second one making the changes after the move, will greatly simplify the review of both patches, as the second patch will clearly show what changes you made.
2. Contribution Email Subject Line
A high-quality email subject line for a contribution contains the following tags:
For a Single patch contribution:
[PATCH] <Subject>
For Multi-part patch set contributions where [PATCH 0/2] is used to described the forth coming patches:
[PATCH 0/2] <Optional unique prefix e.g. "String cleanup:"><Subject related to the patch set e.g. Cleanup all broken string routines.>
[PATCH 1/2] <Optional unique prefix e.g. "String cleanup:"><Distinct subject line for this patch e.g. Cleanup strstr.>
[PATCH 2/2] <Optional unique prefix e.g. "String cleanup:"><Another distinct subject line for this patch e.g. Cleanup strncmp.>
For a request for comments that doesn't have to be a patch at all:
[RFC] <Subject>
For a request for comments on a patch or patch set:
[RFC][PATCH] <Subject>
For a patch that fixes a bug filed in bugzilla:
[PATCH][PR <component/number>] <Subject>
For a patch that is the Nth version of an earlier patch:
[PATCH vN] <Subject>
For a patch that is already committed:
[COMMITTED PATCH] <Subject>
For a patch that falls under the The Obvious Rule as described in gdb/MAINTAINERS:
[OB PATCH] <Subject>
Most reviewers get a lot of email. To help them, please make <Subject> as descriptive as possible, with the most important words early on:
Fix PR 14963 is a bad subject: everybody on the list has to open this email to see what it relates to.
Fix segfault is a little better: we can see this patch fixes an important issue, but in which component?
Fix segfault caused by infinite loop in demangler is better, but the component is at the end and may be truncated in people's email clients.
Fix segfault demangling C++11 rvalue references is pretty great.
3. Detailed Explanation of the Patch
3.1. General requirements
- Describe the rationale for the patch. E.g., don't assume the change will be obvious to others just because it's a small patch. It may be that it only looks obvious to you, because you've spent the effort investigating the issue, and you have all the context in your head. Your job is to make it easy for the reviewer to give you an OK.
- If you experimented other seemingly valid or seemingly better approaches for your fix/feature, and they didn't work out, mention it in the patch submission, along with the reason for why it didn't work out.
- Explain the steps required to reproduce the problem.
- Extra points are awarded for a test-case which demonstrates the problem and can be used in the test-suite to demonstrate success of a fix.
- Explain the observed versus expected behaviour. If you're changing the output of some command, include a paste of the relevant parts of gdb session, before and after the change.
- Explain how you tested the patch. If you ran the testsuite, say so, and say on which targets. If you didn't run the testsuite, you should have. If you still haven't run the testsuite, say so, and explain why.
4. Documentation
If your patch adds a new command then that command should be documented in the GDB manual, and mentioned in the NEWS file. All commands must be documented and mentioned, including maintenance commands (e.g., "set debug" or "maint" commands).
New target ports must be mentioned in the NEWS file.
New host configurations must be mentioned in the NEWS file.
New remote protocol packets must be documented in the GDB manual, and mentioned in the NEWS file.
Standard target features for xml target descriptions (e.g., org.gnu.gdb.arm.core, org.gnu.gdb.i386.sse) must be documented in the GDB manual.
New MI commands, attributes, etc., must be documented in the GDB manual, and mentioned in the NEWS file.
New Python scripting features must be documented in the GDB manual, and mentioned in the NEWS file.
5. Testing
Make sure there are no regressions in the test suite.
- The easiest way to verify this is to run the testsuite before and after your patch, and comparing the testsuite reports obtained before and after.
- Explain for which targets the test suite was run.
- Adding a test-case to the test suite may increase the likelihood of acceptance of your patch. In many cases it will be necessary.
- Don't add new tests that are known to FAIL. Mark them as KFAIL or XFAIL.
6. FSF copyright Assignment
The Free Software Foundation (holder of the GDB copyrights) requires copyright assignment for all legally significant changes by a particular author (read not a company). This webpage describes the copyright assignment requirements.
- Explain your current FSF copyright assignment status.
In the event that you have not previously completed an FSF copyright assignment, a GDB maintainer will direct you to a FSF copyright assignment request form that's best suited to your contribution under the guidelines on this gnu.org webpage.
Note: the following list is quick reference links for the moderator or maintainer. They will send you the correct form.
As from August, 2013, the FSF began accepting GPG signed assignments from U.S. residents. More information at this link.
7. Copyright Header
All source files must have a copyright header. Although the FSF only requires that for legally significant files (IOW, files with only a couple lines wouldn't require a header), GDB maintainers prefer not having to worry about later having to add the header when the file grows.
In addition, we track which files don't have copyright headers, and it improves the S/N ratio of the output of updating copyright years for each new year if every file has a copyright header. Thus a copyright header is required even for simple 3 line testcases. This GNU Copyright-Notices webpage indicates the proper format for the copyright notice and how to write the dates:
- A copyright notice takes the following form:
Copyright (C) year copyright-holder
For GDB, spanning of consecutive years IS allowed. Therefore, when adding a second (or more) consecutive year to the copyright statement, you may write a year range, e.g.,
Copyright (C) year1-year2 copyright-holder
The simplest is to just copy the header from some other file.
If you've based your new file on some other file, as e.g., it is common to do when writing new tests, then the copyright years of the original file must be retained in the new file.
8. Attribution
"Contributed by" statements are no longer required or desired in GDB source files. Attribution is now recorded in git commit logs as the commit "Author". Please do not use "Contributed by" statements in your submissions.
Please do feel free to propose additions to the Contributors node in the manual, however.
9. Properly Formatted Changes
Please see Coding Standards for a full description of coding guidelines.
If you have any questions regarding these criteria please email gdb-patches@sourceware.org .
10. Submitting patches
Please send patches to gdb-patches@sourceware.org; one email per patch as described in the sections above. You can take a look at the list's archives to get a feel for what others do. If you're going to be contributing routinely, it's recommended to subscribe to the list.
Patches must be sent using git send-email command. This makes sure your patch doesn't get wrongly reformatted by your email client. There are numerous good references on how to configure and use git send-email, one of which is here.
If using git send-email is not possible for you, you can still try to send it using your email client, pasting the output of git format-patch in the email body. However, you need to be very careful that the client doesn't modify the patch in a way that makes it impossible to apply:
- Make sure your mailer doesn't turn TABS into SPACES, i.e. turn on preformat.
Don't send patches with format=flowed. This messes up line breaks and makes clients display tabs wrong.
Thunderbird users, see here.
See this excellent guide, that touches on the issues above, and more, including instructions for several mailers (Thunderbird, Alpine, KMail, Gmail, etc.).
Finally, as a last resort, you can send the output of git format-patch as an attachment. This is discouraged, however, because it makes it difficult for others to reply inline when reviewing. Please try to use git send-email as much as possible and everybody will be happy.
11. Ping and keep pinging
We try to do our best, but as a volunteer-based project, unfortunately lots of patches wait a bit before review. If your patch is not reviewed it thus may just mean that the reviewers are busy.
The generally accepted ping frequency is 2 weeks after first submission, and every week thereafter.
When sending a ping, reply to the original email and add "[PING]" to the reply's subject. In other words, don't start a new email thread for the ping. See rationale here.
12. Receiving positive reviews
When your patch is reviewed, you may receive one of these tags:
Acked-By: This means that the reviewer has read your change descriptions and agrees with the direction of the patch, but has not reviewed or tested the code.
Tested-By: Name <email>: This means the reviewer has tested your patch and found no regressions on their system.
Reviewed-By: Name <email>: This means that the reviewer has analysed your patch and thinks it is correct, but cannot (or does not want to) approve it.
Approved-By: Name <email>: This means that the reviewer approves your patch. Some reviewers may only approve parts of your patch, so before pushing it, make sure that all of your patch has been approved.
When you receive such tags, add them to the end of your commit message verbatim before pushing.
Check out the original proposal if you would like more information on those tags or their usage.
13. Properly formatted commit messages
Please ensure the commit messages are formatted as follows (following common git conventions):
<single line summarizing the change> <blank-line> <paragraphs describing of the change> <blank-line> <git trailers>
The first line (which will also be the email's subject when sending the patch with git-send-email), should be as concise as possible while being precise enough to know what the patch is about. It's the one line sales pitch of your patch. There is no strict maximum length, but over 80 characters is getting a bit long.
Git trailers are metadata lines often found at the end of git commit messages, of the form Key: Value. The ones we use are:
Co-Authored-By: Name <email>: when the patch has more than one author, one of them is listed as the patch author, and there are Co-Authored-By lines for the others.
Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=XYZ: Link to the relevant bug on GDB's Bugzilla.
Approved-By: Name <email>: The person (or people) who approved the patch for pushing.
Reviewed-By: Name <email>: People who helped reviewing the patch.
Tested-By: Name <email>: People who helped test the patch.
Acked-By: Name <email>: People who read and agree with the changes, but haven't done code-specific checking.
For example (the bug number is made up):
Make all source files include defs.h or server.h first This commit makes all source files under gdb/ that include headers from gdb/ include either defs.h or server.h before any other code. This ensures that definitions and macros from the two config.h files are always in place for our code. An exception has been made for gdb/gdbserver/gdbreplay.c which seems to be a special case. Co-Authored-By: Janine Sutto <janine.sutto@gmail.com> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=12345
14. Fixing commit dates
If you have used git rebase to create your commits it is likely the commit dates are wrong. Before pushing please run this command:
git rebase --ignore-date origin/master
To set the timestamps of all the commits you are about to push to the current time.
15. Bugzilla
The bug tracker is Bugzilla on sourceware.org. New users do not have the editbugs group access which means that while they can create bugs they cannot edit any aspects of the bug once created. This is done to avoid spammers modifying existing bugs. If you need the ability to edit bugs please request it on gdb@sourceware.org or on IRC freenode at #gdb, the bugzilla admins and senior members of the community can grant this access once they know you're not a spammer.