CodeReview

Code Review in Samba - Mandatory

All Samba code changes written by a Samba Team member must be reviewed by another Samba Team member before they can be delivered. Code from someone outside the team must be reviewed by two Samba Team members. Once the reviewer is happy, a Reviewed-by: tag is added to each git commit.

This Samba Code Review policy outlines the review process in more detail. Samba has a guide for new developers on preparing our patches which includes things like:

  • Each patch should be as small as possible, i.e. changes only one thing. This makes review easier.
  • The patch should have an appropriate commit description.
  • Patches that fix a bug should contain an appropriate BUG tag.

Samba has formal coding guidelines, as well as informal practices that Samba Team Members tend to enforce during review.

policy

The policy we have established for mandatory code code review is this:

  • Each commit in master should have been reviewed by at least two samba team members. For this purpose, involvement in patch creation is considered as reviewing.

commit message tags

There is currently no prescribed way or tool to submit patches for review or mark patches as reviewed. The decisive point is that the patches that end up in master carry notice that they have been reviewed. We achieve this by adding certain tags to the commit messages using the command

git commit --amend

These are the tags we use:

Signed-off-by
The Author(s) of a patch should add their signature to the commit message by adding a "Signed-off-by: " tag.
This tag indicates that the the person it lists was involved in the creation of the patch, that it is fine with the state of the patch and submits it under the license(s) of the affected files.
Author(s) submitting a patch on behalf of an employer should also follow the signing process as described in the Samba Copyright Policy (https://www.samba.org/samba/devel/copyright-policy.html).
The format of the "Signed-off-by:" tag is a line which contains the text "Signed-off-by: ", followed by the full name and email address of the Author. Multiple Authors shall add multiple "Signed-off-by: " lines.
git offers the "-s" or "--signoff" switch for the sub-commands commit, cherry-pick and am (apply-mailbox) which allows you to add the Signed-off-by tag without having to type it.
A sample Signed-off-by message:
Signed-off-by: Author M. Writer <author@example.com>
Reviewed-by
A reviewer should add a "Reviewed-by: " tag if she was not involved in the creation of the patch.
There is currently no git switch smilar to "--signoff", and according to git developers, there won't be. So we have to add the review tags manually.

These tags are inspired by the use in the Linux kernel, but note that they don't always have the same meaning and/or significance in Samba that they have for Linux -- particularly with regard to the DCO. See the Linux website about submitting patches for detailed explanations on how they use these tags.

The Linux also makes use of an "Acked-by: " tag, but I suggest that we don't use it for now to keep things simple, since it does not make a lot of sense, unless we have established maintainers all over the code base and those need to Ack patches for their areas.

If the patch was created by multiple authors, they can optionally add a "Pair-programmed-with: " tag to the commit messages. This is redundant though.

Code Review in Practice

Samba’s coding style (for C) is documented in the README.Coding.md. Adherence to this is encouraged by peer-review and git hooks. The coding guidelines include:

  • Purely stylistic conventions, such as how to format code comments.
  • Sensible programming strategies, such as always initializing pointer variables to NULL.
  • A mixture of both, such as 80-character line lengths, which discourages writing complex, deeply-nested code.

The coding standards are applied to new code that is delivered. However, because the coding standards have evolved over time, existing code that was written a long time ago may not conform to the guidelines.

As well as the C guidelines, the following are used for other languages:

  • Python. The Python coding standard follows PEP8. This is partially enforced by make test. Python code should be compatible with both Python 2 and Python 3.
  • Shell. Samba aims to operate on a POSIX shell as /bin/sh and must not assume /bin/sh is BASH. Specifically Debian and Ubuntu use dash as /bin/sh. If BASH is required for some reason, then #!/bin/bash must be used.

Samba also has a Copyright Policy that requires mandatory tags on every git commit.

Informal coding guidelines

As well as the formal coding standard, Samba Team Members tend to enforce informal, common sense practices during review. For example, the coding guidelines don’t dictate specific naming conventions, but most Samba Team reviewers would suggest existing naming conventions are followed. The general convention is that commonly-used variables just use an abbreviated form of the structure name, such as:

struct ldb_result *res = NULL;
struct ldb_message *msg = NULL;
TALLOC_CTX *mem_ctx = NULL;
struct loadparm_context *lp_ctx = NULL

Another example would be a patch-set where new code is introduced, and then the code is further reworked in a subsequent patch. Reviewers would generally request that the patch-set is redone to avoid unnecessary code churn.

Samba also has an informal (but enforced) policy that code changes and new features should have automated tests. Once the new feature, along with its tests, is integrated into the main codebase, Samba’s autobuild process ensures that future changes cannot break these tests and degrade the new feature’s functionality.

Tools and Scripts

A number of developers use various tools, scripts, hacks, and shortcuts to ease the mechanics of the review process.

Any patches that do large-scale change to code generation tools, #define in headers or configure checks in Waf need to be checked carefully following this advice and use the gcc-E tool.

Where to submit for review

Please follow the Samba Contribution instructions to submit your code for review.

Reviewer's action

How should a reviewer act on a submitted patch?

  1. Apply the patchset to a local checkout of the master branch. This can be done by copying the patch file and doing "git am" or by direclty pulling the submitters git repository if available.
  2. Review the patches (see below for hints), do all the builds and test that seem appropriate and necessary.
  3. If the reviewer is OK with the patch(es) she should add the "Reviewed-by: " tag to the commit message(s) and either push the patches to master via autobuild or git format-patch the adapted patches and send them back to the author.
  4. If the reviewer is not OK with the patch(es), she should send comments to the author and possibly wait for the next iteration of this process.

How To Review Code

(by metze, move to own page?)

I think everybody can learn how to do good code review and I think the most important thing is that everybody reviews his/her own patches in the same way one would review patches from others!

Here are some guidelines for a successful review process:

  • First read the commit message. Does that give you the impression the change is in anyway complex? If so do, a "careful_mode++".
  • Read the diffstat. Does this change touches any critical areas of code? If so, do a "careful_mode++".
  • First read the patch hunk by hunk and watch for Coding-Style issues. If there are strange things, do a "careful_mode++".
  • Read the patch again as whole, watch for changes which could possibly change the blackbox behavior of a function.
  • Depending on the "careful_mode", you should try to fully understand the change.
  • Note: Here it's sometimes useful to use the -U<contextlines> feature of git show/diff, so that you see the changes in the full context of the whole function.
  • If you found a change in the behavior, set "very_careful_mode = true".
  • Ask yourself why a change is needed! If you don't see why it's needed, set "very_careful_mode = true".
  • If you find things you don't understand, set "verfy_careful_mode = true".
  • If very_careful_mode != true you may be done. This happens most of the time and most of the time careful_mode is also 0. And the costs are not too high.
  • Maybe ask other people for review. Trigger discussions on the samba-technical@samba.org mailing list.
  • Read the related sources.
  • Use git grep to find callers of a function.
  • Do manual runtime tests.

Review your own code!

I think the most important thing is that everybody reviews his/her own patches in the same way you would review patches from others!

How to work with code review

Code Review is like a game of tennis: You have to keep hitting the ball back over the net to stay in the game.

The analogy breaks down a bit, but my advise is always if you think you are waiting on someone, check you don't have the ball:

  • go back over their reviews and ensure you have done everything they asked
  • read over the patches as submitted with a clear head the next day
  • if everything really has been addressed, look wider and ensure that advise has been applied generally.
  • if everything really is in order, ask whoever has been reviewing for advise on what to do next. It is a friendly way of saying ping.

Understand that sometimes folks are just busy, so help them out if you have the skills in something they are working on. Code review, indeed open source development in general is a social activity, it helps to do it with friends.

If it feels like a grudge match, then step back and re-evaluate. You and your reviewer are both probably getting really annoyed so double- check that this approach is worthwhile. Sometimes an alternate approach might be needed, but other times it is just about the need to do things right.

Finally, we all do just get busy, have other non-Samba tasks, take leave etc. Sadly this leaves too few folks to review many 'almost but not quite ready' patches, so do whatever you can to ensure your code is a slam-dunk when the time comes.