Tarantool development patches archive
 help / color / mirror / Atom feed
* [Tarantool-patches] [PATCH luajit] Add contributing guidelines for Tarantool fork
@ 2023-08-03  9:16 Igor Munkin via Tarantool-patches
  2023-08-03 14:08 ` Sergey Bronnikov via Tarantool-patches
  2023-08-03 20:48 ` Maxim Kokryashkin via Tarantool-patches
  0 siblings, 2 replies; 3+ messages in thread
From: Igor Munkin via Tarantool-patches @ 2023-08-03  9:16 UTC (permalink / raw)
  To: Maxim Kokryashkin, Sergey Kaplun, Sergey Bronnikov; +Cc: tarantool-patches

This changeset describes the particular flow of working on bug in
Tarantool LuaJIT fork. These guidelines also contains how-to for initial
working environment setup and many recomendations regarding commit
message contents.

The location of CONTRIBUING.rst is chosen to overload **all** possible
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.

 .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
+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
+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.
+
+1.  Separate subject from body with a blank line.
+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
+    subsystem name and semicolon:
+
+    * 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
+    (`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.
+
+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**
+
+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:
+
+.. 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:
+
+.. 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):
+
+.. 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::
+
+    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**
+
+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
+
+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'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.
+
+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**
+
+After having sent your patches, you just wait for a review. The reviewer
+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
+favorite mail agent, or use ``--in-reply-to`` option of ``git send-email``
+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:
+
+.. 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
+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


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Add contributing guidelines for Tarantool fork
  2023-08-03  9:16 [Tarantool-patches] [PATCH luajit] Add contributing guidelines for Tarantool fork Igor Munkin via Tarantool-patches
@ 2023-08-03 14:08 ` Sergey Bronnikov via Tarantool-patches
  2023-08-03 20:48 ` Maxim Kokryashkin via Tarantool-patches
  1 sibling, 0 replies; 3+ messages in thread
From: Sergey Bronnikov via Tarantool-patches @ 2023-08-03 14:08 UTC (permalink / raw)
  To: Igor Munkin, Maxim Kokryashkin, Sergey Kaplun; +Cc: tarantool-patches

Hi, Igor!


Thanks for the patch! LGTM


Sergey

On 8/3/23 12:16, Igor Munkin wrote:
> This changeset describes the particular flow of working on bug in
> Tarantool LuaJIT fork. These guidelines also contains how-to for initial
> working environment setup and many recomendations regarding commit
> message contents.
>
> The location of CONTRIBUING.rst is chosen to overload **all** possible
> 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.
>
>   .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
> +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
> +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.
> +
> +1.  Separate subject from body with a blank line.
> +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
> +    subsystem name and semicolon:
> +
> +    * 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
> +    (`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.
> +
> +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**
> +
> +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:
> +
> +.. 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:
> +
> +.. 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):
> +
> +.. 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::
> +
> +    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**
> +
> +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
> +
> +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'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.
> +
> +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**
> +
> +After having sent your patches, you just wait for a review. The reviewer
> +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
> +favorite mail agent, or use ``--in-reply-to`` option of ``git send-email``
> +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:
> +
> +.. 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
> +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/

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [Tarantool-patches] [PATCH luajit] Add contributing guidelines for Tarantool fork
  2023-08-03  9:16 [Tarantool-patches] [PATCH luajit] Add contributing guidelines for Tarantool fork Igor Munkin via Tarantool-patches
  2023-08-03 14:08 ` Sergey Bronnikov via Tarantool-patches
@ 2023-08-03 20:48 ` Maxim Kokryashkin via Tarantool-patches
  1 sibling, 0 replies; 3+ messages in thread
From: Maxim Kokryashkin via Tarantool-patches @ 2023-08-03 20:48 UTC (permalink / raw)
  To: Igor Munkin; +Cc: tarantool-patches

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
> 

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2023-08-03 20:48 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2023-08-03  9:16 [Tarantool-patches] [PATCH luajit] Add contributing guidelines for Tarantool fork Igor Munkin via Tarantool-patches
2023-08-03 14:08 ` Sergey Bronnikov via Tarantool-patches
2023-08-03 20:48 ` Maxim Kokryashkin via Tarantool-patches

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox