how_to_submit_a_patch.rst (9645B)
1 How to submit a patch 2 ===================== 3 4 Submitting a patch, getting it reviewed, and committed to the Firefox 5 source tree involves several steps. This article explains how. 6 7 .. note:: 8 9 We are also providing a :ref:`Firefox Contributors Quick Reference <Firefox Contributors' Quick Reference>` for contributors. 10 11 The process of submission is illustrated by the following diagram, and 12 each step is detailed below: 13 14 .. mermaid:: 15 16 graph TD; 17 Preparation --> c[Working on a patch]; 18 c[Working on a patch] --> Testing; 19 Testing --> c[Working on a patch]; 20 Testing --> e[Submit the patch]; 21 e[Submit the patch] --> d[Getting Reviews] 22 d[Getting Reviews] -- Addressing Review comment --> c[Working on a patch]; 23 d[Getting Reviews] --> h[Push the change]; 24 25 26 27 28 Preparation 29 ----------- 30 31 Every change to the code is tracked by a bug report 32 in `bugzilla.mozilla.org <https://bugzilla.mozilla.org/>`__. Without a 33 bug, code will not be reviewed, and without review, code will not be 34 accepted. To avoid duplication, `search for an existing 35 bug <https://bugzilla.mozilla.org/query.cgi?format=specific>`__ about 36 your change, and only if none exists, file a new one. Most communication 37 about code changes take place in the associated code 38 review, so be sure the bug describes the exact problem being solved. 39 40 Please verify the bug is for the correct product and component. For more 41 information, ask questions on the newsgroups, or on the #developers room 42 on `chat.mozilla.org <https://chat.mozilla.org>`__. 43 44 The person working on a bug should be the 'assignee' of that bug in 45 Bugzilla. If somebody else is currently the assignee of a bug, email 46 this person to coordinate changes. If the bug is unassigned, leave a 47 message in the bug's comments, stating that you intend working on it, 48 and suggest that someone with bug-editing privileges assign it to you. 49 50 Some teams wait for new contributors to attach their first patch before 51 assigning a bug. This makes it available for other contributors, in case 52 the new contributor is unable to level up to patch creation. By 53 expressing interest in a bug comment, someone from that team should 54 guide you through their process. 55 56 57 Module ownership 58 ---------------- 59 60 All code is supervised by a `module 61 owner <https://www.mozilla.org/en-US/about/governance/policies/module-ownership/>`__. 62 This person will be responsible for reviewing and accepting the change. 63 Before writing your code, determine the module owner, verifying your 64 proposed change is considered acceptable. They may want to look over any 65 new user interface (UI review), functions (API review), or testcases for 66 the proposed change. 67 68 If module ownership is not clear, ask on the newsgroups or `on 69 Matrix <https://chat.mozilla.org>`__. The revision log for the relevant 70 file might also be helpful. For example, see the change log for 71 ``browser/base/content/browser.js``, by clicking the "Git Log" 72 link at the top of `Searchfox <https://searchfox.org/mozilla-central/source/>`__, or 73 by running ``git log browser/base/content/browser.js``. The corresponding 74 checkin message will contain something like "r=nickname", identifying 75 active code submissions, and potential code reviewers. 76 77 78 Working on a patch 79 ------------------ 80 81 Changes to the Firefox source code are presented in the form of a patch. 82 A patch is a commit to version control. Firefox and related code is 83 stored in our `git repository <https://github.com/mozilla-firefox/firefox>`__. 84 85 Each patch should represent a single complete change, separating 86 distinct changes into multiple individual patches. If your change 87 results in a large, complex patch, seek if it can be broken into 88 `smaller, easy to understand patches representing complete 89 steps <https://secure.phabricator.com/book/phabflavor/article/writing_reviewable_code/#many-small-commits>`__, 90 applied on top of each other. This makes it easier to review your 91 changes, `leading to quicker 92 reviews, <https://groups.google.com/group/mozilla.dev.planning/msg/2f99460f57f776ef?hl=en>`__ 93 and improved confidence in this review outcome. 94 95 Also ensure that your commit message is formatted appropriately. A 96 simple commit message should look like this: 97 98 :: 99 100 Bug 123456 - Change this thing to work better by doing something. r=reviewers 101 102 The text of the message should be what you did to fix the bug, not a 103 description of what the bug was. If it is not obvious why this change is 104 appropriate, then `explain why in the commit 105 message <https://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview/commits.html#write-detailed-commit-messages>`__. 106 If this does not fit on one line, then leave a blank line and add 107 further lines for more detail and/or reasoning. 108 109 The ``r=reviewers`` part specifies that ``reviewers`` should review the patch 110 and provide feedback before it is integrated into the Firefox codebase. For 111 choosing reviewers, and the full reviewer syntax, please see 112 :ref:`Getting reviews`. 113 114 You can edit the message of the current commit at any time using 115 ``git commit --amend`` or ``git rebase -i``. 116 117 Also look at our :ref:`Reviewer Checklist` for a list 118 of best practices for patch content that reviewers will check for or 119 require. 120 121 122 Testing 123 ------- 124 125 All changes must be tested. In most cases, :ref:`Automated Testing` is required for every 126 change to the code. 127 128 While we desire to have automated tests for all code, we also have a 129 linter tool which runs static analysis on our JavaScript, for best 130 practices and common mistakes. See :ref:`ESLint` for more information. 131 132 Ensure that your change has not caused regressions, by running the 133 automated test suite locally, or using the `Mozilla try 134 server <https://wiki.mozilla.org/Build:TryServer>`__. Module owners, or 135 developers `on Matrix <https://chat.mozilla.org>`__ may be willing to 136 submit jobs for those currently without try server privileges. 137 138 139 Submit the patch 140 ---------------- 141 142 .. note:: 143 144 Make sure you rebase your patch on top of the latest build before you 145 submit to prevent any merge conflicts. 146 147 Mozilla uses Phabricator for code review. See the `Mozilla Phabricator 148 User 149 Guide <https://moz-conduit.readthedocs.io/en/latest/phabricator-user.html>`__ 150 for instructions. 151 152 Don't be shy in posting partial patches, demonstrating potential 153 approaches, and asking for preliminary feedback. It is easier for others 154 to comment, and offer suggestions, when a question is accompanied by 155 some code. 156 157 158 Getting reviews for my patch 159 ---------------------------- 160 161 See the dedicated page :ref:`Getting reviews` 162 163 164 Addressing review comments 165 -------------------------- 166 167 It is unusual for patches to be perfect the first time around. The 168 reviewer may use the ‘Request Changes’ 169 `action <http://moz-conduit.readthedocs.io/en/latest/phabricator-user.html#reviewing-patches>`__ 170 and list problems that must be addressed before the patch can be 171 accepted. Please remember that requesting revisions is not meant to 172 discourage participation, but rather to encourage the best possible 173 resolution of a bug. Carefully work through the changes that the 174 reviewer recommends, attach a new patch, and request review again. 175 176 Sometimes a reviewer will grant conditional review with the ‘Accept 177 Revision’ action but will also indicate minor necessary changes, such as 178 spelling, or indentation fixes. All recommended corrections should be 179 made, but a re-review is unnecessary. Make the changes and submit a new 180 patch. If there is any confusion about the revisions, another review 181 should be requested. 182 183 Sometimes, after a patch is reviewed, but before it can be committed, 184 someone else makes a conflicting change. If the merge is simple, and 185 non-invasive, post an updated version of the patch. For all non-trivial 186 changes, another review is necessary. 187 188 If at any point the review process stalls for more than two weeks, see 189 the previous 'Getting attention' section. 190 191 In many open source projects, developers will accept patches in an 192 unfinished state, finish them, and apply the completed code. In 193 Mozilla's culture, **the reviewer will only review and comment on a 194 patch**. If a submitter declines to make the revisions, the patch will 195 sit idle, until someone chooses to take it on. 196 197 198 Pushing the change 199 ------------------ 200 201 A patch can be pushed (aka. 'landed') after it has been properly 202 reviewed. 203 204 .. note:: 205 206 Be sure to build the application with the patch applied. This 207 ensures it runs as expected, passing automated tests, and/or runs 208 through the `try 209 server <https://wiki.mozilla.org/Build:TryServerAsBranch>`__. In the 210 bug, please also mention you have completed this step. 211 212 Submitting untested patches wastes the committer's time, and may burn 213 the release tree. Please save everyone's time and effort by 214 completing all necessary verifications. 215 216 217 Ask the reviewer to land the patch for you. 218 For more details, see :ref:`push_a_change` 219 220 `Lando <https://moz-conduit.readthedocs.io/en/latest/lando-user.html>`__ is used 221 to automatically land your code. 222 223 224 Regressions 225 ----------- 226 227 It is possible your code causes functional or performance regressions. 228 There is a tight 229 `policy <https://www.mozilla.org/about/governance/policies/regressions/>`__ on 230 performance regressions, in particular. This means your code may be 231 dropped, leaving you to fix and resubmit it. Regressions, ultimately 232 mean the tests you ran before checking in are not comprehensive enough. 233 A resubmitted patch, or a patch to fix the regression, should be 234 accompanied by appropriate tests. 235 236 After authoring a few patches, consider `getting commit access to 237 Mozilla source code <https://www.mozilla.org/about/governance/policies/commit/>`__.