This document is intended to set a level field for expectations on both developers and inspectors for patches submitted to the Lustre feature and maintenance release branches. It is not intended to replace the detailed Coding Guidelines and Test Coding Style that describe the standard formatting of the Lustre code itself. Rather, it is a higher-level document to explain why a patch might be rejected by an inspector, and the reasoning behind this. The goal is to improve the quality of the initial patches submitted to Gerrit by reducing the amount of time and number of refreshes before a patch can be landed. This benefits both the patch developer and patch inspector by reducing time and effort spent on each patch, and gets fixes/features landed into the code more quickly.
In addition to proper functionality, the most important thing about code is that the next reader understands completely what was in the writer’s head. That means sometimes stylistic concerns rate just as highly as logic errors, since lack of clarity can lead to the next person working on the code to introduce new bugs, even if there are no defects in the current code. That is mainly why the Coding Guidelines exist in the first place - to ensure that the reader does not have to learn the coding style and quirks of each individual developer that has contributed to the code.
Besides the core functionality being added or changed by a patch, it is important that there are adequate tests for each change included in the same patch. If a bug existed in the code and is now being fixed, then the code clearly wasn't being exercised by the current regression tests, and a new test needs to be written. If a new feature is added, we want to ensure that this feature continues to work in the future as the code is modified. Having the tests included with the original patch ensures that this important component is not forgotten once the initial push for landing the patch is past.
In an effort to reduce technical debt in the code, the developer should fix any stye or formatting issues in adjacent lines so that the code is being improved without significantly increasing the footprint of the patch.
It is the responsibility of the developer to ensure that their patch is being moved through the inspection, build, test, and landing process. When a patch is inspected, if there are disagreements about the inspection comments, these should be resolved by replying in Gerrit. In case of a negative inspection, test failures, or merge conflict, it is important to refresh the patch promptly so that any discussion is still fresh in the inspector's memory. Otherwise, the development time and effort put into the patch cannot be captured until the patch has been landed.
When inspecting a patch, it is important to provide timely and complete inspection feedback on the patch. Most developers are also patch inspectors, and prompt patch reviews allows everyone to continue with their work.
Performing only part of an inspection before giving a negative review will result in multiple passed with the patch before it is ready, which increases both the developer effort and inspector effort. It is better to make a complete pass through the patch on the each inspection to find as many issues as possible, so that there is less chance that a patch has to be refreshed multiple times. It can be tempting to wait for another inspector to give a +1 or -1 before doing ones own inspection, but this results in multiple unnecessary iterations on behalf of everyone involved, and can be avoided by performing inspections in a timely manner.
It can be helpful to annotate comments on code into separate categories to help the developer improve their patches in the future:
checkpatch.pl script is run at commit time. The onus is on the developer to look for and fix warnings and errors reported by checkpatch.pl. Since there may be some rare but legitimate reasons to ignore these warnings, they do not immediately cause the commit or push to fail. That said, code style is vital to easy readability across the whole code base, so ignoring the checkpatch.pl output is likely to result in patch rejection.If there is some problem with the patch, in addition to pointing out why the code is wrong it is also best to include a description of how it might be correctly implemented. It typically doesn't makes sense to land a feature patch that is introducing poor code, if it needs to be cleaned up at a later time, so it is important for new features to have high code quality. Otherwise, still another patch will need to be landed, and the "fix it later" patch may never be developed for one reason or another.
It should be noted that the inspector is not infallible, and developers are free to disagree and state reasons in the patch for their disagreement, and an agreement on the best solution can be reached between the inspector and developer. In some cases, looking at a patch for a second or third time brings new insight into some aspect and exposes a bug that wasn't seen the first time around. If it wasn't obvious to the developer that there was a problem in the code, it might not be obvious to the inspector on the first pass either.