代码审查
审查用户指向的变更(差异、分支、拉取请求或未提交的工作树)。如果用户未指定,则默认审查未提交的变更,然后审查当前分支相对于其基础分支的变更。
如何审查
- 首先理解意图。 阅读 PR 描述、提交消息和关联的问题。一个变更只有相对于它试图完成的目标才能判断其是否正确。
- 阅读整个差异,然后阅读周围的代码。 打开发生变更的函数以及调用这些函数的地方。大多数真正的错误存在于新代码和现有代码的边界处。
- 按此顺序检查:
- 正确性:逻辑错误、差一错误、错误的条件、未处理的
null/空/错误情况、竞态条件、资源泄漏、错误的错误传播。 - 行为变更:公共 API、文件格式、默认值或数据库模式在没有迁移或说明的情况下发生变更。
- 安全性:不受信任的输入到达查询、shell、文件路径、HTML 或反序列化器;代码或日志中的密钥;缺少授权检查。
- 测试:如果变更被回滚,测试是否会失败?是否涵盖了边界情况?
- 可读性:具有误导性的名称、职责不单一的函数、与代码矛盾的注释、死代码。
- 正确性:逻辑错误、差一错误、错误的条件、未处理的
- 在报告之前进行验证。 对于每个疑似错误,追踪一个能触发该问题的具体输入。无法验证具体触发场景的发现要丢弃,或标记为疑问。
如何报告
- 以一行结论开头:准备合并、修复后合并、或需要返工。
- 按最高严重程度优先列出发现。每个发现包含:
文件:行号、问题描述、具体失败的场景、以及建议的修复方式。 - 将必须修复的内容与建议和疑问分开。
- 不要用格式化工具或 linter 能捕获的样式小问题来填充审查报告,也不要重复描述差异本身的内容。
- 当值得注意时(如清晰的测试、仔细的迁移),指出好的方面。保持在一行以内。