Re: [PATCH] docs: document patch rules

[Juergen Gross <jgross@xxxxxxxx>]

On 03.02.22 10:36, Roger Pau Monné wrote:
> On Wed, Feb 02, 2022 at 12:44:48PM +0100, Juergen Gross wrote:
> > Add a document to describe the rules for sending a proper patch.
> >
> > As it contains all the information already being present in
> > docs/process/tags.pandoc remove that file.
> >
> > Signed-off-by: Juergen Gross <jgross@xxxxxxxx>
> > ---
> >   docs/process/sending-patches.pandoc | 284 ++++++++++++++++++++++++++++
> >   docs/process/tags.pandoc            |  55 ------
> >   2 files changed, 284 insertions(+), 55 deletions(-)
> >   create mode 100644 docs/process/sending-patches.pandoc
> >   delete mode 100644 docs/process/tags.pandoc
> >
> > diff --git a/docs/process/sending-patches.pandoc 
> > b/docs/process/sending-patches.pandoc
> > new file mode 100644
> > index 0000000000..4cfc6e1a5b
> > --- /dev/null
> > +++ b/docs/process/sending-patches.pandoc
> > @@ -0,0 +1,284 @@
> > +# How a proper patch should look like
> > +
> > +This is a brief description how a proper patch for the Xen project should
> > +look like. Examples and tooling tips are not part of this document, those
> > +can be found in the
> > +[Xen Wiki](https://wiki.xenproject.org/wiki/Submitting_Xen_Project_Patches).
> > +
> > +## The patch subject
> > +
> > +The first line at the top of the patch should contain a short description of
> > +what the patch does, and hints as to what code it touches. This line is used
> > +as the **Subject** line of the mail when sending the patch.
> > +
> > +The hint which code is touched us usually in form of a relative path inside
> > +the Xen git repository, where obvious directories can be omitted or replaced
> > +by abbreviations, or it can be a single word describing the topic:
> > +
> > +    <path>: <description>
> I would use <component> maybe instead of path, to explicitly note this
> is not usually a real path inside the repo.
Good idea.

> > +
> > +E.g.:
> > +
> > +    xen/arm: increase memory banks number define value
> > +    tools/libs/evtchn: Deduplicate xenevtchn_fd()
> Mostly a nit, but since this document is about style: I wouldn't
> recommend using a capital letter after ':' by default. The above line
> should instead be:
>
>      tools/libs/evtchn: deduplicate xenevtchn_fd()
Yes.

> > +    MAINTAINERS: update my email address
> > +    build: correct usage comments in Kbuild.include
> > +
> > +The description should give a rough hint *what* is done in the patch.
> > +
> > +The subject line should in general not exceed 80 characters. It must be
> > +followed by a blank line.
> > +
> > +## The commit message
> > +
> > +The commit message is free text describing *why* the patch is done and
> > +*how* the goal of the patch is achieved. A good commit message will describe
> > +the current situation, the desired goal, and the way this goal is being
> > +achieved. Parts of that can be omitted in obvious cases.
> > +
> > +In case additional changes are done in the patch (like e.g. cleanups), those
> > +should be mentioned.
> > +
> > +When referencing other patches (e.g. `patch xy introduced a bug ...`) those
> > +patches should be referenced via their commit id (at least 12 digits) and the
> > +patch subject:
> > +
> > +    Commit 67d01cdb5518 ("x86: infrastructure to allow converting certain
> > +    indirect calls to direct ones") introduced a bug ...
> > +
> > +The following ``git config`` settings can be used to add a pretty format for
> > +outputting the above style in the ``git log`` or ``git show`` commands:
> > +
> > +        [core]
> > +                abbrev = 12
> > +        [pretty]
> > +                fixes = Fixes: %h (\"%s\")
> > +
> > +Lines in the commit message should not exceed 75 characters, except when
> > +copying error output directly into the commit message.
> > +
> > +## Tags
> > +
> > +Tags are entries in the form
> > +
> > +    Tag: something
> > +
> > +In general tags are added in chronological order. So a `Reviewed-by:` tag
> > +should be added **after** the `Signed-off-by:` tag, as the review happened
> > +after the patch was written.
> > +
> > +Do not split a tag across multiple lines, tags are exempt from the
> > +"wrap at 75 columns" rule in order to simplify parsing scripts.
> > +
> > +### Taken-from:
> > +
> > +Xen has inherited some source files from other open source projects. In case
> > +a patch modifying such an inherited file is taken from that project (maybe in
> > +modified form), the `Taken-from:` tag specifies the source of the patch:
> > +
> > +    Taken-from: <repository-URL> <commit-id>
> > +
> > +E.g.:
> > +
> > +    Taken-from: 
> > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git f093b08c47b3
> > +
> > +All tags **above** the `Taken-from:` tag are from the original patch (which
> > +should all be kept), while tags **after** `Taken-from:` are related to the
> > +normal Xen patch process as described here.
> > +
> > +### Fixes:
> > +
> > +If your patch fixes a bug in a specific commit, e.g. you found an issue using
> > +``git bisect``, please use the `Fixes:` tag with the first 12 characters of
> > +the commit id, and the one line summary.
> > +
> > +    Fixes: <commit-id> ("<patch-subject>")
> > +
> > +E.g.:
> > +
> > +    Fixes: 67d01cdb5518 ("x86: infrastructure to allow converting certain indirect 
> > calls to direct ones")
> > +
> > +### Backport:
> > +
> > +A backport tag is an optional tag in the commit message to request a
> > +given commit to be backported to the released trees:
> > +
> > +    Backport: <version> [# <comment>]
> So we already had a documented usage of '#' in tags, which I think
> should make it a better candidate for the R-b scope limiting.
Yes, I found that by surprise.

> > +
> > +E.g.:
> > +
> > +    Backport: 4.9+
> > +
> > +It marks a commit for being a candidate for backports to all released
> > +trees from 4.9 onward.
> > +
> > +The backport requester is expected to specify which currently supported
> > +releases need the backport; but encouraged to specify a release as far
> > +back as possible which applies. If the requester doesn't know the oldest
> > +affected tree, they are encouraged to append a comment like the
> > +following:
> > +
> > +    Backport: 4.9+ # maybe older
> > +
> > +Maintainers request the Backport tag to be added on commit. Contributors
> > +are welcome to mark their patches with the Backport tag when they deem
> > +appropriate. Maintainers will request for it to be removed when that is
> > +not the case.
> > +
> > +Please note that the Backport tag is a **request** for backport, which
> > +will still need to be evaluated by the maintainers. Maintainers might
> > +ask the requester to help with the backporting work if it is not
> > +trivial.
> > +
> > +### Reported-by:
> > +
> > +This optional tag can be used to give credit to someone reporting an issue.
> > +It is in the format:
> > +
> > +    Reported-by: name <email@domain>
> > +
> > +E.g.:
> > +
> > +    Reported-by: Jane Doe <jane.doe@xxxxxxxxxxx>
> > +
> > +As the email address will be made public via git, the reporter of an issue
> > +should be asked whether he/she is fine with being mentioned in the patch.
> > +
> > +### Suggested-by:
> > +
> > +This optional tag can be used to give credit to someone having suggested the
> > +solution the patch is implementing. It is in the format:
> > +
> > +    Suggested-by: name <email@domain>
> > +
> > +E.g.:
> > +
> > +    Suggested-by: Jane Doe <jane.doe@xxxxxxxxxxx>
> > +
> > +As the email address will be made public via git, the reporter of an issue
> > +should be asked whether he/she is fine with being mentioned in the patch.
> > +
> > +### Signed-off-by:
> > +
> > +This mandatory tag specifies the author(s) of a patch (for each author a
> > +separate `Signed-off-by:` tag is needed). It is in the format:
> > +
> > +    Signed-off-by: name <email@domain>
> > +
> > +E.g.:
> > +
> > +    Signed-off-by: Jane Doe <jane.doe@xxxxxxxxxxx>
> > +
> > +The author must be a natural person (not a team or just a company) and the
> > +`Signed-off-by:` tag must include the real name of the author (no pseudonym).
> > +
> > +By signing the patch with her/his name the author explicitly confirms to have
> > +made the contribution conforming to the `Developer's Certificate of Origin`:
> > +
> > +    Developer's Certificate of Origin 1.1
> > +
> > +    By making a contribution to this project, I certify that:
> > +
> > +    (a) The contribution was created in whole or in part by me and I
> > +        have the right to submit it under the open source license
> > +        indicated in the file; or
> > +
> > +    (b) The contribution is based upon previous work that, to the best
> > +        of my knowledge, is covered under an appropriate open source
> > +        license and I have the right under that license to submit that
> > +        work with modifications, whether created in whole or in part
> > +        by me, under the same open source license (unless I am
> > +        permitted to submit under a different license), as indicated
> > +        in the file; or
> > +
> > +    (c) The contribution was provided directly to me by some other
> > +        person who certified (a), (b) or (c) and I have not modified
> > +        it.
> > +
> > +    (d) I understand and agree that this project and the contribution
> > +        are public and that a record of the contribution (including all
> > +        personal information I submit with it, including my sign-off) is
> > +        maintained indefinitely and may be redistributed consistent with
> > +        this project or the open source license(s) involved.
> > +
> > +### Reviewed-by:
> > +
> > +A `Reviewed-by:` tag can only be given by a reviewer of the patch. With
> > +responding to a sent patch adding the `Reviewed-by:` tag the reviewer
> > +(which can be anybody) confirms to have looked thoroughly at the patch and
> > +didn't find any issue (being it technical, legal or formal ones). If the
> > +review is covering only some parts of the patch, those parts can optionally
> > +be specified (multiple areas can be covered with multiple `Reviewed-by:`
> > +tags). It is in the format:
> > +
> > +    Reviewed-by: name <email@domain> [# area]
> > +
> > +E.g.:
> > +
> > +    Reviewed-by: Jane Doe <jane.doe@xxxxxxxxxxx>
> > +    Reviewed-by: Jane Doe <jane.doe@xxxxxxxxxxx> # xen/x86
> I think you should mention in the commit message that we are also
> adding the R-b scope limiting in this commit? The commit message makes
> it look like this is mostly moving the existing Tags into a new
> document.
Same for the "Taken-from:" tag.

Maybe I should even split the patch and add the extension of
"Reviewed-by:" and the new "Taken-from:" as separate patches, in order
to make the addition more clear. Additionally the base document might
go in while we are still discussing the additions.

> > +
> > +In case a patch is being resent an already given `Reviewed-by:` tag can and
> > +should be included, if the patch didn't change the portions of the patch
> > +covered by the tag, or if the reviewer already made clear it would be fine
> > +to make specific changes and no *other* changes have been made.
> > +
> > +### Acked-by:
> > +
> > +Similar to `Reviewed-by:` the `Acked-by:` tag is given by someone having looked
> > +at the patch. The `Acked-by:` tag can only be given by a **maintainer** of the
> > +modified code, and it only covers the code the maintainer is responsible for.
> > +For this reason there is no optional area possible. With the `Acked-by:` tag
> > +the maintainer states, that he/she is fine with the changes in principle, but
> > +didn't do a thorough review. The format is:
> > +
> > +    Acked-by: name <email@domain>
> > +
> > +E.g.:
> > +
> > +    Acked-by: Jane Doe <jane.doe@xxxxxxxxxxx>
> > +
> > +Including the `Acked-by:` tag in a patch is done under the same rules as for
> > +the `Reviewed-by:` tag, with the implied code area the maintainer who gave the
> > +`Acked-by:` tag is responsible for.
> > +
> > +### Tested-by:
> > +
> > +The `Tested-by:` tag is another tag given by someone else. The one giving it
> > +confirms to have tested the patch without finding any functional issues. The
> > +format is:
> > +
> > +    Tested-by: name <email@domain>
> Trailing white space.
Thanks.

> > +
> > +E.g.:
> > +
> > +    Tested-by: Jane Doe <jane.doe@xxxxxxxxxxx>
> > +
> > +Including the `Tested-by:` tag in a patch is done under the same rules as for
> > +the `Reviewed-by:` tag, now limited to the patch not having been modified
> > +regarding code logic (having changed only coding style, comments, or message
> > +texts is fine).
> > +
> > +## Patch version history (change log), further comments
> > +
> > +When sending revised versions of a patch it is good practice to include a
> > +change log after a line containing only `---` (this line will result in the
> > +following text not being included in the commit message). This change log
> > +will help reviewers to spot which parts of the patch have changed. Attributing
> > +changes due to reviewer comments will help the reviewer even more, e.g.:
> > +
> > +    ---
> > +    Changes in V2:
> I would use v2 (lowercase 'v'), because that's how git format-patch
> places the version in the subject line.
Okay.

> > +    - changed function foo() as requested by Jane Doe
> > +    - code style fixed
> > +
> > +In some cases it might be desirable to add some more information for readers
> > +of the patch, like potential enhancements, other possible solutions, etc.,
> > +which should not be part of the commit message. This information can be
> > +added after the `---` line, too.
> > +
> > +## Recipients of the patch
> > +
> > +A patch should always be sent **to** the xen-devel mailing list 
> > <xen-devel@xxxxxxxxxxxxxxxxxxxx> and all maintainers of all touched code areas 
> > should get a
> Missing newline.
Weird I missed that.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature