| 1 | Submitting Patches |
| 2 | ================== |
| 3 | |
| 4 | == Guidelines |
| 5 | |
| 6 | Here are some guidelines for contributing back to this |
| 7 | project. There is also a link:MyFirstContribution.html[step-by-step tutorial] |
| 8 | available which covers many of these same guidelines. |
| 9 | |
| 10 | [[patch-flow]] |
| 11 | === A typical life cycle of a patch series |
| 12 | |
| 13 | To help us understand the reason behind various guidelines given later |
| 14 | in the document, first let's understand how the life cycle of a |
| 15 | typical patch series for this project goes. |
| 16 | |
| 17 | . You come up with an itch. You code it up. You do not need any |
| 18 | pre-authorization from the project to do so. |
| 19 | + |
| 20 | Your patches will be reviewed by other contributors on the mailing |
| 21 | list, and the reviews will be done to assess the merit of various |
| 22 | things, like the general idea behind your patch (including "is it |
| 23 | solving a problem worth solving in the first place?"), the reason |
| 24 | behind the design of the solution, and the actual implementation. |
| 25 | The guidelines given here are there to help your patches by making |
| 26 | them easier to understand by the reviewers. |
| 27 | |
| 28 | . You send the patches to the list and cc people who may need to know |
| 29 | about the change. Your goal is *not* necessarily to convince others |
| 30 | that what you are building is good. Your goal is to get help in |
| 31 | coming up with a solution for the "itch" that is better than what |
| 32 | you can build alone. |
| 33 | + |
| 34 | The people who may need to know are the ones who worked on the code |
| 35 | you are touching. These people happen to be the ones who are |
| 36 | most likely to be knowledgeable enough to help you, but |
| 37 | they have no obligation to help you (i.e. you ask them for help, |
| 38 | you don't demand). +git log -p {litdd} _$area_you_are_modifying_+ would |
| 39 | help you find out who they are. |
| 40 | + |
| 41 | It is also a good idea to check whether your topic has been discussed |
| 42 | previously on the mailing list, or whether similar work is already in |
| 43 | progress. Prior discussions may contain useful context, design |
| 44 | considerations, or earlier attempts at solving the same problem. Being |
| 45 | aware of such discussions can help you avoid duplicating work and may |
| 46 | allow you to coordinate with other contributors working in the same |
| 47 | area. |
| 48 | |
| 49 | . You get comments and suggestions for improvements. You may even get |
| 50 | them in an "on top of your change" patch form. You are expected to |
| 51 | respond to them with "Reply-All" on the mailing list, while taking |
| 52 | them into account while preparing an updated set of patches. |
| 53 | + |
| 54 | It is often beneficial to allow some time for reviewers to provide |
| 55 | feedback before sending a new version, rather than sending an updated |
| 56 | series immediately after receiving a review. This helps collect broader |
| 57 | input and avoids unnecessary churn from many rapid iterations. |
| 58 | |
| 59 | . These early update iterations are expected to be full replacements, |
| 60 | not incremental updates on top of what you posted already. If you |
| 61 | are correcting mistakes you made in the previous iteration that a |
| 62 | reviewer noticed and pointed out in their review, you _fix_ that |
| 63 | mistake by rewriting your history (e.g., by using "git rebase -i") |
| 64 | to pretend that you never made the mistake in the first place. In |
| 65 | other words, this is a chance to pretend to be a perfect developer, |
| 66 | and you are expected to take advantage of that. In the larger |
| 67 | picture, nobody is interested in your earlier mistakes. Just |
| 68 | present a logical progression made by a perfect developer who makes |
| 69 | no mistakes while working on the topic. |
| 70 | |
| 71 | . Polish, refine, and re-send your patches to the list and to the people |
| 72 | who spent their time to improve your patch. Go back to step (2). |
| 73 | |
| 74 | . While the above iterations improve your patches, the maintainer may |
| 75 | pick the patches up from the list and queue them to the `seen` |
| 76 | branch, in order to make it easier for people to play with it |
| 77 | without having to pick up and apply the patches to their trees |
| 78 | themselves. Being in `seen` has no other meaning. Specifically, it |
| 79 | does not mean the patch was "accepted" in any way. |
| 80 | |
| 81 | . When the discussion reaches a consensus that the latest iteration of |
| 82 | the patches are in good enough shape, the maintainer includes the |
| 83 | topic in the "What's cooking" report that are sent out a few times a |
| 84 | week to the mailing list, marked as "Will merge to 'next'." This |
| 85 | decision is primarily made by the maintainer with help from those |
| 86 | who participated in the review discussion. |
| 87 | |
| 88 | . After the patches are merged to the 'next' branch, the discussion |
| 89 | can still continue to further improve them by adding more patches on |
| 90 | top, but by the time a topic gets merged to 'next', it is expected |
| 91 | that everybody agrees that the scope and the basic direction of the |
| 92 | topic are appropriate, so such an incremental updates are limited to |
| 93 | small corrections and polishing. After a topic cooks for some time |
| 94 | (like 7 calendar days) in 'next' without needing further tweaks on |
| 95 | top, it gets merged to the 'master' branch and waits to become part |
| 96 | of the next major release. |
| 97 | |
| 98 | In the following sections, many techniques and conventions are listed |
| 99 | to help your patches get reviewed effectively in such a life cycle. |
| 100 | |
| 101 | |
| 102 | [[choose-starting-point]] |
| 103 | === Choose a starting point. |
| 104 | |
| 105 | As a preliminary step, you must first choose a starting point for your |
| 106 | work. Typically this means choosing a branch, although technically |
| 107 | speaking it is actually a particular commit (typically the HEAD, or tip, |
| 108 | of the branch). |
| 109 | |
| 110 | There are several important branches to be aware of. Namely, there are |
| 111 | four integration branches as discussed in linkgit:gitworkflows[7]: |
| 112 | |
| 113 | * maint |
| 114 | * master |
| 115 | * next |
| 116 | * seen |
| 117 | |
| 118 | The branches lower on the list are typically descendants of the ones |
| 119 | that come before it. For example, `maint` is an "older" branch than |
| 120 | `master` because `master` usually has patches (commits) on top of |
| 121 | `maint`. |
| 122 | |
| 123 | There are also "topic" branches, which contain work from other |
| 124 | contributors. Topic branches are created by the Git maintainer (in |
| 125 | their fork) to organize the current set of incoming contributions on |
| 126 | the mailing list, and are itemized in the regular "What's cooking in |
| 127 | git.git" announcements. To find the tip of a topic branch, run `git log |
| 128 | --first-parent master..seen` and look for the merge commit. The second |
| 129 | parent of this commit is the tip of the topic branch. |
| 130 | |
| 131 | There is one guiding principle for choosing the right starting point: in |
| 132 | general, always base your work on the oldest integration branch that |
| 133 | your change is relevant to (see "Merge upwards" in |
| 134 | linkgit:gitworkflows[7]). What this principle means is that for the |
| 135 | vast majority of cases, the starting point for new work should be the |
| 136 | latest HEAD commit of `maint` or `master` based on the following cases: |
| 137 | |
| 138 | * If you are fixing bugs in the released version, use `maint` as the |
| 139 | starting point (which may mean you have to fix things without using |
| 140 | new API features on the cutting edge that recently appeared in |
| 141 | `master` but were not available in the released version). |
| 142 | |
| 143 | * Otherwise (such as if you are adding new features) use `master`. |
| 144 | |
| 145 | |
| 146 | NOTE: In exceptional cases, a bug that was introduced in an old |
| 147 | version may have to be fixed for users of releases that are much older |
| 148 | than the recent releases. `git describe --contains X` may describe |
| 149 | `X` as `v2.30.0-rc2-gXXXXXX` for the commit `X` that introduced the |
| 150 | bug, and the bug may be so high-impact that we may need to issue a new |
| 151 | maintenance release for Git 2.30.x series, when "Git 2.41.0" is the |
| 152 | current release. In such a case, you may want to use the tip of the |
| 153 | maintenance branch for the 2.30.x series, which may be available in the |
| 154 | `maint-2.30` branch in https://github.com/gitster/git[the maintainer's |
| 155 | "broken out" repo]. |
| 156 | |
| 157 | This also means that `next` or `seen` are inappropriate starting points |
| 158 | for your work, if you want your work to have a realistic chance of |
| 159 | graduating to `master`. They are simply not designed to be used as a |
| 160 | base for new work; they are only there to make sure that topics in |
| 161 | flight work well together. This is why both `next` and `seen` are |
| 162 | frequently re-integrated with incoming patches on the mailing list and |
| 163 | force-pushed to replace previous versions of themselves. A topic that is |
| 164 | literally built on top of `next` cannot be merged to `master` without |
| 165 | dragging in all the other topics in `next`, some of which may not be |
| 166 | ready. |
| 167 | |
| 168 | For example, if you are making tree-wide changes, while somebody else is |
| 169 | also making their own tree-wide changes, your work may have severe |
| 170 | overlap with the other person's work. This situation may tempt you to |
| 171 | use `next` as your starting point (because it would have the other |
| 172 | person's work included in it), but doing so would mean you'll not only |
| 173 | depend on the other person's work, but all the other random things from |
| 174 | other contributors that are already integrated into `next`. And as soon |
| 175 | as `next` is updated with a new version, all of your work will need to |
| 176 | be rebased anyway in order for them to be cleanly applied by the |
| 177 | maintainer. |
| 178 | |
| 179 | Under truly exceptional circumstances where you absolutely must depend |
| 180 | on a select few topic branches that are already in `next` but not in |
| 181 | `master`, you may want to create your own custom base-branch by forking |
| 182 | `master` and merging the required topic branches into it. You could then |
| 183 | work on top of this base-branch. But keep in mind that this base-branch |
| 184 | would only be known privately to you. So when you are ready to send |
| 185 | your patches to the list, be sure to communicate how you created it in |
| 186 | your cover letter. This critical piece of information would allow |
| 187 | others to recreate your base-branch on their end in order for them to |
| 188 | try out your work. |
| 189 | |
| 190 | Finally, note that some parts of the system have dedicated maintainers |
| 191 | with their own separate source code repositories (see the section |
| 192 | "Subsystems" below). |
| 193 | |
| 194 | [[separate-commits]] |
| 195 | === Make separate commits for logically separate changes. |
| 196 | |
| 197 | Unless your patch is really trivial, you should not be sending |
| 198 | out a patch that was generated between your working tree and |
| 199 | your commit head. Instead, always make a commit with complete |
| 200 | commit message and generate a series of patches from your |
| 201 | repository. It is a good discipline. |
| 202 | |
| 203 | Give an explanation for the change(s) that is detailed enough so |
| 204 | that people can judge if it is good thing to do, without reading |
| 205 | the actual patch text to determine how well the code does what |
| 206 | the explanation promises to do. |
| 207 | |
| 208 | If your description starts to get too long, that's a sign that you |
| 209 | probably need to split up your commit to finer grained pieces. |
| 210 | That being said, patches which plainly describe the things that |
| 211 | help reviewers check the patch, and future maintainers understand |
| 212 | the code, are the most beautiful patches. Descriptions that summarize |
| 213 | the point in the subject well, and describe the motivation for the |
| 214 | change, the approach taken by the change, and if relevant how this |
| 215 | differs substantially from the prior version, are all good things |
| 216 | to have. |
| 217 | |
| 218 | Make sure that you have tests for the bug you are fixing. See |
| 219 | `t/README` for guidance. |
| 220 | |
| 221 | [[tests]] |
| 222 | When adding a new feature, make sure that you have new tests to show |
| 223 | the feature triggers the new behavior when it should, and to show the |
| 224 | feature does not trigger when it shouldn't. After any code change, |
| 225 | make sure that the entire test suite passes. When fixing a bug, make |
| 226 | sure you have new tests that break if somebody else breaks what you |
| 227 | fixed by accident to avoid regression. Also, try merging your work to |
| 228 | 'next' and 'seen' and make sure the tests still pass; topics by others |
| 229 | that are still in flight may have unexpected interactions with what |
| 230 | you are trying to do in your topic. |
| 231 | |
| 232 | Pushing to a fork of https://github.com/git/git will use their CI |
| 233 | integration to test your changes on Linux, Mac and Windows. See the |
| 234 | <<GHCI,GitHub CI>> section for details. |
| 235 | |
| 236 | Do not forget to update the documentation to describe the updated |
| 237 | behavior and make sure that the resulting documentation set formats |
| 238 | well (try the Documentation/doc-diff script). |
| 239 | |
| 240 | [[typofixes]] |
| 241 | We currently have a liberal mixture of US and UK English norms for |
| 242 | spelling and grammar, which is somewhat unfortunate. A huge patch that |
| 243 | touches the files all over the place only to correct the inconsistency |
| 244 | is not welcome, though. Potential clashes with other changes that can |
| 245 | result from such a patch are not worth it. We prefer to gradually |
| 246 | reconcile the inconsistencies in favor of US English, with small and |
| 247 | easily digestible patches, as a side effect of doing some other real |
| 248 | work in the vicinity (e.g. rewriting a paragraph for clarity, while |
| 249 | turning en_UK spelling to en_US). Obvious typographical fixes are much |
| 250 | more welcomed ("teh -> "the"), preferably submitted as independent |
| 251 | patches separate from other documentation changes. |
| 252 | |
| 253 | [[whitespace-check]] |
| 254 | Oh, another thing. We are picky about whitespaces. Make sure your |
| 255 | changes do not trigger errors with the sample pre-commit hook shipped |
| 256 | in `templates/hooks--pre-commit`. To help ensure this does not happen, |
| 257 | run `git diff --check` on your changes before you commit. |
| 258 | |
| 259 | [[describe-changes]] |
| 260 | === Describe your changes well. |
| 261 | |
| 262 | The log message that explains your changes is just as important as the |
| 263 | changes themselves. Your code may be clearly written with in-code |
| 264 | comment to sufficiently explain how it works with the surrounding |
| 265 | code, but those who need to fix or enhance your code in the future |
| 266 | will need to know _why_ your code does what it does, for a few |
| 267 | reasons: |
| 268 | |
| 269 | . Your code may be doing something differently from what you wanted it |
| 270 | to do. Writing down what you actually wanted to achieve will help |
| 271 | them fix your code and make it do what it should have been doing |
| 272 | (also, you often discover your own bugs yourself, while writing the |
| 273 | log message to summarize the thought behind it). |
| 274 | |
| 275 | . Your code may be doing things that were only necessary for your |
| 276 | immediate needs (e.g. "do X to directories" without implementing or |
| 277 | even designing what is to be done on files). Writing down why you |
| 278 | excluded what the code does not do will help guide future developers. |
| 279 | Writing down "we do X to directories, because directories have |
| 280 | characteristic Y" would help them infer "oh, files also have the same |
| 281 | characteristic Y, so perhaps doing X to them would also make sense?". |
| 282 | Saying "we don't do the same X to files, because ..." will help them |
| 283 | decide if the reasoning is sound (in which case they do not waste |
| 284 | time extending your code to cover files), or reason differently (in |
| 285 | which case, they can explain why they extend your code to cover |
| 286 | files, too). |
| 287 | |
| 288 | The goal of your log message is to convey the _why_ behind your change |
| 289 | to help future developers. The reviewers will also make sure that |
| 290 | your proposed log message will serve this purpose well. |
| 291 | |
| 292 | The first line of the commit message should be a short description (50 |
| 293 | characters is the soft limit, see DISCUSSION in linkgit:git-commit[1]), |
| 294 | and should skip the full stop. It is also conventional in most cases to |
| 295 | prefix the first line with "area: " where the area is a filename or |
| 296 | identifier for the general area of the code being modified, e.g. |
| 297 | |
| 298 | * doc: clarify distinction between sign-off and pgp-signing |
| 299 | * githooks.txt: improve the intro section |
| 300 | |
| 301 | If in doubt which identifier to use, run `git log --no-merges` on the |
| 302 | files you are modifying to see the current conventions. |
| 303 | |
| 304 | [[summary-section]] |
| 305 | The title sentence after the "area:" prefix omits the full stop at the |
| 306 | end, and its first word is not capitalized (the omission |
| 307 | of capitalization applies only to the word after the "area:" |
| 308 | prefix of the title) unless there is a reason to |
| 309 | capitalize it other than because it is the first word in the sentence. |
| 310 | E.g. "doc: clarify...", not "doc: Clarify...", or "githooks.txt: |
| 311 | improve...", not "githooks.txt: Improve...". But "refs: HEAD is also |
| 312 | treated as a ref" is correct, as we spell `HEAD` in all caps even when |
| 313 | it appears in the middle of a sentence. |
| 314 | |
| 315 | [[meaningful-message]] |
| 316 | The body should provide a meaningful commit message, which: |
| 317 | |
| 318 | . explains the problem the change tries to solve, i.e. what is wrong |
| 319 | with the current code without the change. |
| 320 | |
| 321 | . justifies the way the change solves the problem, i.e. why the |
| 322 | result with the change is better. |
| 323 | |
| 324 | . alternate solutions considered but discarded, if any. |
| 325 | |
| 326 | [[present-tense]] |
| 327 | The problem statement that describes the status quo is written in the |
| 328 | present tense. Write "The code does X when it is given input Y", |
| 329 | instead of "The code used to do Y when given input X". You do not |
| 330 | have to say "Currently"---the status quo in the problem statement is |
| 331 | about the code _without_ your change, by project convention. |
| 332 | |
| 333 | [[imperative-mood]] |
| 334 | Describe your changes in imperative mood, e.g. "make xyzzy do frotz" |
| 335 | instead of "[This patch] makes xyzzy do frotz" or "[I] changed xyzzy |
| 336 | to do frotz", as if you are giving orders to the codebase to change |
| 337 | its behavior. Try to make sure your explanation can be understood |
| 338 | without external resources. Instead of giving a URL to a mailing list |
| 339 | archive, summarize the relevant points of the discussion. |
| 340 | |
| 341 | [[commit-reference]] |
| 342 | |
| 343 | There are a few reasons why you may want to refer to another commit in |
| 344 | the "more stable" part of the history (i.e. on branches like `maint`, |
| 345 | `master`, and `next`): |
| 346 | |
| 347 | . A commit that introduced the root cause of a bug you are fixing. |
| 348 | |
| 349 | . A commit that introduced a feature that you are enhancing. |
| 350 | |
| 351 | . A commit that conflicts with your work when you made a trial merge |
| 352 | of your work into `next` and `seen` for testing. |
| 353 | |
| 354 | When you reference a commit on a more stable branch (like `master`, |
| 355 | `maint` and `next`), use the format "abbreviated hash (subject, |
| 356 | date)", like this: |
| 357 | |
| 358 | .... |
| 359 | Commit f86a374 (pack-bitmap.c: fix a memleak, 2015-03-30) |
| 360 | noticed that ... |
| 361 | .... |
| 362 | |
| 363 | The "Copy commit reference" command of gitk can be used to obtain this |
| 364 | format (with the subject enclosed in a pair of double-quotes), or this |
| 365 | invocation of `git show`: |
| 366 | |
| 367 | .... |
| 368 | git show -s --pretty=reference <commit> |
| 369 | .... |
| 370 | |
| 371 | or, on an older version of Git without support for --pretty=reference: |
| 372 | |
| 373 | .... |
| 374 | git show -s --date=short --pretty='format:%h (%s, %ad)' <commit> |
| 375 | .... |
| 376 | |
| 377 | [[sign-off]] |
| 378 | === Certify your work by adding your `Signed-off-by` trailer |
| 379 | |
| 380 | To improve tracking of who did what, we ask you to certify that you |
| 381 | wrote the patch or have the right to pass it on under the same license |
| 382 | as ours, by "signing off" your patch. Without sign-off, we cannot |
| 383 | accept your patches. |
| 384 | |
| 385 | If (and only if) you certify the below D-C-O: |
| 386 | |
| 387 | [[dco]] |
| 388 | .Developer's Certificate of Origin 1.1 |
| 389 | ____ |
| 390 | By making a contribution to this project, I certify that: |
| 391 | |
| 392 | a. The contribution was created in whole or in part by me and I |
| 393 | have the right to submit it under the open source license |
| 394 | indicated in the file; or |
| 395 | |
| 396 | b. The contribution is based upon previous work that, to the best |
| 397 | of my knowledge, is covered under an appropriate open source |
| 398 | license and I have the right under that license to submit that |
| 399 | work with modifications, whether created in whole or in part |
| 400 | by me, under the same open source license (unless I am |
| 401 | permitted to submit under a different license), as indicated |
| 402 | in the file; or |
| 403 | |
| 404 | c. The contribution was provided directly to me by some other |
| 405 | person who certified (a), (b) or (c) and I have not modified |
| 406 | it. |
| 407 | |
| 408 | d. I understand and agree that this project and the contribution |
| 409 | are public and that a record of the contribution (including all |
| 410 | personal information I submit with it, including my sign-off) is |
| 411 | maintained indefinitely and may be redistributed consistent with |
| 412 | this project or the open source license(s) involved. |
| 413 | ____ |
| 414 | |
| 415 | you add a "Signed-off-by" trailer to your commit, that looks like |
| 416 | this: |
| 417 | |
| 418 | .... |
| 419 | Signed-off-by: Random J Developer <random@developer.example.org> |
| 420 | .... |
| 421 | |
| 422 | This line can be added by Git if you run the git-commit command with |
| 423 | the -s option. |
| 424 | |
| 425 | Notice that you can place your own `Signed-off-by` trailer when |
| 426 | forwarding somebody else's patch with the above rules for |
| 427 | D-C-O. Indeed you are encouraged to do so. Do not forget to |
| 428 | place an in-body "From: " line at the beginning to properly attribute |
| 429 | the change to its true author (see (2) above). |
| 430 | |
| 431 | This procedure originally came from the Linux kernel project, so our |
| 432 | rule is quite similar to theirs, but what exactly it means to sign-off |
| 433 | your patch differs from project to project, so it may be different |
| 434 | from that of the project you are accustomed to. |
| 435 | |
| 436 | [[real-name]] |
| 437 | Please use a known identity in the `Signed-off-by` trailer, since we cannot |
| 438 | accept anonymous contributions. It is common, but not required, to use some form |
| 439 | of your real name. We realize that some contributors are not comfortable doing |
| 440 | so or prefer to contribute under a pseudonym or preferred name and we can accept |
| 441 | your patch either way, as long as the name and email you use are distinctive, |
| 442 | identifying, and not misleading. |
| 443 | |
| 444 | The goal of this policy is to allow us to have sufficient information to contact |
| 445 | you if questions arise about your contribution. |
| 446 | |
| 447 | [[commit-trailers]] |
| 448 | If you like, you can put extra trailers at the end: |
| 449 | |
| 450 | . `Reported-by:` is used to credit someone who found the bug that |
| 451 | the patch attempts to fix. |
| 452 | . `Acked-by:` says that the person who is more familiar with the area |
| 453 | the patch attempts to modify liked the patch. |
| 454 | . `Reviewed-by:`, unlike the other trailers, can only be offered by the |
| 455 | reviewers themselves when they are completely satisfied with the |
| 456 | patch after a detailed analysis. |
| 457 | . `Tested-by:` is used to indicate that the person applied the patch |
| 458 | and found it to have the desired effect. |
| 459 | . `Co-authored-by:` is used to indicate that people exchanged drafts |
| 460 | of a patch before submitting it. |
| 461 | . `Helped-by:` is used to credit someone who suggested ideas for |
| 462 | changes without providing the precise changes in patch form. |
| 463 | . `Mentored-by:` is used to credit someone with helping develop a |
| 464 | patch as part of a mentorship program (e.g., GSoC or Outreachy). |
| 465 | . `Suggested-by:` is used to credit someone with suggesting the idea |
| 466 | for a patch. |
| 467 | |
| 468 | While you can also create your own trailer if the situation warrants it, we |
| 469 | encourage you to instead use one of the common trailers in this project |
| 470 | highlighted above. |
| 471 | |
| 472 | Only capitalize the very first letter of the trailer, i.e. favor |
| 473 | "Signed-off-by" over "Signed-Off-By" and "Acked-by:" over "Acked-By". |
| 474 | |
| 475 | [[cover-letter]] |
| 476 | === Cover Letter |
| 477 | |
| 478 | The purpose of your cover letter is to sell your changes, explain what |
| 479 | they are about, and get your target audience interested enough to read |
| 480 | the patches. |
| 481 | |
| 482 | . Every code change comes with risk of regression and maintenance cost. |
| 483 | The cover letter should clearly communicate why the value of your |
| 484 | proposed change is worth applying. You can also describe how the risk |
| 485 | is reduced by the design choices you made while writing the patches. |
| 486 | |
| 487 | . Make sure your target audience can understand what the patches are |
| 488 | about and why they are needed without prior context. |
| 489 | |
| 490 | . For a second or subsequent iteration of the same topic, make sure |
| 491 | people who missed the earlier discussion can still understand what |
| 492 | the patches are about, so they can judge if the topic is worth their |
| 493 | time to read and comment on. |
| 494 | |
| 495 | . To help those who are familiar with earlier iterations, give a |
| 496 | summary of changes since the previous rounds. |
| 497 | |
| 498 | |
| 499 | [[ai]] |
| 500 | === Use of Artificial Intelligence (AI) |
| 501 | |
| 502 | The Developer's Certificate of Origin requires contributors to certify |
| 503 | that they know the origin of their contributions to the project and |
| 504 | that they have the right to submit it under the project's license. |
| 505 | It's not yet clear that this can be legally satisfied when submitting |
| 506 | significant amount of content that has been generated by AI tools. |
| 507 | |
| 508 | Another issue with AI generated content is that AIs still often |
| 509 | hallucinate or just produce bad code, commit messages, documentation |
| 510 | or output, even when you point out their mistakes. |
| 511 | |
| 512 | To avoid these issues, we will reject anything that looks AI |
| 513 | generated, that sounds overly formal or bloated, that looks like AI |
| 514 | slop, that looks good on the surface but makes no sense, or that |
| 515 | senders don’t understand or cannot explain. |
| 516 | |
| 517 | We strongly recommend using AI tools carefully and responsibly. |
| 518 | |
| 519 | Contributors would often benefit more from AI by using it to guide and |
| 520 | help them step by step towards producing a solution by themselves |
| 521 | rather than by asking for a full solution that they would then mostly |
| 522 | copy-paste. They can also use AI to help with debugging, or with |
| 523 | checking for obvious mistakes, things that can be improved, things |
| 524 | that don’t match our style, guidelines or our feedback, before sending |
| 525 | it to us. |
| 526 | |
| 527 | [[git-tools]] |
| 528 | === Generate your patch using Git tools out of your commits. |
| 529 | |
| 530 | Git based diff tools generate unidiff which is the preferred format. |
| 531 | |
| 532 | You do not have to be afraid to use `-M` option to `git diff` or |
| 533 | `git format-patch`, if your patch involves file renames. The |
| 534 | receiving end can handle them just fine. |
| 535 | |
| 536 | [[review-patch]] |
| 537 | Please make sure your patch does not add commented out debugging code, |
| 538 | or include any extra files which do not relate to what your patch |
| 539 | is trying to achieve. Make sure to review |
| 540 | your patch after generating it, to ensure accuracy. Before |
| 541 | sending out, please make sure it cleanly applies to the starting point you |
| 542 | have chosen in the "Choose a starting point" section. |
| 543 | |
| 544 | NOTE: From the perspective of those reviewing your patch, the `master` |
| 545 | branch is the default expected starting point. So if you have chosen a |
| 546 | different starting point, please communicate this choice in your cover |
| 547 | letter. |
| 548 | |
| 549 | |
| 550 | [[send-patches]] |
| 551 | === Sending your patches. |
| 552 | |
| 553 | ==== Choosing your reviewers |
| 554 | |
| 555 | :security-ml: footnoteref:[security-ml,The Git Security mailing list: git-security@googlegroups.com] |
| 556 | |
| 557 | NOTE: Patches that may be |
| 558 | security relevant should be submitted privately to the Git Security |
| 559 | mailing list{security-ml}, instead of the public mailing list. |
| 560 | |
| 561 | :contrib-scripts: footnoteref:[contrib-scripts,Scripts under `contrib/` are + |
| 562 | not part of the core `git` binary and must be called directly. Clone the Git + |
| 563 | codebase and run `perl contrib/contacts/git-contacts`.] |
| 564 | |
| 565 | Send your patch with "To:" set to the mailing list, with "cc:" listing |
| 566 | people who are involved in the area you are touching (the `git-contacts` |
| 567 | script in `contrib/contacts/`{contrib-scripts} can help to |
| 568 | identify them), to solicit comments and reviews. Also, when you made |
| 569 | trial merges of your topic to `next` and `seen`, you may have noticed |
| 570 | work by others conflicting with your changes. There is a good possibility |
| 571 | that these people may know the area you are touching well. |
| 572 | |
| 573 | If you are using `send-email`, you can feed it the output of `git-contacts` like |
| 574 | this: |
| 575 | |
| 576 | .... |
| 577 | git send-email --cc-cmd='perl contrib/contacts/git-contacts' feature/*.patch |
| 578 | .... |
| 579 | |
| 580 | :current-maintainer: footnote:[The current maintainer: gitster@pobox.com] |
| 581 | :git-ml: footnote:[The mailing list: git@vger.kernel.org] |
| 582 | |
| 583 | After the list reached a consensus that it is a good idea to apply the |
| 584 | patch, re-send it with "To:" set to the maintainer{current-maintainer} |
| 585 | and "cc:" the list{git-ml} for inclusion. This is especially relevant |
| 586 | when the maintainer did not heavily participate in the discussion and |
| 587 | instead left the review to trusted others. |
| 588 | |
| 589 | Do not forget to add trailers such as `Acked-by:`, `Reviewed-by:` and |
| 590 | `Tested-by:` lines as necessary to credit people who helped your |
| 591 | patch, and "cc:" them when sending such a final version for inclusion. |
| 592 | |
| 593 | ==== `format-patch` and `send-email` |
| 594 | |
| 595 | Learn to use `format-patch` and `send-email` if possible. These commands |
| 596 | are optimized for the workflow of sending patches, avoiding many ways |
| 597 | your existing e-mail client (often optimized for "multipart/*" MIME |
| 598 | type e-mails) might render your patches unusable. |
| 599 | |
| 600 | NOTE: Here we outline the procedure using `format-patch` and |
| 601 | `send-email`, but you can instead use GitGitGadget to send in your |
| 602 | patches (see link:MyFirstContribution.html[MyFirstContribution]). |
| 603 | |
| 604 | People on the Git mailing list need to be able to read and |
| 605 | comment on the changes you are submitting. It is important for |
| 606 | a developer to be able to "quote" your changes, using standard |
| 607 | e-mail tools, so that they may comment on specific portions of |
| 608 | your code. For this reason, each patch should be submitted |
| 609 | "inline" in a separate message. |
| 610 | |
| 611 | All subsequent versions of a patch series and other related patches should be |
| 612 | grouped into their own e-mail thread to help readers find all parts of the |
| 613 | series. To that end, send them as replies to either an additional "cover |
| 614 | letter" message (see below), the first patch, or the respective preceding patch. |
| 615 | Here is a link:MyFirstContribution.html#v2-git-send-email[step-by-step guide] on |
| 616 | how to submit updated versions of a patch series. |
| 617 | |
| 618 | If your log message (including your name on the |
| 619 | `Signed-off-by` trailer) is not writable in ASCII, make sure that |
| 620 | you send off a message in the correct encoding. |
| 621 | |
| 622 | WARNING: Be wary of your MUAs word-wrap |
| 623 | corrupting your patch. Do not cut-n-paste your patch; you can |
| 624 | lose tabs that way if you are not careful. |
| 625 | |
| 626 | It is a common convention to prefix your subject line with |
| 627 | [PATCH]. This lets people easily distinguish patches from other |
| 628 | e-mail discussions. Use of markers in addition to PATCH within |
| 629 | the brackets to describe the nature of the patch is also |
| 630 | encouraged. E.g. [RFC PATCH] (where RFC stands for "request for |
| 631 | comments") is often used to indicate a patch needs further |
| 632 | discussion before being accepted, [PATCH v2], [PATCH v3] etc. |
| 633 | are often seen when you are sending an update to what you have |
| 634 | previously sent. |
| 635 | |
| 636 | The `git format-patch` command follows the best current practice to |
| 637 | format the body of an e-mail message. At the beginning of the |
| 638 | patch should come your commit message, ending with the |
| 639 | `Signed-off-by` trailers, and a line that consists of three dashes, |
| 640 | followed by the diffstat information and the patch itself. If |
| 641 | you are forwarding a patch from somebody else, optionally, at |
| 642 | the beginning of the e-mail message just before the commit |
| 643 | message starts, you can put a "From: " line to name that person. |
| 644 | To change the default "[PATCH]" in the subject to "[<text>]", use |
| 645 | `git format-patch --subject-prefix=<text>`. As a shortcut, you |
| 646 | can use `--rfc` instead of `--subject-prefix="RFC PATCH"`, or |
| 647 | `-v <n>` instead of `--subject-prefix="PATCH v<n>"`. |
| 648 | |
| 649 | You often want to add additional explanation about the patch, |
| 650 | other than the commit message itself. Place such "cover letter" |
| 651 | material between the three-dash line and the diffstat. For |
| 652 | patches requiring multiple iterations of review and discussion, |
| 653 | an explanation of changes between each iteration can be kept in |
| 654 | Git-notes and inserted automatically following the three-dash |
| 655 | line via `git format-patch --notes`. |
| 656 | |
| 657 | [[the-topic-summary]] |
| 658 | *This is EXPERIMENTAL*. |
| 659 | |
| 660 | When sending a topic, you can optionally propose a topic name and/or a |
| 661 | one-paragraph summary that should appear in the "What's cooking" |
| 662 | report when it is picked up to explain the topic. If you choose to do |
| 663 | so, please write a 2-5 line paragraph that will fit well in our |
| 664 | release notes (see many bulleted entries in the |
| 665 | Documentation/RelNotes/* files for examples), and make it the first |
| 666 | (or second, if including a suggested topic name) paragraph of the |
| 667 | cover letter. If suggesting a topic name, use the format |
| 668 | "XX/your-topic-name", where "XX" is a stand-in for the primary |
| 669 | author's initials, and "your-topic-name" is a brief, dash-delimited |
| 670 | description of what your topic does. For a single-patch series, use |
| 671 | the space between the three-dash line and the diffstat, as described |
| 672 | earlier. |
| 673 | |
| 674 | [[multi-series-efforts]] |
| 675 | If your patch series is part of a larger effort spanning multiple |
| 676 | patch series, briefly describe the broader goal, and state where the |
| 677 | current series fits into that goal. If you are suggesting a topic |
| 678 | name as in <<the-topic-summary, section above>>, consider |
| 679 | "XX/the-broader-goal-part-one", "XX/the-broader-goal-part-two", and so |
| 680 | on. |
| 681 | |
| 682 | [[attachment]] |
| 683 | Do not attach the patch as a MIME attachment, compressed or not. |
| 684 | Do not let your e-mail client send quoted-printable. Do not let |
| 685 | your e-mail client send format=flowed which would destroy |
| 686 | whitespaces in your patches. Many |
| 687 | popular e-mail applications will not always transmit a MIME |
| 688 | attachment as plain text, making it impossible to comment on |
| 689 | your code. A MIME attachment also takes a bit more time to |
| 690 | process. This does not decrease the likelihood of your |
| 691 | MIME-attached change being accepted, but it makes it more likely |
| 692 | that it will be postponed. |
| 693 | |
| 694 | Exception: If your mailer is mangling patches then someone may ask |
| 695 | you to re-send them using MIME, that is OK. |
| 696 | |
| 697 | [[pgp-signature]] |
| 698 | Do not PGP sign your patch. Most likely, your maintainer or other people on the |
| 699 | list would not have your PGP key and would not bother obtaining it anyway. |
| 700 | Your patch is not judged by who you are; a good patch from an unknown origin |
| 701 | has a far better chance of being accepted than a patch from a known, respected |
| 702 | origin that is done poorly or does incorrect things. |
| 703 | |
| 704 | If you really really really really want to do a PGP signed |
| 705 | patch, format it as "multipart/signed", not a text/plain message |
| 706 | that starts with `-----BEGIN PGP SIGNED MESSAGE-----`. That is |
| 707 | not a text/plain, it's something else. |
| 708 | |
| 709 | === Handling Conflicts and Iterating Patches |
| 710 | |
| 711 | When revising changes made to your patches, it's important to |
| 712 | acknowledge the possibility of conflicts with other ongoing topics. To |
| 713 | navigate these potential conflicts effectively, follow the recommended |
| 714 | steps outlined below: |
| 715 | |
| 716 | . Build on a suitable base branch, see the <<choose-starting-point, section above>>, |
| 717 | and format-patch the series. If you are doing "rebase -i" in-place to |
| 718 | update from the previous round, this will reuse the previous base so |
| 719 | (2) and (3) may become trivial. |
| 720 | |
| 721 | . Find the base of where the last round was queued |
| 722 | + |
| 723 | $ mine='kn/ref-transaction-symref' |
| 724 | $ git checkout "origin/seen^{/^Merge branch '$mine'}...master" |
| 725 | |
| 726 | . Apply your format-patch result. There are two cases |
| 727 | .. Things apply cleanly and tests fine. Go to (4). |
| 728 | .. Things apply cleanly but does not build or test fails, or things do |
| 729 | not apply cleanly. |
| 730 | + |
| 731 | In the latter case, you have textual or semantic conflicts coming from |
| 732 | the difference between the old base and the base you used to build in |
| 733 | (1). Identify what caused the breakages (e.g., a topic or two may have |
| 734 | merged since the base used by (2) until the base used by (1)). |
| 735 | + |
| 736 | Check out the latest 'origin/master' (which may be newer than the base |
| 737 | used by (2)), "merge --no-ff" the topics you newly depend on in there, |
| 738 | and use the result of the merge(s) as the base, rebuild the series and |
| 739 | test again. Run format-patch from the last such merges to the tip of |
| 740 | your topic. If you did |
| 741 | + |
| 742 | $ git checkout origin/master |
| 743 | $ git merge --no-ff --into-name kn/ref-transaction-symref fo/obar |
| 744 | $ git merge --no-ff --into-name kn/ref-transaction-symref ba/zqux |
| 745 | ... rebuild the topic ... |
| 746 | + |
| 747 | Then you'd just format your topic above these "preparing the ground" |
| 748 | merges, e.g. |
| 749 | + |
| 750 | $ git format-patch "HEAD^{/^Merge branch 'ba/zqux'}"..HEAD |
| 751 | + |
| 752 | Do not forget to write in the cover letter you did this, including the |
| 753 | topics you have in your base on top of 'master'. Then go to (4). |
| 754 | |
| 755 | . Make a trial merge of your topic into 'next' and 'seen', e.g. |
| 756 | + |
| 757 | $ git checkout --detach 'origin/seen' |
| 758 | $ git revert -m 1 <the merge of the previous iteration into seen> |
| 759 | $ git merge kn/ref-transaction-symref |
| 760 | + |
| 761 | The "revert" is needed if the previous iteration of your topic is |
| 762 | already in 'seen' (like in this case). You could choose to rebuild |
| 763 | master..origin/seen from scratch while excluding your previous |
| 764 | iteration, which may emulate what happens on the maintainers end more |
| 765 | closely. |
| 766 | + |
| 767 | This trial merge may conflict. It is primarily to see what conflicts |
| 768 | _other_ topics may have with your topic. In other words, you do not |
| 769 | have to depend on it to make your topic work on 'master'. It may |
| 770 | become the job of the other topic owners to resolve conflicts if your |
| 771 | topic goes to 'next' before theirs. |
| 772 | + |
| 773 | Make a note on what conflict you saw in the cover letter. You do not |
| 774 | necessarily have to resolve them, but it would be a good opportunity to |
| 775 | learn what others are doing in related areas. |
| 776 | + |
| 777 | $ git checkout --detach 'origin/next' |
| 778 | $ git merge kn/ref-transaction-symref |
| 779 | + |
| 780 | This is to see what conflicts your topic has with other topics that are |
| 781 | already cooking. This should not conflict if (3)-2 prepared a base on |
| 782 | top of updated master plus dependent topics taken from 'next'. Unless |
| 783 | the context is severe (one way to tell is try the same trial merge with |
| 784 | your old iteration, which may conflict in a similar way), expect that it |
| 785 | will be handled on maintainers end (if it gets unmanageable, I'll ask to |
| 786 | rebase when I receive your patches). |
| 787 | |
| 788 | == Subsystems with dedicated maintainers |
| 789 | |
| 790 | Some parts of the system have dedicated maintainers with their own |
| 791 | repositories. |
| 792 | |
| 793 | - `git-gui/` comes from the git-gui project, maintained by Johannes Sixt: |
| 794 | |
| 795 | https://github.com/j6t/git-gui |
| 796 | |
| 797 | Contibutions should go via the git mailing list. |
| 798 | |
| 799 | - `gitk-git/` comes from the gitk project, maintained by Johannes Sixt: |
| 800 | |
| 801 | https://github.com/j6t/gitk |
| 802 | |
| 803 | Contibutions should go via the git mailing list. |
| 804 | |
| 805 | - `po/` comes from the localization coordinator, Jiang Xin: |
| 806 | |
| 807 | https://github.com/git-l10n/git-po/ |
| 808 | |
| 809 | Patches to these parts should be based on their trees. |
| 810 | |
| 811 | - The "Git documentation translations" project, led by Jean-Noël |
| 812 | Avila, translates our documentation pages. Their work products are |
| 813 | maintained separately from this project, not as part of our tree: |
| 814 | |
| 815 | https://github.com/jnavila/git-manpages-l10n/ |
| 816 | |
| 817 | |
| 818 | == GitHub CI[[GHCI]] |
| 819 | |
| 820 | With an account at GitHub, you can use GitHub CI to test your changes |
| 821 | on Linux, Mac and Windows. See |
| 822 | https://github.com/git/git/actions/workflows/main.yml for examples of |
| 823 | recent CI runs. |
| 824 | |
| 825 | Follow these steps for the initial setup: |
| 826 | |
| 827 | . Fork https://github.com/git/git to your GitHub account. |
| 828 | You can find detailed instructions how to fork here: |
| 829 | https://help.github.com/articles/fork-a-repo/ |
| 830 | |
| 831 | After the initial setup, CI will run whenever you push new changes |
| 832 | to your fork of Git on GitHub. You can monitor the test state of all your |
| 833 | branches here: `https://github.com/<Your GitHub handle>/git/actions/workflows/main.yml` |
| 834 | |
| 835 | If a branch does not pass all test cases then it will be marked with a |
| 836 | red +x+, instead of a green check. In that case, you can click on the |
| 837 | failing job and navigate to "ci/run-build-and-tests.sh" and/or |
| 838 | "ci/print-test-failures.sh". You can also download "Artifacts" which |
| 839 | are zip archives containing tarred (or zipped) archives with test data |
| 840 | relevant for debugging. |
| 841 | |
| 842 | Then fix the problem and push your fix to your GitHub fork. This will |
| 843 | trigger a new CI build to ensure all tests pass. |
| 844 | |
| 845 | Even if you do not use GitHub CI to test your changes, pay close |
| 846 | attention to new failures on the branches when the maintainer pushes |
| 847 | out after your topic gets merged to the 'seen' branch to make sure |
| 848 | that your topic is not breaking the CI, and retract your breaking |
| 849 | topic quickly while you fix the breakage you caused. |
| 850 | |
| 851 | To see maintainer's push, keep an eye on this page: |
| 852 | |
| 853 | `https://github.com/git/git/actions/workflows/main.yml?query=event%3Apush+actor%3Agitster` |
| 854 | |
| 855 | |
| 856 | [[mua]] |
| 857 | == MUA specific hints |
| 858 | |
| 859 | Some of the patches I receive or pick up from the list share common |
| 860 | patterns of breakage. Please make sure your MUA is set up |
| 861 | properly not to corrupt whitespaces. |
| 862 | |
| 863 | See the DISCUSSION section of linkgit:git-format-patch[1] for hints on |
| 864 | checking your patch by mailing it to yourself and applying with |
| 865 | linkgit:git-am[1]. |
| 866 | |
| 867 | While you are at it, check the resulting commit log message from |
| 868 | a trial run of applying the patch. If what is in the resulting |
| 869 | commit is not exactly what you would want to see, it is very |
| 870 | likely that your maintainer would end up hand editing the log |
| 871 | message when he applies your patch. Things like "Hi, this is my |
| 872 | first patch.\n", if you really want to put in the patch e-mail, |
| 873 | should come after the three-dash line that signals the end of the |
| 874 | commit message. |
| 875 | |
| 876 | |
| 877 | === Pine |
| 878 | |
| 879 | (Johannes Schindelin) |
| 880 | |
| 881 | .... |
| 882 | I don't know how many people still use pine, but for those poor |
| 883 | souls it may be good to mention that the quell-flowed-text is |
| 884 | needed for recent versions. |
| 885 | |
| 886 | ... the "no-strip-whitespace-before-send" option, too. AFAIK it |
| 887 | was introduced in 4.60. |
| 888 | .... |
| 889 | |
| 890 | (Linus Torvalds) |
| 891 | |
| 892 | .... |
| 893 | And 4.58 needs at least this. |
| 894 | |
| 895 | diff-tree 8326dd8350be64ac7fc805f6563a1d61ad10d32c (from e886a61f76edf5410573e92e38ce22974f9c40f1) |
| 896 | Author: Linus Torvalds <torvalds@g5.osdl.org> |
| 897 | Date: Mon Aug 15 17:23:51 2005 -0700 |
| 898 | |
| 899 | Fix pine whitespace-corruption bug |
| 900 | |
| 901 | There's no excuse for unconditionally removing whitespace from |
| 902 | the pico buffers on close. |
| 903 | |
| 904 | diff --git a/pico/pico.c b/pico/pico.c |
| 905 | --- a/pico/pico.c |
| 906 | +++ b/pico/pico.c |
| 907 | @@ -219,7 +219,9 @@ PICO *pm; |
| 908 | switch(pico_all_done){ /* prepare for/handle final events */ |
| 909 | case COMP_EXIT : /* already confirmed */ |
| 910 | packheader(); |
| 911 | +#if 0 |
| 912 | stripwhitespace(); |
| 913 | +#endif |
| 914 | c |= COMP_EXIT; |
| 915 | break; |
| 916 | .... |
| 917 | |
| 918 | (Daniel Barkalow) |
| 919 | |
| 920 | .... |
| 921 | > A patch to SubmittingPatches, MUA specific help section for |
| 922 | > users of Pine 4.63 would be very much appreciated. |
| 923 | |
| 924 | Ah, it looks like a recent version changed the default behavior to do the |
| 925 | right thing, and inverted the sense of the configuration option. (Either |
| 926 | that or Gentoo did it.) So you need to set the |
| 927 | "no-strip-whitespace-before-send" option, unless the option you have is |
| 928 | "strip-whitespace-before-send", in which case you should avoid checking |
| 929 | it. |
| 930 | .... |
| 931 | |
| 932 | === Thunderbird, KMail, GMail |
| 933 | |
| 934 | See the MUA-SPECIFIC HINTS section of linkgit:git-format-patch[1]. |
| 935 | |
| 936 | === Gnus |
| 937 | |
| 938 | "|" in the `*Summary*` buffer can be used to pipe the current |
| 939 | message to an external program, and this is a handy way to drive |
| 940 | `git am`. However, if the message is MIME encoded, what is |
| 941 | piped into the program is the representation you see in your |
| 942 | `*Article*` buffer after unwrapping MIME. This is often not what |
| 943 | you would want for two reasons. It tends to screw up non-ASCII |
| 944 | characters (most notably in people's names), and also |
| 945 | whitespaces (fatal in patches). Running "C-u g" to display the |
| 946 | message in raw form before using "|" to run the pipe can work |
| 947 | this problem around. |