Re: [PATCH] docs: document patch rules

[Juergen Gross <jgross@xxxxxxxx>]

On 03.02.22 11:07, Jan Beulich wrote:
> On 02.02.2022 12:44, Juergen Gross wrote:
> > --- /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
> Nit: s/ us / is /
>
> > +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>
> > +
> > +E.g.:
> > +
> > +    xen/arm: increase memory banks number define value
> > +    tools/libs/evtchn: Deduplicate xenevtchn_fd()
> > +    MAINTAINERS: update my email address
> > +    build: correct usage comments in Kbuild.include
> I realize there's "usually" in the wording, but I'm still uncertain in how
> far we want to suggest paths here. I have to admit that I never really
> liked overly long prefixes like the "tools/libs/evtchn:" you give as
> example. The prefix should be sufficiently unambiguous, yes, but in this
> particular case "libs/evtchn:" or "libxenevtchn:" would be enough to
> achieve that.
>
> I'd prefer if the tag was described as specifying a (sub-)component (or
> other abstract entity, like is the case for your "build:" example).
Okay.

> > +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 ...
> I think this should have a reference to the Fixes: tag, as generally it
> makes the text less convoluted if it references such a tag rather than
> spelling out hash and title a 2nd time.
I think this depends on the use case. If the cited patch is in the
Fixes: tag I agree. Sometimes a patch is cited for other reasons, e.g.
when adding a fix similar to the one in the cited patch. I always like
to have the commit id in such a case.

Are you fine with me rephrasing the text to:

When referencing other patches (e.g. `similar to patch xy ...`) those
patches should be referenced via their commit id (at least 12 digits)
and the patch subject, if the very same patch isn't referenced by the
`Fixes:` tag, too:

> > +## 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.
> While I don't mind it becoming "Taken-from:", I'd like to put up for
> consideration the (slightly shorter) alternative of "Origin:".
Fine with me.

> > +### 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.
> Besides these two we've also been using Requested-by:, which I think in
> some cases conveys information more precisely than Suggested-by: (e.g.
> when some result was to be achieved without a solution or path there
> having been given).
Will add it.

> > +### 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).
> I'd prefer if the comma separated form was also explicitly mentioned
> (and hence permitted) here. I'd even go as far as suggesting that this
> should be the preferred form as long as line length constraints permit.
OTOH this will make automated parsing harder.

I'm open for both variants, just wanted to mention why I've chosen the
multiline form initially.

> > 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
> > +
> > +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,
> May I suggest to insert "meaningfully" or some such here?
Okay.

> > 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.
> I'd like this to say "normally" or alike. Maintainers may choose to
> restrict their ack to less than what they're listed for, requiring
> remaining areas to gain another maintainer's ack.
Okay, will add that.

> > +## 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
> Nit: Split this line?
>
> > +copy of the mail via **Cc**. In case some other recipients are known to be
> > +interested in the patch, they can be added via **Cc**, too.
> Prior to or alongside "interested" parties, I think we will want to mention
> dedicated reviewers.
Oh, indeed.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature