Re: [Tarantool-patches] [PATCH luajit] Add contributing guidelines for Tarantool fork

[Maxim Kokryashkin via Tarantool-patches <tarantool-patches@dev.tarantool.org>]

Hi, Igor!
Thanks for the guidelines!

On Thu, Aug 03, 2023 at 09:16:07AM +0000, Igor Munkin wrote:
> This changeset describes the particular flow of working on bug in
Typo: s/on bug/on a bug/
> Tarantool LuaJIT fork. These guidelines also contains how-to for initial
Typo: s/Tarantool/the Tarantool/
Typo: s/also contains/also contain a/
> working environment setup and many recomendations regarding commit
Typo: s/recomendations/recommendations/
> message contents.
Typo: s/regarding commit message contents/regarding the contents of commit messages/
> 
> The location of CONTRIBUING.rst is chosen to overload **all** possible
Typo: s/CONTRIBUING.rst/CONTRIBUTING.rst/
> alternatives (i.e. repository root, docs/ directory, etc). For more
> info, see GitHub documentation[1].
> 
> [1]: https://docs.github.com/en/communities/setting-up-your-project-for-healthy-contributions/setting-guidelines-for-repository-contributors#adding-a-contributing-file
> 
> Signed-off-by: Igor Munkin <imun@tarantool.org>
> ---
> 
> Branch: https://github.com/tarantool/luajit/tree/imun/contribution-guidelines
> 
> Comments and enhancements are very welcome! I've seen that NOTE section
> is misrendered by GitHub, so I have no strong opinion regarding its
> usage in scope of the contributing guide.

Some general thoughts: 
- I thinks we should add something about checking the spelling, gramma, 
  phrasing and abscence of profanity in the patch.
- I completely understand that this guide is targeted at an imaginary
  external contributor, but it's unlikely someone other then a member of
  our team will do something. So maybe we should add something about our CI?
- Also, it is important to mention, that the whole test set must be executed
  before sending the patch anywhere. 
- Not sure about the "favorite mailing client" thing. For example, I used to
  send my reviews via the web interface, which resulted in incorrectly displayed
  interleaved comments for everyone else. So, IMO, at least a list of some good
  options (Thunderbird, [neo]mutt, etc.) should be provided.
> 
>  .github/CONTRIBUTING.rst | 337 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 337 insertions(+)
>  create mode 100644 .github/CONTRIBUTING.rst
> 
> diff --git a/.github/CONTRIBUTING.rst b/.github/CONTRIBUTING.rst
> new file mode 100644
> index 00000000..70012468
> --- /dev/null
> +++ b/.github/CONTRIBUTING.rst
> @@ -0,0 +1,337 @@
> +.. _developer_guidelines:
> +
> +-------------------------------------------------------------------------------
> +Developer guidelines
> +-------------------------------------------------------------------------------
> +
> +.. _dev_guidelines-work_on_a_bug:
> +
> +===========================================================
> +How to work on a bug
> +===========================================================
> +
> +Any defect, even minor, if it changes the user-visible behavior, needs a bug
Typo: s/even minor/even a minor one/
> +report. Report a bug at https://github.com/tarantool/tarantool/issues.
> +
> +When reporting a bug, try to come up with a test case that can be reproduced
> +with LuaJIT. Set the ``luajit`` label and label with LuaJIT subsystem affected
Typo: s/with/with the/
> +if possible (e.g. ``memprof``) and the target branch for the bug fix. Assign
> +the bug to yourself. Put the status to ``'In progress'``. Once the patch is
> +ready, push it to your remote branch, wait for CI and fix all problems, if any
> +occur. If there are test fails that look irrelevant to the changes, highlight
> +this fact while emailing the patch. When CI is green, send the patch to the
> +reviewers and solicit a review for the fix.
> +
> +Once there is a positive code review, push the patch and set the status to
> +``'Done'``.
> +
> +Patches for bugs should contain a reference to the respective GitHub issue.
> +Each patch should have a test, unless coming up with one is difficult in the
> +current framework, in which case QA should be alerted.
> +
> +Don't forget to delete the remote branch, when your patch makes it into the
> +master.
> +
> +.. _dev_guidelines-commit_message:
> +
> +===========================================================
> +How to write a commit message
> +===========================================================
> +
> +Any commit needs a helpful message. Mind the following guidelines when
> +committing to any of Tarantool repositories at GitHub.
Typo: s/of/of the/
> +
> +1.  Separate subject from body with a blank line.
Typo: s/subject from body/the subject from the body/
> +2.  Try to limit the subject line to **50 characters** or so.
> +3.  Start the subject line with a capital letter unless it prefixed with a
Typo: s/it/it is/
> +    subsystem name and semicolon:
Typo: s/and/and a/
> +
> +    * build:
> +    * ci:
> +    * cmake:
> +    * core:
> +    * gdb:
> +    * jit:
> +    * lldb:
> +    * memprof:
> +    * misc:
> +    * sysprof:
> +    * test:
> +    * tools:
> +    * vm:
> +
> +4.  Do not end the subject line with a period.
> +5.  Do not put "gh-xx", "closes #xxx" to the subject line.
> +6.  Use the imperative mood in the subject line.
> +    A properly formed Git commit subject line should always be able to complete
> +    the following sentence: "If applied, this commit will
> +    */your subject line here/*".
> +7.  Wrap the body to **72 characters** or so.
> +8.  Use the body to explain **what and why** vs. how.
> +9.  Link GitHub issues on the lasts lines
Typo: s/lasts/last/
> +    (`see how <https://help.github.com/articles/closing-issues-via-commit-messages>`_).
> +10. Use your real name and real email address.
> +    For Tarantool team members, **@tarantool.org** email is preferred, but not
> +    mandatory.

Side note: I believe you should also specify, that some of those requirements
can be ignored for backported patches.
> +
> +A template:
> +
> +.. code-block:: none
> +
> +    Summarize changes in 50 characters or less
> +
> +    More detailed explanatory text, if necessary.
> +    Wrap it to 72 characters or so.
> +    In some contexts, the first line is treated as the subject of the
> +    commit, and the rest of the text as the body.
> +    The blank line separating the summary from the body is critical
> +    (unless you omit the body entirely); various tools like `log`,
> +    `shortlog` and `rebase` can get confused if you run the two together.
> +
> +    Explain the problem that this commit is solving. Focus on why you
> +    are making this change as opposed to how (the code explains that).
> +    Are there side effects or other unintuitive consequences of this
> +    change? Here's the place to explain them.
> +
> +    Further paragraphs come after blank lines.
> +
> +    * Bullet points are okay, too.
> +
> +    * Typically an asterisk or hyphen is used for the bullet, preceded
> +      by a single space, with blank lines in between, but conventions
> +      vary here.
> +
> +    Fixes tarantool/tarantool#123
> +    Closes tarantool/tarantool#456
> +    Needed for tarantool/tarantool#859
> +    See also tarantool/tarantool#343, tarantool/tarantool#789
> +
> +Some real-world examples:
> +
> +* `tarantool/luajit@3a2e484 <https://github.com/tarantool/luajit/commit/3a2e484>`_
> +* `tarantool/luajit@475359b <https://github.com/tarantool/luajit/commit/475359b>`_
> +* `tarantool/luajit@47f5383 <https://github.com/tarantool/luajit/commit/47f5383>`_
> +* `tarantool/luajit@4f4fd9e <https://github.com/tarantool/luajit/commit/4f4fd9e>`_
> +* `tarantool/luajit@7570ff6 <https://github.com/tarantool/luajit/commit/7570ff6>`_
> +* `tarantool/luajit@814625f <https://github.com/tarantool/luajit/commit/814625f>`_
> +* `tarantool/luajit@88d2600 <https://github.com/tarantool/luajit/commit/88d2600>`_
> +* `tarantool/luajit@9d78aa1 <https://github.com/tarantool/luajit/commit/9d78aa1>`_
> +* `tarantool/luajit@a0483bd <https://github.com/tarantool/luajit/commit/a0483bd>`_
> +* `tarantool/luajit@fd3f061 <https://github.com/tarantool/luajit/commit/fd3f061>`_
> +
> +Based on [1_] and [2_].
> +
> +.. _dev_guidelines-patch-review:
> +
> +===========================================================
> +How to submit a patch for review
> +===========================================================
> +
> +We don't accept GitHub pull requests. Instead, all patches should be sent as
> +plain-text messages to tarantool-patches@dev.tarantool.org. Please subscribe
> +to our mailing list at https://lists.tarantool.org/tarantool-patches to ensure
> +that your messages are added to the archive.
> +
> +1. **Preparing a patch**
Nit: Further contents of this bullet are under-indented on the GitHun. I'd be nice 
to fix it or just make it a subsection, but feel free to ignore.
> +
> +Once you have committed a patch to your local git repository, you can
> +submit it for review.
> +
> +To prepare an email, use ``git format-patch`` command:
Typo: s/use/use the/
> +
> +.. code-block:: console
> +
> +    $ git format-patch -1 --subject-prefix='PATCH luajit'
> +
> +It will format the commit at the top of your local git repository as
> +a plain-text email and write it to a file in the current directory.
> +The file name will look like ``0001-your-commit-subject-line.patch``.
> +To specify a different directory, use ``-o`` option:
Typo: s/use/use the/
> +
> +.. code-block:: console
> +
> +    $ git format-patch -1 --subject-prefix='PATCH luajit' -o ~/patches-to-send
> +
> +Once the patch has been formatted, you can view and edit it with your favorite
> +text editor (after all, it is a plain-text file!). We strongly recommend
> +adding:
> +
> +* a hyperlink to the branch where this patch can be found at GitHub, and
> +* a hyperlink to the GitHub issue your patch is supposed to fix, if any.
> +
> +If there is just one patch, the change log should go right after ``---`` in the
> +message body (it will be ignored by ``git am`` then).
> +
> +If there are multiple patches you want to submit in one go (e.g. this is
> +a big feature which requires some preparatory patches to be committed
> +first), you should send each patch in a separate email in reply to a cover
> +letter. To format a patch series accordingly, pass the following options
> +to ``git format-patch``:
> +
> +.. code-block:: console
> +
> +    $ git format-patch --cover-letter --thread=shallow HEAD~2 --subject-prefix='PATCH luajit'
> +
> +where:
> +
> +* ``--cover-letter`` will make ``git format-patch`` generate a cover letter;
> +* ``--thread=shallow`` will mark each formatted patch email to be sent
> +  in reply to the cover letter;
> +* ``HEAD~2`` (we now use it instead of ``-1``) will make ``git format-patch``
> +  format the first two patches at the top of your local git branch instead
> +  of just one. To format three patches, use ``HEAD~3``, and so forth.
> +
> +After the command has been successfully executed, you will find all your
> +patches formatted as separate emails in your current directory (or in the
> +directory specified via ``-o`` option):
Typo: s/via/via the/
> +
> +.. code-block:: none
> +
> +    0000-cover-letter.patch
> +    0001-first-commit.patch
> +    0002-second-commit.patch
> +    ...
> +
> +The cover letter will have BLURB in its subject and body. You'll have to
> +edit it before submitting (again, it is a plain text file). Please write:
> +
> +* a short series description in the subject line;
> +* a few words about each patch of the series in the body.
> +
> +And don't forget to add hyperlinks to the GitHub issue and branch where
> +your series can be found. In this case you don't need to put links or any
> +additional information to each individual email -- the cover letter will
> +cover everything.
> +
> +.. NOTE::
Nit: Maybe just a bold text will do?
> +
> +    To omit ``--subject-prefix='PATCH luajit'``, ``--cover-letter`` and
> +    ``--thread=shallow`` options, you can add the following lines to
> +    your gitconfig:
> +
> +    .. code-block:: none
> +
> +        [format]
> +            thread = shallow
> +            coverLetter = auto
> +            subjectPrefix = PATCH luajit
> +
> +2. **Sending a patch**
Same as for the bullet above.
> +
> +Once you have formatted your patches, they are ready to be sent via email.
> +Of course, you can send them with your favorite mail agent, but it is
> +much easier to use ``git send-email`` for this. Before using this command,
> +you need to configure it.
> +
> +If you use a GMail account, add the following code to your ``.gitconfig``:
> +
> +.. code-block:: none
> +
> +    [sendemail]
> +        smtpencryption = tls
> +        smtpserver = smtp.gmail.com
> +        smtpserverport = 587
> +        smtpuser = your.name@gmail.com
> +        smtppass = topsecret
> +
> +For mail.ru users, the configuration will be slightly different:
> +
> +.. code-block:: none
> +
> +    [sendemail]
> +        smtpencryption = ssl
> +        smtpserver = smtp.mail.ru
> +        smtpserverport = 465
> +        smtpuser = your.name@mail.ru
> +        smtppass = topsecret

Side note: Let's go in and out. Quick, 20-minute adventure.
> +
> +If your email account is hosted by another service, consult your service
> +provider about your SMTP settings.
> +
> +Once configured, use the following command to send your patches:
> +
> +.. code-block:: console
> +
> +    $ git send-email --to tarantool-patches@dev.tarantool.org 00*
> +
> +(``00*`` wildcard will be expanded by your shell to the list of patches
> +generated at the previous step.)
> +
> +If you want someone in particular to review your patch, add them to the
> +list of recipients by passing ``--to`` or ``--cc`` once per each recipient.
It is not necessary to do that for each recipient. You can just do this:
| --to=tarantool-patches@dev.tarantool.org,colleague1@tarantool.org,colleague2@tarantool.org

Also, I guess that 'if you want someone in particular to review your patch' is kind of
misleading. I doubt anyone would review a patch, that was not CC'ed to them.

> +It's worth mentioning that both ``--to`` and ``--cc`` can be added on
> +``git-format-patch`` step or even added to the config (it's quite convenient
> +to omit ``--cc tarantool-patches@dev.tarantool.org``).
> +
> +.. NOTE::
> +
> +    It is useful to check that ``git send-email`` will work as expected
> +    without sending anything to the world. Use ``--dry-run`` option for that.

Side note: Maybe it is worth mentioning that for the backported patches, Mike's
stub <mike> should be dropped during the sending process from the list of recipients.
> +
> +We also use git trailers in our commit messages to provide the knowledge
> +about the authors and other fellows forcing the patch to appear in the
> +trunk. Sometimes these guys are not actively involved in the process, so
> +to avoid sending spam to any of the mentioned person just add
> +``--suppress-cc=misc-by`` option to the command.
> +
> +3. **Review process**
Same as for the bullet above.
> +
> +After having sent your patches, you just wait for a review. The reviewer
Typo: s/having sent/sending/
> +will send their comments back to you in reply to the email that contains
> +the patch that in their opinion needs to be fixed.
> +
> +Upon receiving an email with review remarks, you carefully read it and reply
> +about whether you agree or disagree with. Please note that we use the
> +interleaved reply style (aka "inline reply") for communications over email.
> +
> +Upon reaching an agreement, you send a fixed patch in reply to the email that
> +ended the discussion. To send a patch, you can either attach a plain diff
> +(created by ``git diff`` or ``git format-patch``) to email and send it with your
Typo: s/to email/to your email/
> +favorite mail agent, or use ``--in-reply-to`` option of ``git send-email``
Typo: s/use/use the/
Typo: s/of/of the/
> +command.
> +
> +If you feel that the accumulated change set is large enough to send the
> +whole series anew and restart the review process in a different thread,
> +you generate the patch email(s) again with ``git format-patch``, this time
> +adding v2 (then v3, v4, and so forth) to the subject and a change log to
> +the message body. To modify the subject line accordingly, use the
> +``--subject-prefix`` option to ``git format-patch`` command:
Typo: s/to/to the/
> +
> +.. code-block:: console
> +
> +    $ git format-patch -1 --subject-prefix='PATCH luajit' --reroll-count=2
> +
> +To add a change log, open the generated email with you favorite text
Typo: s/you/your/
> +editor and edit the message body. If there is just one patch, the change
> +log should go right after ``---`` in the message body (it will be ignored
> +by ``git am`` then). If there is more than one patch, the change log should
> +be added to the cover letter. Here is an example of a good change log:
> +
> +.. code-block:: console
> +
> +    Changes in v3:
> +      - Fixed comments as per review by Alice
> +      - Added more tests
> +    Changes in v2:
> +      - Fixed a crash if the user passes invalid options
> +      - Fixed a memory leak at exit
> +
> +It is also a good practice to add a reference to the previous version of
> +your patch set (via a hyperlink or message id).
> +
> +.. NOTE::
> +
> +    * Do not disagree with the reviewer without providing a good argument
> +      supporting your point of view.
> +    * Do not take every word the reviewer says for granted. Reviewers are
> +      humans too, hence fallible.
> +    * Do not expect that the reviewer will tell you how to do your thing.
> +      It is not their job. The reviewer might suggest alternative ways to
> +      tackle the problem, but in general it is your responsibility.
> +    * Do not forget to update your remote git branch every time you send a
> +      new version of your patch.
> +    * Do follow the guidelines above. If you do not comply, your patches are
> +      likely to be silently ignored.
> +
> +.. _1: https://git-scm.com/book/en/v2/Distributed-Git-Contributing-to-a-Project
> +.. _2: https://chris.beams.io/posts/git-commit/
> -- 
> 2.30.2
>