代码审查
最后更新于
最后更新于
原文链接：
作者：JOHN BALDWIN
软件是由不完美的人编写的。因此，软件本身也是不完美的，因为它反映了开发者在理解问题和作为解决方案的软件之间的差距和误解。这些差距和误解表现为错误，导致不正确的行为、不一致的行为、崩溃、数据丢失等。减少这种问题的一种工具是让其他开发者审查源代码。虽然这些开发者本身也不完美，但每个人的差距和误解是不同的。能够审查给定源代码的开发者越多，越有可能发现软件中的漏洞。此外，代码审查还可以促进弗雷德里克·布鲁克斯在《人月神话》中描述的“概念完整性”。换句话说，熟悉系统整体设计的开发者进行代码审查，有助于确保新的更改与系统设计一致，或者更改“按照系统通常的方式执行”。
随着时间的推移，FreeBSD 项目逐渐形成了代码审查的文化。尽管代码审查不是强制性的，但被审查的提交更改的比例不断增加。当我在 1990 年代末期首次参与项目时，代码审查相当非正式，尤其是对于源代码树的更改。通常被审查的更改是在私人电子邮件线程中、通过 IRC，或者偶尔在面对面交流时进行的。在发布的代码冻结期间，代码审查在一定程度上是由发布工程团队批准代码冻结提交的形式进行的。然而，这种批准通常侧重于评估更改对即将发布的稳定性的影响，而不是代码本身的质量。Ports 开发者在代码审查文化上比源代码树更为强大，几年前，几位 Ports 开发者启动了 Phabricator 代码审查工具实例来审查 FreeBSD 补丁。最初，重点是提供一个比 Bugzilla 更好的机制来审查 Port 的补丁，然而，几位源代码开发者也开始使用它。
像 Phabricator 这样的工具相对于 FreeBSD 使用过的其他系统具有几个优势。审查通常包括关于新更改的各种权衡或假设的讨论。当这些审查发生在私人电子邮件线程中时，这些知识仅限于私密线程中的个人。而当这些审查发生在 Phabricator 中时，这些讨论会被存档，并且可以通过从提交更改到代码树的链接找到。同时，Phabricator 能让开发者限制他们希望参与的审查，而无需让所有开发者都浏览每个更改的所有审查评论。与 Bugzilla 不同，Phabricator 能对补丁的单独行进行评论。最新版本的 Phabricator 包括允许审查者输入代码片段作为建议，并可以将其应用于待处理的更改。
有效的代码审查需要提交补丁的开发者和审查更改的开发者之间的合作。本文的其余部分将重点讨论使用 FreeBSD 的 Phabricator 实例时的最佳实践，尽管类似的指导原则也适用于通过其他媒介（如电子邮件）进行的审查。一些这些指导原则也可以在非 FreeBSD 环境中使用。
在提交补丁时，开发者应：
上传包含完整上下文的补丁。使用 arcanist 工具（来自 Port 或包 devel/arcanist）上传补丁时，默认情况下会包含完整上下文。如果从版本控制系统生成 diff，强制 diff 包含完整上下文。使用 diff、git diff 或 git show 时，在命令行中添加参数 -U999999 。要在 Subversion 检出中生成 diff，请使用 svn diff --diff-cmd=diff -x -U999999。包括完整上下文使得开发者能够查看更改周围的其他代码部分。
在更改描述中提供拟议更改的提交日志。这使审查者能够审查代码和描述更改的提交日志。编写合适的提交日志是另一个话题。
将相关小组标记为审查者。例如，对于对 PCI 总线驱动程序的更改，添加 #PCI 小组。一些小组使用规则自动为某些路径下的文件添加小组。例如，对 bhyve 虚拟机监控程序的更改会自动标记为 #bhyve 小组。
如果补丁提交者在提交补丁审查之前曾与特定开发者合作过，请将这些开发者添加为审查者或订阅者。
对于 FreeBSD 源代码更改，尽量按照 style(9) 手册页中的指南格式化代码。某些指南可能含糊不清，补丁不必完美。然而，越接近现有风格，您将需要做的更改就越少，也更有可能让其他开发者愿意审查您的更改，并在必要时将其纳入代码库。
如果审查者批准了您的补丁，但包括了说明批准依赖于某些更改的评论，那么批准仅在补丁提交者进行所请求的更改时有效。另一方面，如果审查者批准了补丁，但包括了不要求批准的附加评论，则由补丁提交者决定是否进行所请求的更改。待补丁获得批准，可以提交，而无需再次上传到 Phabricator 以明确批准所请求的更改。
保持耐心。
对反馈做出响应。
在审查更改时，开发者应：
首先审查设计，而不是像样式这样的较小修复。如果更改与系统架构根本不兼容，要求提交者重新调整补丁的样式然后再因架构问题拒绝它，会浪费每个人的时间。虽然在讨论设计时，可能会有一些关于样式规则的一般性建议是有意义的，但初步审查应重点关注补丁的设计和结构。
如果批评补丁的设计或结构，提供建设性的批评，包括替代设计或方法，以实现期望的结果。
及时回应。尤其是当审查者已经回应并解决了早期反馈时，批准更改/将其合并到代码库中是很重要的。
如果您信任提交者能够在提交之前做出必要的更改，请愿意批准仍需一些更改的补丁。在要求更改时，明确说明进一步的更改是可选建议（因此可以选择提交或不提交），还是必须在批准有效之前应用的强制修复。
愿意承认某些建议实际上是偏好，并将其分类为偏好，而非强制修复。
当注意到需要更改的模式时（例如，提交者可能误解或不知道的样式规则），与其在第一次审查时指出每个缺陷，不如指出一个实例并引用一般规则，让提交者有机会在整个补丁中解决该问题。