Mixing two unrelated functional changes.Īgain the reviewer will find it harder to identify flaws if two unrelated changes are mixed together.Typically the whitespace change would be done first, but that need not be a hard rule. Solution: Create 2 commits, one with the whitespace changes, one with the functional changes. The whitespace changes will obscure the important functional changes, making it harder for a reviewer to correctly determine whether the change is correct. Mixing whitespace changes with functional code changes.With that in mind, there are some commonly encountered examples of bad things to avoid When browsing history using Git annotate/blame, small well defined changes also aid in isolating exactly where & why a piece of code came from.When troubleshooting problems using Git's bisect capability, small well defined changes will aid in isolating exactly where the code problem was introduced.This is much easier to do if there are not other unrelated code changes entangled with the original commit. If a change is found to be flawed later, it may be necessary to revert the broken commit.The smaller the amount of code being changed, the quicker & easier it is to review & identify potential flaws.There are many reasons why this is an important rule: The cardinal rule for creating good commits is to ensure there is only one "logical change" per commit. Software source code is "read mostly, write occassionally" and thus the most important criteria is to improve the long term maintainability by the large pool of developers in the community, and not to sacrifice too much for the sake of the single author who may never touch the code again.Īnd now the long detailed guidelines & examples of good & bad practice The tools being used should be subservient to developers needs, and since they are open source they can be fixed / improved. This is not a valid reason to avoid creating patch series. It might be mentioned that Gerrit's handling of patch series is not entirely perfect. Ensure no-op code refactoring is done separately from functional changes. Ensure whitespace changes are not mixed in with functional changes. Look out for commits which are mixing multiple logical changes and require the submitter to split them into separate commits. Review the commit message itself and request improvements to its content. In other words, when reviewing a change in Gerrit, do not simply look at the correctness of the code. This document intends to be the carrot by alerting people to the benefits, while anyone doing Gerrit code review can act as the stick -P Both a carrot & stick will be required to effect changes. If these guidelines were widely applied it would result in a significant improvement in the quality of the OpenStack Git history. The points and examples that will be raised in this document ought to clearly demonstrate the value in splitting up changes into a sequence of individual commits, and the importance in writing good commit messages to go along with them. The information provided in the commit message.The structured set/split of the code changes.This topic can be split into two areas of concern We can, however, come up with some general guidelines for what to do, or conversely what not to do, when publishing Git commits for merge with a project, in this case, OpenStack. Quality is a hard term to define in computing one person's "Thing of Beauty" is another's "Evil Hack". It is motivated by a desire to improve the quality of the Nova Git history. Examination of other open source projects such as the Kernel, CoreUtils, GNULIB and more suggested they all follow a fairly common practice. The following document is based on experience doing code development, bug troubleshooting and code review across a number of projects using Git, including libvirt, QEMU and OpenStack Nova.
0 Comments
Leave a Reply. |
AuthorWrite something about yourself. No need to be fancy, just an overview. ArchivesCategories |