| Message ID | 20211119193118.949698-2-kchamart@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Fix broken rST conversion of "submitting-a-patch.rst" | expand |
On 19/11/2021 20.31, Kashyap Chamarthy wrote: > I completely botched up the merged[0] rST conversion of this document by > accidentally dropping entire hunks (!) of text. :-( I made it very hard > for reviewers to spot it, as the omitted text was buried deep in the > document. To fix my hatchet job, I reconverted the "SubmitAPatch" > wiki[1] page from scratch and replaced the existing rST with it, while > making sure I incorporated previous feedback. Thanks for the quick update! I've now tripple-checked the differences between the old wiki page and the new rst page, and I think with some additional small changes on top (which I will do while picking up your patch, see below), we should be fine now. [...] > @@ -55,16 +55,15 @@ Writing your Patches > Use the QEMU coding style > ~~~~~~~~~~~~~~~~~~~~~~~~~ > > -You can run run *scripts/checkpatch.pl <patchfile>* before submitting to > -check that you are in compliance with our coding standards. Be aware > -that ``checkpatch.pl`` is not infallible, though, especially where C > +You can run run *scripts/checkpatch.pl <patch file>* before submitting > +to check that you are in compliance with our coding standards. Be aware > +that checkpatch.pl is not infallible, though, especially where C > preprocessor macros are involved; use some common sense too. See also: > > -- `QEMU Coding Style > - <https://qemu-project.gitlab.io/qemu/devel/style.html>`__ > - > +- `QEMU Coding Style <https://qemu-project.gitlab.io/qemu/devel/style.html>`__ While we're at it, I'll replace the link with an internal link when picking up this patch (so that it can also be used in offline documentation). > - `Automate a checkpatch run on > - commit <http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html>`__ > + commit <https://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html>`__ > +- Spell check your patches I'll add the link to https://wiki.qemu.org/Contribute/SpellCheck again. > @@ -85,7 +91,7 @@ Split up longer patches into a patch series of logical code changes. > Each change should compile and execute successfully. For instance, don't > add a file to the makefile in patch one and then add the file itself in > patch two. (This rule is here so that people can later use tools like > -`git bisect <http://git-scm.com/docs/git-bisect>`__ without hitting > +```git bisect`` <http://git-scm.com/docs/git-bisect>`__ without hitting You've re-introduced the broken git-bisect link. I'll fix it again. > @@ -170,11 +175,74 @@ displays the subject line some distance apart (that is, a body that > starts with "... so that" as a continuation of the subject line is > harder to follow). > > +If your patch fixes a commit that is already in the repository, please > +add a line with "Fixes: ("Fixed commit subject")" below the patch Missing the "<at-least-12-digits-of-SHA-commit-id>" from the Wiki page. I'll add it. > +description / before your "Signed-off-by:" line in the commit message. > + > +If your patch fixes a bug in the gitlab bug tracker, please add a line > +with "Resolves: " to the commit message, too. Gitlab can close bugs Missing the "<URL-of-the-bug>" from the Wiki. I'll add it. > +automatically once commits with the "Resolved:" keyword get merged into > +the master branch of the project. And if your patch addresses a bug in > +another public bug tracker, you can also use a line with "Buglink: " for dito. > @@ -223,13 +291,22 @@ use --numbered so the cover and the patch have distinct subject lines). I noticed that the --numbered will be translated into –numbered, so I'll add some `` quotes here. > @@ -288,6 +370,18 @@ it's best to: > of the patchset you're looking for review on, and why reviewers > should care > > +.. _consider_whether_your_patch_is_applicable_for_stable: > + > +Consider whether your patch is applicable for stable > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > + > +If your patch fixes a severe issue or a regression, it may be applicable > +for stable. In that case, consider adding ``Cc: qemu-stable@nongnu.org`` > +to your patch to notify the stable maintainers. > + > +For more details on how QEMU's stable process works, refer to > +`docs/devel/stable-process.rst <https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/devel/stable-process.rst;hb=HEAD>`__. While at this, I'll also replace this external link with an internal one. > Is my patch in? > ~~~~~~~~~~~~~~~ > > +QEMU has some Continuous Integration machines that try to catch patch > +submission problems as soon as possible. `patchew > +<http://patchew.org/QEMU/>`__ includes a web interface for tracking the > +status of various threads that have been posted to the list, and may > +send you an automated mail if it detected a problem with your patch. > + > Once your patch has had enough review on list, the maintainer for that > area of code will send notification to the list that they are including > your patch in a particular staging branch. Periodically, the maintainer > then sends a `pull request > <https://qemu-project.gitlab.io/qemu/devel/submitting-a-pull-request.html>`__ I'll also use an internal link for this reference here. > -for aggregating topic branches into mainline qemu. Generally, you do not > +for aggregating topic branches into mainline QEMU. Generally, you do not > need to send a pull request unless you have contributed enough patches > to become a maintainer over a particular section of code. Maintainers > may further modify your commit, by resolving simple merge conflicts or Thomas
On Mon, Nov 22, 2021 at 10:51:15AM +0100, Thomas Huth wrote: > On 19/11/2021 20.31, Kashyap Chamarthy wrote: > > I completely botched up the merged[0] rST conversion of this document by > > accidentally dropping entire hunks (!) of text. :-( I made it very hard > > for reviewers to spot it, as the omitted text was buried deep in the > > document. To fix my hatchet job, I reconverted the "SubmitAPatch" > > wiki[1] page from scratch and replaced the existing rST with it, while > > making sure I incorporated previous feedback. > > Thanks for the quick update! I've now tripple-checked the differences > between the old wiki page and the new rst page, and I think with some > additional small changes on top (which I will do while picking up your > patch, see below), we should be fine now. Thanks for your eagle eyes. [...] > > -- `QEMU Coding Style > > - <https://qemu-project.gitlab.io/qemu/devel/style.html>`__ > > - > > +- `QEMU Coding Style <https://qemu-project.gitlab.io/qemu/devel/style.html>`__ > > While we're at it, I'll replace the link with an internal link when picking > up this patch (so that it can also be used in offline documentation). Sure. > > - `Automate a checkpatch run on > > - commit <http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html>`__ > > + commit <https://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html>`__ > > +- Spell check your patches > > I'll add the link to https://wiki.qemu.org/Contribute/SpellCheck again. Please don't -- I intentionally dropped it in a previous iteration based on this feedback from Peter Maydell: "I'm not sure that SpellCheck in particular is sufficiently baked to be in the actual docs. I'd rather just drop the reference to it from SubmitAPatch." (https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg00137.html) > > @@ -85,7 +91,7 @@ Split up longer patches into a patch series of logical code changes. > > Each change should compile and execute successfully. For instance, don't > > add a file to the makefile in patch one and then add the file itself in > > patch two. (This rule is here so that people can later use tools like > > -`git bisect <http://git-scm.com/docs/git-bisect>`__ without hitting > > +```git bisect`` <http://git-scm.com/docs/git-bisect>`__ without hitting > > You've re-introduced the broken git-bisect link. I'll fix it again. Oops; thanks! It's hard to spot these double-ticks. > > @@ -170,11 +175,74 @@ displays the subject line some distance apart (that is, a body that > > starts with "... so that" as a continuation of the subject line is > > harder to follow). > > +If your patch fixes a commit that is already in the repository, please > > +add a line with "Fixes: ("Fixed commit subject")" below the patch > > Missing the "<at-least-12-digits-of-SHA-commit-id>" from the Wiki page. I'll > add it. > > > +description / before your "Signed-off-by:" line in the commit message. > > + > > +If your patch fixes a bug in the gitlab bug tracker, please add a line > > +with "Resolves: " to the commit message, too. Gitlab can close bugs > > Missing the "<URL-of-the-bug>" from the Wiki. I'll add it. Uh, not sure how I missed the above two bits. > > +automatically once commits with the "Resolved:" keyword get merged into > > +the master branch of the project. And if your patch addresses a bug in > > +another public bug tracker, you can also use a line with "Buglink: " for > > dito. > > > @@ -223,13 +291,22 @@ use --numbered so the cover and the patch have distinct subject lines). > > I noticed that the --numbered will be translated into –numbered, so I'll add > some `` quotes here. Good catch; my eyes glazed over it. > > @@ -288,6 +370,18 @@ it's best to: > > of the patchset you're looking for review on, and why reviewers > > should care > > +.. _consider_whether_your_patch_is_applicable_for_stable: > > + > > +Consider whether your patch is applicable for stable > > +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > + > > +If your patch fixes a severe issue or a regression, it may be applicable > > +for stable. In that case, consider adding ``Cc: qemu-stable@nongnu.org`` > > +to your patch to notify the stable maintainers. > > + > > +For more details on how QEMU's stable process works, refer to > > +`docs/devel/stable-process.rst <https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/devel/stable-process.rst;hb=HEAD>`__. > > While at this, I'll also replace this external link with an internal one. [...] > > your patch in a particular staging branch. Periodically, the maintainer > > then sends a `pull request > > <https://qemu-project.gitlab.io/qemu/devel/submitting-a-pull-request.html>`__ > > I'll also use an internal link for this reference here. Sure. Many thanks for the careful combing! [...]
On 22/11/2021 11.20, Kashyap Chamarthy wrote: > On Mon, Nov 22, 2021 at 10:51:15AM +0100, Thomas Huth wrote: >> On 19/11/2021 20.31, Kashyap Chamarthy wrote: >>> I completely botched up the merged[0] rST conversion of this document by >>> accidentally dropping entire hunks (!) of text. :-( I made it very hard >>> for reviewers to spot it, as the omitted text was buried deep in the >>> document. To fix my hatchet job, I reconverted the "SubmitAPatch" >>> wiki[1] page from scratch and replaced the existing rST with it, while >>> making sure I incorporated previous feedback. >> >> Thanks for the quick update! I've now tripple-checked the differences >> between the old wiki page and the new rst page, and I think with some >> additional small changes on top (which I will do while picking up your >> patch, see below), we should be fine now. > > Thanks for your eagle eyes. > > [...] > >>> -- `QEMU Coding Style >>> - <https://qemu-project.gitlab.io/qemu/devel/style.html>`__ >>> - >>> +- `QEMU Coding Style <https://qemu-project.gitlab.io/qemu/devel/style.html>`__ >> >> While we're at it, I'll replace the link with an internal link when picking >> up this patch (so that it can also be used in offline documentation). > > Sure. > >>> - `Automate a checkpatch run on >>> - commit <http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html>`__ >>> + commit <https://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html>`__ >>> +- Spell check your patches >> >> I'll add the link to https://wiki.qemu.org/Contribute/SpellCheck again. > > Please don't -- I intentionally dropped it in a previous iteration based > on this feedback from Peter Maydell: > > "I'm not sure that SpellCheck in particular is sufficiently baked > to be in the actual docs. I'd rather just drop the reference to it > from SubmitAPatch." > > (https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg00137.html) Ok, so I'll drop that line completely (otherwise it does not really make sense to have it in this list after the "See also:" right in front of it). Thomas
On Mon, Nov 22, 2021 at 11:36:54AM +0100, Thomas Huth wrote: > On 22/11/2021 11.20, Kashyap Chamarthy wrote: > > On Mon, Nov 22, 2021 at 10:51:15AM +0100, Thomas Huth wrote: [...] > > > I'll add the link to https://wiki.qemu.org/Contribute/SpellCheck again. > > > > Please don't -- I intentionally dropped it in a previous iteration based > > on this feedback from Peter Maydell: > > > > "I'm not sure that SpellCheck in particular is sufficiently baked > > to be in the actual docs. I'd rather just drop the reference to it > > from SubmitAPatch." > > (https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg00137.html) > > Ok, so I'll drop that line completely (otherwise it does not really make > sense to have it in this list after the "See also:" right in front of it). I kept that because an explicit reminder about spell-check is useful; it's easy to forget. Another option is to retain the line, but replace "See also:" with "And:". Not sure if it's any better. But I don't mind either way.
On 22/11/2021 13.19, Kashyap Chamarthy wrote: > On Mon, Nov 22, 2021 at 11:36:54AM +0100, Thomas Huth wrote: >> On 22/11/2021 11.20, Kashyap Chamarthy wrote: >>> On Mon, Nov 22, 2021 at 10:51:15AM +0100, Thomas Huth wrote: > > [...] > >>>> I'll add the link to https://wiki.qemu.org/Contribute/SpellCheck again. >>> >>> Please don't -- I intentionally dropped it in a previous iteration based >>> on this feedback from Peter Maydell: >>> >>> "I'm not sure that SpellCheck in particular is sufficiently baked >>> to be in the actual docs. I'd rather just drop the reference to it >>> from SubmitAPatch." >>> (https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg00137.html) >> >> Ok, so I'll drop that line completely (otherwise it does not really make >> sense to have it in this list after the "See also:" right in front of it). > > I kept that because an explicit reminder about spell-check is useful; > it's easy to forget. Another option is to retain the line, but replace > "See also:" with "And:". Not sure if it's any better. But I don't mind > either way. What about simply replacing it with a new sentence below the bullet list, saying: "Please also use a spell checker like `codespell https://github.com/codespell-project/codespell` with your patches" ? Thomas
On Mon, Nov 22, 2021 at 01:36:53PM +0100, Thomas Huth wrote: > On 22/11/2021 13.19, Kashyap Chamarthy wrote: [...] > > I kept that because an explicit reminder about spell-check is useful; > > it's easy to forget. Another option is to retain the line, but replace > > "See also:" with "And:". Not sure if it's any better. But I don't mind > > either way. > > What about simply replacing it with a new sentence below the bullet list, > saying: > > "Please also use a spell checker like `codespell > https://github.com/codespell-project/codespell` with your patches" > > ? Sounds good to me; go for it. Thanks!
On Mon, 22 Nov 2021 at 12:37, Thomas Huth <thuth@redhat.com> wrote: > What about simply replacing it with a new sentence below the bullet list, > saying: > > "Please also use a spell checker like `codespell > https://github.com/codespell-project/codespell` with your patches" How many regular contributors actually do that? -- PMM
On 22/11/2021 14.25, Peter Maydell wrote: > On Mon, 22 Nov 2021 at 12:37, Thomas Huth <thuth@redhat.com> wrote: >> What about simply replacing it with a new sentence below the bullet list, >> saying: >> >> "Please also use a spell checker like `codespell >> https://github.com/codespell-project/codespell` with your patches" > > How many regular contributors actually do that? Considering the typos that we have in the code, not enough ;-) Anyway, it's just a polite recommendation here, not a must-do, so mentioning codespell here doesn't really hurt, does it? Thomas
On Mon, Nov 22, 2021 at 02:30:10PM +0100, Thomas Huth wrote: > On 22/11/2021 14.25, Peter Maydell wrote: > > On Mon, 22 Nov 2021 at 12:37, Thomas Huth <thuth@redhat.com> wrote: > > > What about simply replacing it with a new sentence below the bullet list, > > > saying: > > > > > > "Please also use a spell checker like `codespell > > > https://github.com/codespell-project/codespell` with your patches" > > > > How many regular contributors actually do that? > > Considering the typos that we have in the code, not enough ;-) > > Anyway, it's just a polite recommendation here, not a must-do, so mentioning > codespell here doesn't really hurt, does it? Well if you run 'codespell' with no args on qemu.git right now, you get over 5000 possible mistakes reported. Many (perhaps even most) will be false positives, but with that amount of existing report, I don't think its credible to request contributors to run this and wade through its results to see if they made things worse or not. Regards, Daniel
On 22/11/2021 14.33, Daniel P. Berrangé wrote: > On Mon, Nov 22, 2021 at 02:30:10PM +0100, Thomas Huth wrote: >> On 22/11/2021 14.25, Peter Maydell wrote: >>> On Mon, 22 Nov 2021 at 12:37, Thomas Huth <thuth@redhat.com> wrote: >>>> What about simply replacing it with a new sentence below the bullet list, >>>> saying: >>>> >>>> "Please also use a spell checker like `codespell >>>> https://github.com/codespell-project/codespell` with your patches" >>> >>> How many regular contributors actually do that? >> >> Considering the typos that we have in the code, not enough ;-) >> >> Anyway, it's just a polite recommendation here, not a must-do, so mentioning >> codespell here doesn't really hurt, does it? > > Well if you run 'codespell' with no args on qemu.git right now, you > get over 5000 possible mistakes reported. Many (perhaps even most) > will be false positives, but with that amount of existing report, > I don't think its credible to request contributors to run this > and wade through its results to see if they made things worse or > not. Ok, then maybe something like this: "If your patch adds new documentation, then please also consider to use a spell checker like `codespell` to avoid typos in the new text" ? Otherwise, I can also simply drop that sentence about spell checking. Thomas
On Mon, 22 Nov 2021 at 13:30, Thomas Huth <thuth@redhat.com> wrote: > > On 22/11/2021 14.25, Peter Maydell wrote: > > On Mon, 22 Nov 2021 at 12:37, Thomas Huth <thuth@redhat.com> wrote: > >> What about simply replacing it with a new sentence below the bullet list, > >> saying: > >> > >> "Please also use a spell checker like `codespell > >> https://github.com/codespell-project/codespell` with your patches" > > > > How many regular contributors actually do that? > > Considering the typos that we have in the code, not enough ;-) > > Anyway, it's just a polite recommendation here, not a must-do, so mentioning > codespell here doesn't really hurt, does it? I don't think we should be recommending to new contributors that they do things that established contributors generally do not do. The document has enough "things you should do or think about" already. My preference would be for simply not mentioning spelling-checking. (If we do want to come up with some process for dealing with spelling issues in the codebase, then we either need to put it into CI so it's run automatically, or we need to have something that works on the individual patch level.) -- PMM
On 22/11/2021 14.53, Peter Maydell wrote: > On Mon, 22 Nov 2021 at 13:30, Thomas Huth <thuth@redhat.com> wrote: >> >> On 22/11/2021 14.25, Peter Maydell wrote: >>> On Mon, 22 Nov 2021 at 12:37, Thomas Huth <thuth@redhat.com> wrote: >>>> What about simply replacing it with a new sentence below the bullet list, >>>> saying: >>>> >>>> "Please also use a spell checker like `codespell >>>> https://github.com/codespell-project/codespell` with your patches" >>> >>> How many regular contributors actually do that? >> >> Considering the typos that we have in the code, not enough ;-) >> >> Anyway, it's just a polite recommendation here, not a must-do, so mentioning >> codespell here doesn't really hurt, does it? > > I don't think we should be recommending to new contributors that > they do things that established contributors generally do not do. > The document has enough "things you should do or think about" already. > My preference would be for simply not mentioning spelling-checking. > > (If we do want to come up with some process for dealing with > spelling issues in the codebase, then we either need to put it > into CI so it's run automatically, or we need to have something > that works on the individual patch level.) Ok ... In any case - seems like this needs more discussion, so I'll drop it from the patch for now. We can still add some wording or CI magic later, but that's certainly something that we rather want to do after version 6.2 has been released... Thomas
On Mon, Nov 22, 2021 at 03:01:58PM +0100, Thomas Huth wrote: > On 22/11/2021 14.53, Peter Maydell wrote: [...] > > I don't think we should be recommending to new contributors that > > they do things that established contributors generally do not do. > > The document has enough "things you should do or think about" already. > > My preference would be for simply not mentioning spelling-checking. Fair points; and yes, the doc can be intimidating as is. > > (If we do want to come up with some process for dealing with > > spelling issues in the codebase, then we either need to put it > > into CI so it's run automatically, or we need to have something > > that works on the individual patch level.) For individual patches, some projects use commit hooks for `aspell` / `codespell`. The contributor still needs to wade through false positives, though. For Sphinx-based documentation, there's "sphinxcontrib-spelling"[1]. I don't know how effective it is, but it lets one configure project-specific private dictionaries[2], which could eliminate many false positives. > Ok ... In any case - seems like this needs more discussion, so I'll > drop it from the patch for now. We can still add some wording or CI > magic later, but that's certainly something that we rather want to do > after version 6.2 has been released... Yeah, dropping it sounds fine. [1] https://github.com/sphinx-contrib/spelling [2] https://sphinxcontrib-spelling.readthedocs.io/en/latest/customize.html#private-dictionaries
diff --git a/docs/devel/submitting-a-patch.rst b/docs/devel/submitting-a-patch.rst index 6fefc67d52..ae6a9d45ab 100644 --- a/docs/devel/submitting-a-patch.rst +++ b/docs/devel/submitting-a-patch.rst @@ -12,19 +12,19 @@ one-shot fix, the bare minimum we ask is that: - You **must** provide a Signed-off-by: line (this is a hard requirement because it's how you say "I'm legally okay to contribute - this and happy for it to go into QEMU", modeled after the `Linux kernel - <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__ + this and happy for it to go into QEMU", modeled after the `Linux + kernel <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__ policy.) ``git commit -s`` or ``git format-patch -s`` will add one. - All contributions to QEMU must be **sent as patches** to the qemu-devel `mailing list <MailingLists>`__. Patch contributions should not be posted on the bug tracker, posted on forums, or externally hosted and linked to. (We have other mailing lists too, but all patches must go to qemu-devel, possibly with a Cc: to another - list.) ``git send-email`` works best for delivering the patch without - mangling it (`hints for setting it - up <http://lxr.free-electrons.com/source/Documentation/process/email-clients.rst>`__), - but attachments can be used as a last resort on a first-time - submission. + list.) ``git send-email`` (`step-by-step setup + guide <https://git-send-email.io/>`__ and `hints and + tips <https://elixir.bootlin.com/linux/latest/source/Documentation/process/email-clients.rst>`__) + works best for delivering the patch without mangling it, but + attachments can be used as a last resort on a first-time submission. - You must read replies to your message, and be willing to act on them. Note, however, that maintainers are often willing to manually fix up first-time contributions, since there is a learning curve involved in @@ -55,16 +55,15 @@ Writing your Patches Use the QEMU coding style ~~~~~~~~~~~~~~~~~~~~~~~~~ -You can run run *scripts/checkpatch.pl <patchfile>* before submitting to -check that you are in compliance with our coding standards. Be aware -that ``checkpatch.pl`` is not infallible, though, especially where C +You can run run *scripts/checkpatch.pl <patch file>* before submitting +to check that you are in compliance with our coding standards. Be aware +that checkpatch.pl is not infallible, though, especially where C preprocessor macros are involved; use some common sense too. See also: -- `QEMU Coding Style - <https://qemu-project.gitlab.io/qemu/devel/style.html>`__ - +- `QEMU Coding Style <https://qemu-project.gitlab.io/qemu/devel/style.html>`__ - `Automate a checkpatch run on - commit <http://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html>`__ + commit <https://blog.vmsplice.net/2011/03/how-to-automatically-run-checkpatchpl.html>`__ +- Spell check your patches .. _base_patches_against_current_git_master: @@ -76,6 +75,13 @@ of QEMU because development will have moved on from then and it probably won't even apply to master. We only apply selected bugfixes to release branches and then only as backports once the code has gone into master. +It is also okay to base patches on top of other on-going work that is +not yet part of the git master branch. To aid continuous integration +tools, such as `patchew <http://patchew.org/QEMU/>`__, you should `add a +tag <https://lists.gnu.org/archive/html/qemu-devel/2017-08/msg01288.html>`__ +line ``Based-on: $MESSAGE_ID`` to your cover letter to make the series +dependency obvious. + .. _split_up_long_patches: Split up long patches @@ -85,7 +91,7 @@ Split up longer patches into a patch series of logical code changes. Each change should compile and execute successfully. For instance, don't add a file to the makefile in patch one and then add the file itself in patch two. (This rule is here so that people can later use tools like -`git bisect <http://git-scm.com/docs/git-bisect>`__ without hitting +```git bisect`` <http://git-scm.com/docs/git-bisect>`__ without hitting points in the commit history where QEMU doesn't work for reasons unrelated to the bug they're chasing.) Put documentation first, not last, so that someone reading the series can do a clean-room evaluation @@ -104,18 +110,17 @@ Make code motion patches easy to review If a series requires large blocks of code motion, there are tricks for making the refactoring easier to review. Split up the series so that semantic changes (or even function renames) are done in a separate patch -from the raw code motion. Use a one-time setup of -``git config diff.renames true; git config diff.algorithm patience`` -(Refer to `git-config <http://git-scm.com/docs/git-config>`__.) The -``diff.renames`` property ensures file rename patches will be given in a -more compact representation that focuses only on the differences across -the file rename, instead of showing the entire old file as a deletion -and the new file as an insertion. Meanwhile, the 'diff.algorithm' -property ensures that extracting a non-contiguous subset of one file -into a new file, but where all extracted parts occur in the same order -both before and after the patch, will reduce churn in trying to treat -unrelated ``}`` lines in the original file as separating hunks of -changes. +from the raw code motion. Use a one-time setup of ``git config +diff.renames true;`` ``git config diff.algorithm patience`` (refer to +`git-config <http://git-scm.com/docs/git-config>`__). The 'diff.renames' +property ensures file rename patches will be given in a more compact +representation that focuses only on the differences across the file +rename, instead of showing the entire old file as a deletion and the new +file as an insertion. Meanwhile, the 'diff.algorithm' property ensures +that extracting a non-contiguous subset of one file into a new file, but +where all extracted parts occur in the same order both before and after +the patch, will reduce churn in trying to treat unrelated ``}`` lines in +the original file as separating hunks of changes. Ideally, a code motion patch can be reviewed by doing:: @@ -154,7 +159,7 @@ QEMU follows the usual standard for git commit messages: the first line (which becomes the email subject line) is "subsystem: single line summary of change". Whether the "single line summary of change" starts with a capital is a matter of taste, but we prefer that the summary does -not end in ".". Look at ``git shortlog -30`` for an idea of sample +not end in a dot. Look at ``git shortlog -30`` for an idea of sample subject lines. Then there is a blank line and a more detailed description of the patch, another blank and your Signed-off-by: line. Please do not use lines that are longer than 76 characters in your @@ -170,11 +175,74 @@ displays the subject line some distance apart (that is, a body that starts with "... so that" as a continuation of the subject line is harder to follow). +If your patch fixes a commit that is already in the repository, please +add a line with "Fixes: ("Fixed commit subject")" below the patch +description / before your "Signed-off-by:" line in the commit message. + +If your patch fixes a bug in the gitlab bug tracker, please add a line +with "Resolves: " to the commit message, too. Gitlab can close bugs +automatically once commits with the "Resolved:" keyword get merged into +the master branch of the project. And if your patch addresses a bug in +another public bug tracker, you can also use a line with "Buglink: " for +reference here, too. + +Example:: + + Fixes: 14055ce53c2d ("s390x/tcg: avoid overflows in time2tod/tod2time") + Resolves: https://gitlab.com/qemu-project/qemu/-/issues/42 + Buglink: https://bugs.launchpad.net/qemu/+bug/1804323`` + +.. _test_your_patches: + +Test your patches +~~~~~~~~~~~~~~~~~ + +Although QEMU has `continuous integration +services <Testing#Continuous_Integration>`__ that attempt to test +patches submitted to the list, it still saves everyone time if you have +already tested that your patch compiles and works. Because QEMU is such +a large project, it's okay to use configure arguments to limit what is +built for faster turnaround during your development time; but it is +still wise to also check that your patches work with a full build before +submitting a series, especially if your changes might have an unintended +effect on other areas of the code you don't normally experiment with. +See `Testing <Testing>`__ for more details on what tests are available. +Also, it is a wise idea to include a testsuite addition as part of your +patches - either to ensure that future changes won't regress your new +feature, or to add a test which exposes the bug that the rest of your +series fixes. Keeping separate commits for the test and the fix allows +reviewers to rebase the test to occur first to prove it catches the +problem, then again to place it last in the series so that bisection +doesn't land on a known-broken state. + .. _submitting_your_patches: Submitting your Patches ----------------------- +.. _if_you_cannot_send_patch_emails: + +If you cannot send patch emails +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +In rare cases it may not be possible to send properly formatted patch +emails. You can use `sourcehut <https://sourcehut.org/>`__ to send your +patches to the QEMU mailing list by following these steps: + +#. Register or sign in to your account +#. Add your SSH public key in `meta \| + keys <https://meta.sr.ht/keys>`__. +#. Publish your git branch using **git push git@git.sr.ht:~USERNAME/qemu + HEAD** +#. Send your patches to the QEMU mailing list using the web-based + ``git-send-email`` UI at https://git.sr.ht/~USERNAME/qemu/send-email + +`This video +<https://spacepub.space/videos/watch/ad258d23-0ac6-488c-83fc-2bacf578de3a>`__ +shows the web-based ``git-send-email`` workflow. Documentation is +available `here +<https://man.sr.ht/git.sr.ht/#sending-patches-upstream>`__. + .. _cc_the_relevant_maintainer: CC the relevant maintainer @@ -223,13 +291,22 @@ use --numbered so the cover and the patch have distinct subject lines). Patches are easier to find if they start a new top-level thread, rather than being buried in-reply-to another existing thread. +.. _avoid_posting_large_binary_blob: + +Avoid posting large binary blob +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If you added binaries to the repository, consider producing the patch +emails using ``git format-patch --no-binary`` and include a link to a +git repository to fetch the original commit. + .. _patch_emails_must_include_a_signed_off_by_line: Patch emails must include a ``Signed-off-by:`` line ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -For more information see `1.12) Sign your work -<http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n296>`__. +For more information see `SubmittingPatches +1.12 <http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297>`__. This is vital or we will not be able to apply your patch! Please use your real name to sign a patch (not an alias or acronym). @@ -246,8 +323,13 @@ that author's Signed-off-by: line is mandatory, with the same spelling. Include a meaningful cover letter ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ -This usually applies only to a series that includes multiple patches; -the cover letter explains the overall goal of such a series. +This is a requirement for any series with multiple patches (as it aids +continuous integration), but optional for an isolated patch. The cover +letter explains the overall goal of such a series, and also provides a +convenient 0/N email for others to reply to the series as a whole. A +one-time setup of ``git config format.coverletter auto`` (refer to +`git-config <http://git-scm.com/docs/git-config>`__) will generate the +cover letter as needed. When reviewers don't know your goal at the start of their review, they may object to early changes that don't make sense until the end of the @@ -288,6 +370,18 @@ it's best to: of the patchset you're looking for review on, and why reviewers should care +.. _consider_whether_your_patch_is_applicable_for_stable: + +Consider whether your patch is applicable for stable +~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ + +If your patch fixes a severe issue or a regression, it may be applicable +for stable. In that case, consider adding ``Cc: qemu-stable@nongnu.org`` +to your patch to notify the stable maintainers. + +For more details on how QEMU's stable process works, refer to +`docs/devel/stable-process.rst <https://git.qemu.org/?p=qemu.git;a=blob_plain;f=docs/devel/stable-process.rst;hb=HEAD>`__. + .. _participating_in_code_review: Participating in Code Review @@ -367,19 +461,19 @@ Include version history in patchset revisions For later versions of patches, include a summary of changes from previous versions, but not in the commit message itself. In an email -formatted as a git patch, the commit message is the part above the "---" +formatted as a git patch, the commit message is the part above the ``---`` line, and this will go into the git changelog when the patch is committed. This part should be a self-contained description of what this version of the patch does, written to make sense to anybody who comes back to look at this commit in git in six months' time. The part below -the "---" line and above the patch proper (git format-patch puts the +the ``---`` line and above the patch proper (git format-patch puts the diffstat here) is a good place to put remarks for people reading the patch email, and this is where the "changes since previous version" -summary belongs. The -`git-publish <https://github.com/stefanha/git-publish>`__ script can -help with tracking a good summary across versions. Also, the -`git-backport-diff <https://github.com/codyprime/git-scripts>`__ script -can help focus reviewers on what changed between revisions. +summary belongs. The `git-publish +<https://github.com/stefanha/git-publish>`__ script can help with +tracking a good summary across versions. Also, the `git-backport-diff +<https://github.com/codyprime/git-scripts>`__ script can help focus +reviewers on what changed between revisions. .. _tips_and_tricks: @@ -411,27 +505,33 @@ If your patch seems to have been ignored If your patchset has received no replies you should "ping" it after a week or two, by sending an email as a reply-to-all to the patch mail, including the word "ping" and ideally also a link to the page for the -patch on -`patchwork <http://patchwork.ozlabs.org/project/qemu-devel/list/>`__ or -GMANE. It's worth double-checking for reasons why your patch might have -been ignored (forgot to CC the maintainer? annoyed people by failing to -respond to review comments on an earlier version?), but often for -less-maintained areas of QEMU patches do just slip through the cracks. -If your ping is also ignored, ping again after another week or so. As -the submitter, you are the person with the most motivation to get your -patch applied, so you have to be persistent. +patch on `patchew <https://patchew.org/QEMU/>`__ or +`lore.kernel.org <https://lore.kernel.org/qemu-devel/>`__. It's worth +double-checking for reasons why your patch might have been ignored +(forgot to CC the maintainer? annoyed people by failing to respond to +review comments on an earlier version?), but often for less-maintained +areas of QEMU patches do just slip through the cracks. If your ping is +also ignored, ping again after another week or so. As the submitter, you +are the person with the most motivation to get your patch applied, so +you have to be persistent. .. _is_my_patch_in: Is my patch in? ~~~~~~~~~~~~~~~ +QEMU has some Continuous Integration machines that try to catch patch +submission problems as soon as possible. `patchew +<http://patchew.org/QEMU/>`__ includes a web interface for tracking the +status of various threads that have been posted to the list, and may +send you an automated mail if it detected a problem with your patch. + Once your patch has had enough review on list, the maintainer for that area of code will send notification to the list that they are including your patch in a particular staging branch. Periodically, the maintainer then sends a `pull request <https://qemu-project.gitlab.io/qemu/devel/submitting-a-pull-request.html>`__ -for aggregating topic branches into mainline qemu. Generally, you do not +for aggregating topic branches into mainline QEMU. Generally, you do not need to send a pull request unless you have contributed enough patches to become a maintainer over a particular section of code. Maintainers may further modify your commit, by resolving simple merge conflicts or
I completely botched up the merged[0] rST conversion of this document by accidentally dropping entire hunks (!) of text. :-( I made it very hard for reviewers to spot it, as the omitted text was buried deep in the document. To fix my hatchet job, I reconverted the "SubmitAPatch" wiki[1] page from scratch and replaced the existing rST with it, while making sure I incorporated previous feedback. In summary, in this reconverted edition: - I did a careful (to the extent my eyes allowed) para-by-para comparison of the wiki and the rST to make sure I didn't omit anything accidentally. - I made sure to work in the cosmetic feedback[2] that Thomas Huth pointed out in the merged (and botched) edition: - fix the hyperlinks in "Split up long patches" - replace ".". with "does not end with a dot" (in "Write a meaningful commit message" section) - replace "---" with ``---`` so that it doesn't render as an em-dash (there were two other occurrences; I fixed those too) - Use "QEMU" spelling consistently in prose usage - Add a consistent "refer to git-config" link where appropriate Thanks to Thomas Huth and Alex Bennée for noticing it on IRC. And sorry for my sloppiness. Fixes: 9f73de8df033 ("docs: rSTify the "SubmitAPatch" wiki") [0] https://gitlab.com/qemu-project/qemu/-/commit/9f73de8df033 [1] https://wiki.qemu.org/index.php?title=Contribute/SubmitAPatch&oldid=10387 [2] https://lists.nongnu.org/archive/html/qemu-devel/2021-11/msg03600.html Signed-off-by: Kashyap Chamarthy <kchamart@redhat.com> --- docs/devel/submitting-a-patch.rst | 198 ++++++++++++++++++++++-------- 1 file changed, 149 insertions(+), 49 deletions(-)