Re: [PATCH] Add CONTRIBUTING.md

[Yumei Huang <yuhuang@redhat.com>]

Thank you Stefano and David for the suggestions.  Sent V2, please let
me know if I miss anything.

On Mon, Sep 15, 2025 at 2:43 PM Stefano Brivio <sbrivio@redhat.com> wrote:
>
> On top of David's comments:
>
> On Mon, 15 Sep 2025 12:10:54 +1000
> David Gibson <david@gibson.dropbear.id.au> wrote:
>
> > On Fri, Sep 12, 2025 at 03:54:23PM +0800, Yumei Huang wrote:
> > > Signed-off-by: Yumei Huang <yuhuang@redhat.com>
> >
> > Nice first draft, but I think it will need some further work before
> > being ready to merge.  Here are some things that are probably worth adding:
> >
> >
> >  * Incorporate the information in the "Contribute" section
> >    of README.md (and change README to reference this document)
> >  * Code style conventions (kernel net-code)
> >  * The meaning of the "Signed-off-by" line (see kernel docs for full
> >    details or [0] or [1] for some simpler examples
> >  * Reference test/README.md for information on running the testsuite
> >    (that also needs updating)
> >  * Add a copyright and authorship banner (see README.md for an example)
> >
> > A few more minor comments below.
> >
> > [0]
> > https://gitlab.com/dgibson/exeter/-/blob/main/CONTRIBUTING.md?ref_type=heads#developers-certificate-of-origin
> > [1]
> > https://github.com/dgibson/dtc/blob/main/CONTRIBUTING.md#developers-certificate-of-origin
> >
> > > ---
> > >  CONTRIBUTING.md | 86 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 86 insertions(+)
> > >  create mode 100644 CONTRIBUTING.md
> > >
> > > diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md
> > > new file mode 100644
> > > index 0000000..70e35b1
> > > --- /dev/null
> > > +++ b/CONTRIBUTING.md
> > > @@ -0,0 +1,86 @@
> > > +# Contributing to passt
> > > +
> > > +Thank you for your interest in contributing! This document explains how to prepare patches and participate in the email-based review process.
> >
> > By convention we wrap Markdown files at 80 columns, so it's easier to
> > read in text form.
> >
> > > +
> > > +## Workflow
> > > +
> > > +### 1. Clone the project
> > > +
> > > +    git clone git://passt.top/passt
> > > +
> > > +### 2. Create a Branch
> > > +
> > > +    cd passt
> > > +    git checkout -b my-feature-branch
> > > +
> > > +Work on a separate branch instead of committing directly to `master`.
>
> This is not necessary, strictly speaking.
>
> In fact, a bit out of laziness, but also because I'm sometimes rebasing
> while working on more than one feature / issue in parallel, I work on
> the master branch most of the times.
>
> And while it's sometimes out of laziness, I can probably argue that in
> some cases it's actually faster.
>
> I would suggest to word this as a possibility, "You can decide to work
> ...".
>
> > > +
> > > +### 3. Make Changes and Commit
> > > +
> > > +* Edit the source code or documentation.
> > > +
> > > +* Stage your changes:
> > > +
> > > +        git add <file1> <file2> ...
> > > +
> > > +* Commit with a clear message containing `Signed-off-by:` tag:
>
> What is a "clear" message? Almost nobody writes obscure messages on
> purpose, but people might be not aware of what we want in commit
> messages, which is roughly this:
>
>   https://docs.kernel.org/process/submitting-patches.html#describe-your-changes
>
> > > +
> > > +        git commit -s
> > > +
> > > +    Commit message format:
> > > +
> > > +        Subsystem: Brief summary (max ~50 chars)
>
> No, why 50? It's quite hard to do that. 80 is fine, and 80 shouldn't be
> a hard limit here (nice if it fits in a terminal, but sometimes it's
> impossible).
>
> We should make clear what "Subsystem" is, with an example. It's not
> really well defined, by the way, it's a loose "area" (one might want to
> say "tcp:" for tcp_buf.c changes, or maybe "tcp_buf:"... both are fine).
>
> > > +
> > > +        More detailed explanation if needed, wrapped at 72 chars.
> > > +
> > > +        Signed-off-by: Your Name <author@email>
> > > +
> > > +    If there are some references, use "Links: " tag for simplicity.
>
> This is for simplicity over other mechanisms (with Resolves: and
> Bugzilla: and whatnot), but by itself, adding tags is not something one
> does for simplicity.
>
> I'd say "just use Links: tags for anything".
>
> > > +
> > > +### 4. Generate Patches
> > > +
> > > +Use `git format-path` to generate patch(es):
> > > +
> > > +    git format-patch origin/master
> > > +
> > > +This will generate numbered patch files such as 0001-...patch, 0002-...patch, etc.
>
> Or git format-patch -n, that is, if you want to format just three
> patches and not the whole branch (especially if you're working on
> master as I do), you can do git format-patch -3.
>
> > > +
> > > +If you send a series of patches, use the `--cover-letter` option with `git format-patch`:
> > > +
> > > +    git format-patch origin/main --cover-letter
> > > +
> > > +This will generate a cover letter besides your patches. You can edit the cover letter before sending.
> > > +
> > > +### 5. Send Patches
> > > +
> > > +Use `git send-email` to send patches directly to the mailing list:
> > > +
> > > +    git send-email --to=passt-dev@passt.top 000*.patch
>
> Perhaps better to tell people to use '-o' and use a separate directory
> to send patches from.
>
> > > +
> > > +If there are CCs (e.g. maintainers, reviewers), you can add them with `--cc`:
> > > +
> > > +    git send-email --to=passt-dev@passt.top --cc=maintainer@example.com 000*.patch
>
> Actually, I slightly prefer what David is doing and what is (was?)
> commonly done for most Linux kernel subsystems, that is, the maintainer
> (myself) in To:, Cc: or To: list, Cc: others who should know.
>
> That's because if I'm in To:, that's an obvious sign I need to take
> action (review / apply). If I'm on Cc:, that's something I need to be
> aware of, but not necessarily take action myself.
>
> On the other hand, I don't want to have my email address hardcoded
> here, and having a MAINTAINERS file look overkill, so I'm fine with
> this. After all, it's as described at:
>
>   https://passt.top/#contribute
>
> just, well, if you're reading this, here's a tip that can happily
> remain undocumented: add me in To:, and you'll get a slightly faster
> reaction from my side.
>
> > It might be worth mentioning git-publish
> > (https://github.com/stefanha/git-publish) as an easier way of doing
> > steps 4 & 5.
>
> It breaks two things if I recall correctly:
>
> - it adds a spurious line between tags and Signed-off-by:
>
> - it shortens the description of referenced commit with "...", that is,
>   for example,
>
>     Fixes: 0123456789ab ("very long description that never en...")
>
>   and we don't want that (that's how it's done in the kernel).
>
> I planned to send a patch for the first one but couldn't quite
> understand where that comes from so I never took care of that.
>
> > > +### 6. Responding to Review Feedback
> > > +
> > > +* Be open to feedback on both code and documentation.
> > > +
> > > +* Update your patch as needed, and regenerate patches:
> > > +
> > > +        git add <file1> <file2> ...
> > > +        git commit --amend
> > > +        git format-patch -v2 origin/master
> > > +
> > > +* Send the revised patches
> > > +
> > > +        git send-email --to=passt-dev@passt.top v2-000*.patch
> > > +
> > > +### 7. Tips and Best Practices
> > > +
> > > +* Keep changes focused and easy to review.
>
> Maybe add a reference to:
>
>   https://docs.kernel.org/process/submitting-patches.html#split-changes
>
> ...even though we're not really strict about it. The git log is full of
> "While at it..." sentences, and it's fine if it saves effort without
> compromising ease of review.
>
> > > +
> > > +* Test your changes thoroughly.
>
> ...here should come the reference to test/README.md (which should be
> updated first).

I will update test/README.md soon after I manage to run the full tests.
>
> If running tests is too complicated, maybe mention running at least a
> 'make cppcheck' and 'make clang-tidy' other than a specific manual test
> of the functionality / issue at hand.
>
> > > +
> > > +* Include documentation updates if your change affects usage or APIs.
>
> No APIs here.
>
> > > +
> > > +Thank you for helping improve passt! Your contributions make a big difference.
> > > --
> > > 2.47.0
> > >
>
> ...I'll probably have more comments and feel like adding more stuff at
> some point but I can also submit some stuff as a separate patch on top
> of a base you're creating.
>
> --
> Stefano
>


-- 
Thanks,

Yumei Huang