The review procedure is supposed to be followed by reviewers. But most importantly it should be followed by the patch author before requesting a review. In order not to waste time of other people on pointing out obvious problems which the author could easily identify by themselves.

• For C code we use Linux Kernel style, the rest is in the developer manual. To verify coding style indent (apt install indent) tool might be used [1]. However code style to use also depends on the file you edit. The file can belong to a third party library with its own code style very different from Tarantool's code style. For Lua code just look at code style in the existing files. There are some most common code style problems:

  • Pay attention to multi-line statements alignment. Especially when edit existing code. See if the changes break the alignment and fix it;
  • In C comments out of functions and inside of functions should be different in how they are started. Everything else is wrong. Below are correct examples. /** comes for documentation comments, /* for local not documented comments. However the difference is vague already, so the rule is simple - out of function = /**, inside = /*.

  /**
   * Out of function comment, option 1.
   */
  
  /** Out of function comment, option 2. */
  
  int
  function()
  {
      /* Comment inside function, option 1. */
  
      /*
       * Comment inside function, option 2.
       */
  }
  

  • Maximal width of a code and a comment line is 80 symbols, including tabs and whitespaces, including indentation;
  • In C we don't apply ! operator to non-boolean values. It means, to check if an integer is not 0, you use != 0. To check if a pointer is not NULL, you use != NULL. The same for ==;
  • In C use tabs, not whitespaces. Tab width should be 8 symbols. In Lua use whitespaces. Indentation step is 4 whitespaces;
  • Remove trailing whitespaces and tabs.

• Check your comments, commit title, and even variable names to be grammatically correct. One of the most typical problems in the patches - typos. Enable a spell checker in your editor, if it has any. Start sentences from a capital letter, end with a dot. Everywhere - in the code, in the tests, in the commit message.

• If a function has declaration and implementation separated, the function comment should be for the declaration. Usually in the header file. Don't duplicate the comment.

• If the patch tries to add a comment with function's parameters and return value description, it should follow Doxygen syntax for that. The most common problems:

  • If you want to mention a function parameter in the comment, use @a <param-name>, not @<param-name>;
  • To describe individual return values and return value in general there are different commands:

  For individual values:
  @retval <value1> Description.
  @retval <value2> Description.
  
  For value in general:
  @return Description.
  

• A comment and the function signature should be synchronized. Double-check if the parameter names are the same as used in the comment, and mean the same. Especially when you change one of them - ensure you changed the other.

• Every patch should have a test except some very rare cases discussed individually. A feature usually have many tests. A bug usually has one, or two.

  • The test shouldn't pass without the patch;
  • The test should not be slow. I.e. it should run less than a second, usually. Exception are tests starting multiple instances, or stress tests;
  • A test for a bugfix should be placed into a separate file whose name conforms to a template gh-####-short-description.test.lua. Where #### is the github issue number covered by this test;
  • A test for a feature may have its own test file, or be added to an existing suitable test file. In case of new file it conforms to the name template feature-name.test.lua. Without issue number.

• The whole test suite should pass on your machine and in CI (not counting old bugs not related to your patch).

• The test shouldn't be flaky. You should be able to run it tens of times in parallel and in a row, it shouldn't fail. Usually for that you need to avoid any fixed timeouts like 'wait for 100ms hoping something in the background will manage to finish work'.

• Commit title should be of a form <subsystem>: <description>. Subsystem is almost always needed. See git log for examples. Because there is no a strict list of subsystem names to use. Description is what the patch does, started from lowercase letter, without a dot in the end, in the imperative mood. It means the titles like Added fix, Bug fixed, Some refactoring are very bad. Commit message should describe reasons why the patch is needed. And optionally, depending on the patch, what was done.

• Width limit for commit title is 50 symbols including subsystem name. Commit message should fit 72 symbols line width. This comes from GitHub UI restrictions, and from general rules of formatting commits in git. For example 72 comes from being able to add 4 whitespaces shift, so as the result message is aligned in the middle of 80. However sometimes 50 is too small, so this is an optional limit. The commit message limit of 72 symbols is not applied when need to copy-paste something long without changes, such as a failed test output.

• If the patch makes user visible changes, such as change of the documented behaviour, or introduces a new public feature, it should include a docbot request. For that mention @TarantoolBot and follow the template. The docbot request should go after commit title, after commit message, and even after Closes Fixed and other GitHub commands. All what is below the docbot request becomes a part of it. Keep in mind that the request will be used to create a GitHub issue, so it is allowed to use markdown here. Sometimes it is necessary - for example, when a code hunk is included into the request. The docbot request should be detailed enough so as not a coder from the documentation team could understand it and convert to text for the site. It won't work if you will write here a couple of useless water sentences as a formal excuse, and will just slow down the review process.

• If you reference old commits in your message using their hash, make sure you also add the referenced commit title in that form so the whole reference looks as follows: abcdehash ("the old commit title"). Such output can be generated via git log command. For example if commit id is 14fa5fd82 then use git log --pretty=format:'%h ("%s")' -1 14fa5fd82.

• In C check, that diag_set()'s are set in all error places. Usually these are OOM errors.

• Use const for parameters, if possible.

• Don't do a change if it is not related to the patch. For example, it may look like a good idea to change style of tens of code lines to make them look "better", along with a 2-line functional patch in the same commit, but actually it is not. It is usually a very bad idea. Such changes make harder to review the patch and to see git blame results in the future. Such unnecessary changes may even look functional, but still if a change is not related to the patch, it shouldn't be done. At least in the same commit as the functional changes. There is no such a rule that 'if something is already done, then why not to commit it'. If you made 1000 lines of random style check fixes, they will be dropped. If you made 1000 lines of refactoring, they will also be dropped if are not needed for anything.

• Commits should be atomic, not possible to split into smaller patches. If you managed to split a problem into several independent tasks, do them in separate commits. Not in one huge commit doing multiple things.

• We use the mailing list to review the patches. They should be sent there using git send-email. GitHub pull requests can be used by external contributors only.

• Git send-email should use --to="[email protected]" or --to="..." and --cc or --to with emails of the people you want to review the patch. If a patch is sent without cc or to, it is likely to be ignored. As a rule, a patch should have 2 reviewers.

• To create a set of mails to send, use git format-patch command. What are the options you can find on the site.

• If the patchset consists of multiple emails, there should be a cover letter. The cover letter should contain the patchset description, difference from the previous version of the patch, GitHub issue and branch links. Not names. Links. On which it would be possible to click.

• If the patchset is a single email, then the branch and issue links, the difference from the previous version, should go after '---' in the patch. The '---' is placed by git format-patch automatically. You need to find it and put your text below.

• For the perfect formatting see create_commits tool.

• If the change is visible to users, there should be a changelog file in changelogs/unreleased folder. Note, this is not only about documented behaviour and features. This is about any observable behaviour, including bug fixes. For examples how to make them read the existing files in this folder and doc/changelogs.md.

• If a patch during the review changes insignificantly, you can send the changes in response to the reviewer's comments in the same email thread. Try to provide separate diff hunks for each fixed comment. Don't merge them into a monolithic diff mess.

• If the patch was reworked significantly during the review, next time it should be sent in a new email thread, with a new version number. For example, [Tarantool-patches] [PATCH v3 1/2] sys: commit title.

• Regardless of how big are the changes, rebase the branch regularly on the latest base branch (usually master).

• If a reviewer asks questions like 'Why ... ?', 'Can it be done differently?' and so on, don't blindly throw a diff changing this place. Usually if a question is asked, it assumes you will answer. Because a reviewer may be wrong. A question does not mean this place should be changed right away.

• Changes, made during the review, should be squashed into the original commits. Don't put fixes into separate commits, it does not simplify anything. The changes should be formatted like git format-patch does - with the original code, with tabs not replaced by whitespaces. For that you can use git --no-pager diff. That will output the raw diff, not altered by tools like less, as when it is created by format-patch.

• If the branch name is changed, don't forget to send a new branch name in one of the emails. Or better in a new email thread with a bumped patchset version. However typically branch name shouldn't be changed at all.

• When you fixed something, ensure you don't make the same mistake again. It is a surprisingly common problem when a review comment needs to be repeated again and again during review of even the same patch. It is the patch author's responsibility to locate and fix the bad places.

There are some tips how to self review. If you don't review your own patches before sending, you will have serious problems during the review for sure. Sending raw inaccurate patches wastes time - your own and the reviewer's. And lowers priority of your patches in the review queue.

For the review and self-review as well it is usually the best to use git diff and git show. These tools show your own incremental changes, and additionally are able to highlight trailing tabs and whitespaces.

When the patch consists of multiple commits, you need to self-review each one, ensure that after each commit the branch stays stable, the tests pass, and the next commits don't override the previous commits in some insane way. To re-play your commits one by one you can use git rebase -i <commit hash before your first commit>. Option -i means interactive. In the appeared menu you change 'patch' to 'edit' or 'e'. Then the commits will be replayed one by one. You will stop after each one, build the sources, run the tests, check all is fine using git show, and git diff, probably apply some changes, and go further using git rebase --continue.

To apply changes to a commit without creating a new one and doing a squash use git commit --amend.

Basically that is the whole process - almost entirely git diff, git show, git rebase, git commit --amend + the review check list.

Self-review may last longer than the patch creation, and it is ok. You may need to look at your commits tens of times, to every changed line, and ask yourself if it is needed, if its purpose is clear, and what questions a reviewer may ask.

At first it may help not to try to pass all the points from the checklist at once. You can try to scan the diff for each point separately. For example, during first self-review be attentive and look for missed diag_set() after an allocation. During the second self-review pass look for inconsistencies between comments and function signatures, and so on.

[1] Indent command to format code. Although the result not necessary will pass the review.

linux $ indent -nbad -bap -nbc -bbo -hnl -br -brs -c33 -cd33 -ncdb -ce \
-ci4 -cli0 -d0 -di1 -nfc1 -i8 -ip0 -l80 -lp -npcs -nprs -sai -saf \
-saw -ncs -nsc -sob -nfca -cp33 -ss -ts8 -il1 <filename>

mac $ indent -nbad -bap -nbc -br -brs -c33 -cd33 -ncdb -ce -ci4 -cli0 \
-d0 -di1 -nfc1 -i8 -ip0 -l80 -lp -npcs -nsc -sob -i8 -sc <filename>