Re: [PATCH] docs: document patch rules

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

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.

> +
> +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()

> +    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.

> +
> +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.

> +
> +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.

> +
> +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.

> +    - 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.

Thanks, Roger.