From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from [87.239.111.99] (localhost [127.0.0.1]) by dev.tarantool.org (Postfix) with ESMTP id 6EC7058EA56; Thu, 3 Aug 2023 23:48:31 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org 6EC7058EA56 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=tarantool.org; s=dev; t=1691095711; bh=4UtXwEd/ynaKp5g/7wd2Iav3zPtuKAVot0jr0wOt5ME=; h=Date:To:References:In-Reply-To:Subject:List-Id:List-Unsubscribe: List-Archive:List-Post:List-Help:List-Subscribe:From:Reply-To:Cc: From; b=QfmfWt2s7wUZ5nZeQeSA9Q6u0hFhRG2LYYTixRMxb478NHiVGxLRreu/8S2kzWvgO IZsU+zC87My5KH6/Lvy5/SnLhAfg7qG5lrPhYxZRoeRfDtlKAYa7KiamITCdhk9DBj 9t70Voqn/djDezAghvwkr2FE+iwr/jIse6fUaJtc= Received: from smtp52.i.mail.ru (smtp52.i.mail.ru [95.163.41.88]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id A98EE589AFD for ; Thu, 3 Aug 2023 23:48:30 +0300 (MSK) DKIM-Filter: OpenDKIM Filter v2.11.0 dev.tarantool.org A98EE589AFD Received: by smtp52.i.mail.ru with esmtpa (envelope-from ) id 1qRfFF-00HA4E-1p; Thu, 03 Aug 2023 23:48:30 +0300 Date: Thu, 3 Aug 2023 23:48:04 +0300 To: Igor Munkin Message-ID: References: <0b6fee103c3d00eb92b8f997b31fae23d0fb8d52.1691053948.git.imun@tarantool.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0b6fee103c3d00eb92b8f997b31fae23d0fb8d52.1691053948.git.imun@tarantool.org> X-Mailru-Src: smtp X-4EC0790: 10 X-7564579A: 646B95376F6C166E X-77F55803: 4F1203BC0FB41BD969E04B5EED670DC836E8CADCCFC7EB90264FE23C7F795239182A05F538085040960FDEC8AFB85EFA87EC23CEDB6A762CDED4813F912CBD996EF15259A0E05490 X-7FA49CB5: FF5795518A3D127A4AD6D5ED66289B5278DA827A17800CE76042E2DB3E33BF2BEA1F7E6F0F101C67BD4B6F7A4D31EC0BCC500DACC3FED6E28638F802B75D45FF8AA50765F79006370003D0394D5A38D88638F802B75D45FF36EB9D2243A4F8B5A6FCA7DBDB1FC311F39EFFDF887939037866D6147AF826D83365501C52763C8C45407577E18DC4ED117882F4460429724CE54428C33FAD305F5C1EE8F4F765FCAE9A1BBD95851C5BA471835C12D1D9774AD6D5ED66289B52BA9C0B312567BB23117882F4460429728776938767073520C65AC60A1F0286FE618001F51B5FD3F9D2E47CDBA5A96583BA9C0B312567BB2376E601842F6C81A19E625A9149C048EE4B6963042765DA4BC0837EA9F3D197644AD6D5ED66289B523666184CF4C3C14F6136E347CC761E07725E5C173C3A84C32739D626790C831376E601842F6C81A1F004C906525384303E02D724532EE2C3F43C7A68FF6260569E8FC8737B5C2249EC8D19AE6D49635B68655334FD4449CB33AC447995A7AD18A91E23F1B6B78B78D5E8D9A59859A8B6300D3B61E77C8D3B089D37D7C0E48F6C5571747095F342E88FB05168BE4CE3AF X-C1DE0DAB: 0D63561A33F958A5EA8D4D2799BCC1049A3C0656209118C1345A3CAFE968DDF4F87CCE6106E1FC07E67D4AC08A07B9B02A336C6518635091CB5012B2E24CD356 X-C8649E89: 1C3962B70DF3F0ADBF74143AD284FC7177DD89D51EBB7742424CF958EAFF5D571004E42C50DC4CA955A7F0CF078B5EC49A30900B95165D348D68DCC07DD06FF5709F19EA65D6C8BC8ADB9749E7CE8217C10169BB02BABD48DA25A27EE98822D71D7E09C32AA3244C31D6CA4A41FFAB213AC7024A5841D0608894E9C85370243EBAD658CF5C8AB4025DA084F8E80FEBD396F07DFE06A4A8314E894E437E78228B66933FA05BD8EF0CAD958392AE682691 X-D57D3AED: 3ZO7eAau8CL7WIMRKs4sN3D3tLDjz0dLbV79QFUyzQ2Ujvy7cMT6pYYqY16iZVKkSc3dCLJ7zSJH7+u4VD18S7Vl4ZUrpaVfd2+vE6kuoey4m4VkSEu530nj6fImhcD4MUrOEAnl0W826KZ9Q+tr5ycPtXkTV4k65bRjmOUUP8cvGozZ33TWg5HZplvhhXbhDGzqmQDTd6OAevLeAnq3Ra9uf7zvY2zzsIhlcp/Y7m53TZgf2aB4JOg4gkr2biojcir52QaMQ81qkJbxy3Mybg== X-Mailru-Sender: 7940E2A4EB16C9974CFF14AB62FAC1505817BCA77E0BDC880A56374087EA81B1E2527C969975515CFF9FCECFB8D89CB6C77752E0C033A69E235A20A81F3B0E39AB3C5F247CB2F7F93A5DB60FBEB33A8A0DA7A0AF5A3A8387 X-Mras: Ok Subject: Re: [Tarantool-patches] [PATCH luajit] Add contributing guidelines for Tarantool fork X-BeenThere: tarantool-patches@dev.tarantool.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , From: Maxim Kokryashkin via Tarantool-patches Reply-To: Maxim Kokryashkin Cc: tarantool-patches@dev.tarantool.org Errors-To: tarantool-patches-bounces@dev.tarantool.org Sender: "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 > --- > > 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 `_). > +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 `_ > +* `tarantool/luajit@475359b `_ > +* `tarantool/luajit@47f5383 `_ > +* `tarantool/luajit@4f4fd9e `_ > +* `tarantool/luajit@7570ff6 `_ > +* `tarantool/luajit@814625f `_ > +* `tarantool/luajit@88d2600 `_ > +* `tarantool/luajit@9d78aa1 `_ > +* `tarantool/luajit@a0483bd `_ > +* `tarantool/luajit@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 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 >