代码审查

• 原文链接：Code Review

• 作者：JOHN BALDWIN

软件是由不完美的人编写的。因此，软件本身也是不完美的，因为它反映了开发者对正在解决的问题和作为解决方案的软件的理解上的差距和误解。这些差距和误解表现为缺陷，导致不正确的行为、不一致的行为、崩溃、数据丢失等。减少这种问题的一种工具是让其他开发者审查源代码。虽然这些开发者本身也不完美，但差距和误解因人而异。能够审查给定源代码的开发者越多，越有可能发现软件中的缺陷。此外，代码审查还可以促进弗雷德里克·布鲁克斯在《人月神话》中描述的“概念完整性”。换句话说，熟悉系统整体设计的开发者进行代码审查，有助于确保新的更改与系统设计一致，或者更改“按照系统通常的方式执行”。

随着时间的推移，FreeBSD 项目逐渐形成了代码审查的文化。尽管代码审查不是强制性的，但提交的更改中被审查的比例不断增加。当我在 1990 年代末期首次参与项目时，代码审查相当非正式，尤其是对于源代码树的更改。通常的审查发生在私人电子邮件线程中、通过 IRC，或者偶尔在面对面交流时。在发布版的代码冻结期间，代码审查在某种程度上以发布工程团队批准代码冻结提交的形式强制进行。然而，这种批准通常侧重于对更改相对于即将发布的稳定性进行风险评估，而不是代码本身的质量。Ports 开发者的代码审查文化比源代码树更强，几年前，几位 Ports 开发者搭建了 Phabricator 代码审查工具实例来审查 FreeBSD 补丁。最初的重点是提供比 Bugzilla 更好的机制来审查 Ports 的补丁，不过，几位源代码开发者也开始使用它。

像 Phabricator 这样的工具相对于 FreeBSD 使用过的其他系统具有几个优势。审查通常包括关于新更改的各种权衡或假设的讨论。当这些审查发生在私人电子邮件线程中时，这些知识仅限于参与私密线程的个人。而当这些审查发生在 Phabricator 中时，这些讨论会被存档，并且可以通过将更改添加到代码树的提交中的 URL 找到。同时，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 以明确批准所请求的更改。

• 保持耐心。

• 对反馈做出响应。

在审查更改时，开发者应：

• 首先审查设计，而不是像样式这样的较小修复。如果更改与系统架构根本不兼容，要求提交者重新调整补丁的样式然后再因架构问题拒绝它，会浪费大家的时间。虽然在讨论设计时，可能会有一些关于样式规则的一般性建议是有意义的，但初步审查应重点关注补丁的设计和结构。

• 如果批评补丁的设计或结构，提供建设性的批评，包括替代设计或方法，以实现期望的结果。

• 及时回应。尤其是当审查者已经回应并解决了早期反馈时，及时批准更改并将其合并到源代码树中很重要。

• 如果你信任提交者能够在提交之前做出必要的更改，可以批准仍需一些更改的补丁。在要求更改时，明确说明进一步的更改是可选建议（因此可以选择提交或不提交），还是必须在批准有效之前应用的强制修复。

• 愿意承认某些建议实际上是偏好，并将其分类为偏好，而非强制修复。

• 当注意到需要更改的模式时（例如，提交者可能误解或不知道的样式规则），与其在第一次审查时指出每个缺陷，不如指出一个实例并引用一般规则，让提交者有机会在整个补丁中解决该问题。