Suggest to run --record for mysql-test-run for anything that breaks.
Disable Tests within a Bug Fix.
Both Fix bugs and clean up code in the same patch.
Ignore new code that you don’t understand during a review.
Discourage developer from writing test to prove that the bug was fixed.
Prefer changes that require huge changes to all tests, they were probably broken.
Never ask from help from the developer who wrote the original code.
Methodology
Read bug report, find out what user’s problem was, and read the description of the developer’s fix. Read also comments from other developers to verify if the bug really is a bug or if there is a better fix.
Scan patch for test case, compare to user’s problem.
Look to see if patch was minimal change (read comments in commits on file to see if “additional” fixes were done).
Look to see if all new variables that are allocated are properly de’allocated.
Look for orphaned variables/functions.
Check that all return values are properly handled, or declared VOID()
Look at all lock additions and changes in mutex. Make sure lock order is preserved (release first what you last locked).
Make sure that “bk collapse” has been used so that only one changeset is pushed.
Reviewer responsibilities
Reviewer need to understand that he is responsible for the quality of the patch
Reviewer should understand every single change made
In doubt, require more comments
There should be a test case that ensures that the reported bug is fixed
For a production/beta code review, strive for minimum changes
If old test results changes, discuss this with another senior developer
In doubt of solution, discuss things with a senior developer
If you are first reviewer, read comments from second reviewer
Tools to use
For non trivial changes, require that patch is tested with valgrind
All new code should be tested; In doubt, require that everything is tested with dgcov and that purecov comments are used.
Things to watch out for
Memory allocation (Malloc is generally bad); Ensure that all memory is initialized and freed.
Question all ‘cast’
Check code for portability issues (Wrong functions, alignment, storage of numbers etc)
Ensure that coding standard from internals.texi is followed
Ensure that all syntax, API and behavior changes are compatible with previous MySQL production releases (when in doubt check with documentation)
There should be no compiler warnings
Ensure that new code and change set is properly commented
Other things
If there is a better solution (for a future MySQL release), reviewer or implementor should write it into worklog
Require comments on all blocks of code, and changesets.
Look for regression.
Fix the root cause of the bug.
Look to see if there are no bugs in similar code (aka code which has similar architecture)
First and Second Reviewer
Goals
First reviewers are for teaching
Second reviewer is ultimately responsible.
First reviewer needs to supply a list of all code that they were not able to verify for the second reviewer.