代码审查指南

本指南包含执行代码审查以及让您的代码接受审查的建议和最佳实践。

所有针对GitLab CE和EE的合并请求，无论是由GitLab团队成员还是更广泛的社区成员编写，都必须经过代码审查流程，以确保代码有效、可理解、可维护且安全。

让您的合并请求获得审查、批准并合并

开始前：

• 熟悉贡献接受标准。

• 如果您需要指导（例如，这是您的第一个合并请求），请随时向其中一位合并请求教练求助。

一旦您有代码需要审查，就让该代码由评审者进行审查。这位评审者可以是您所在组或团队的成员，也可以是领域专家。评审者可以：

• 对所选方案和实现的第二意见提供参考。

• 帮助寻找 bug、逻辑问题或未覆盖的边缘情况。

如果合并请求很小且易于审查，您可以跳过评审者步骤，直接向maintainer求助。

“小而简单”的定义是一个灰色地带。以下是一些小而简单的变更示例：

• 修复拼写错误或进行小的文本更改（示例）。

• 不改变任何行为或数据的微小重构。

• 移除已默认启用超过1个月的功能标志引用。

• 移除未使用的函数或类。

• 需要修改少于5行代码的广为人知的逻辑变更。

否则，合并请求应先由每个类别（例如：后端、数据库）中涉及的MR的评审者进行审查，因为维护者可能不具备相关的领域知识。这也有助于分散工作量。

若需协助安全扫描或评论，请在注释中提及应用安全团队（@gitlab-com/gl-security/appsec）。

评审者使用侧边栏中的评审功能。评审者可以通过额外批准来添加他们的批准。

根据您的合并请求涉及的领域，它必须由一位或多位maintainer进行批准。“批准”按钮位于合并请求小组件中。

让您的合并请求合并也需要一位维护者。如果需要多个批准，最后一位审查并批准的维护者负责合并。

某些领域（如Verify）需要领域专家的批准，基于CODEOWNERS规则。由于CODEOWNERS章节是独立的批准规则，我们可能有某些规则（例如Verify）是其他更通用的批准规则（例如backend）的子集。为了更高效的过程，作者应在通用批准之前寻找领域特定的批准。领域特定批准者也可能是维护者，如果是这样，他们应同时审查领域细节和更广泛的变化，并一次性批准两个角色。

阅读下方关于作者责任的内容了解更多。

领域专家

领域专家是对特定技术、产品功能或代码库区域有丰富经验的团队成员。鼓励团队成员自我认定为领域专家，并将其添加到其团队资料中。

当自我认定为领域专家时，建议将修改.yml文件的MR分配给已建立的领域专家或相应的工程经理。

我们对自动认定为领域专家做出以下假设：

• 在特定阶段/组（例如，创建：源代码）工作的团队成员被视为其工作应用的该区域的领域专家。

• 在特定功能（例如，搜索）工作的团队成员被视为该功能的领域专家。

我们默认将代码审查分配给具有领域专长的团队成员。UX审查默认采用来自Review Roulette的推荐审阅者。由于设计师产能限制，未由产品设计师支持的领域将不再需要UX审查，除非是社区贡献。

当合适的领域专家不可用时，您可以选择任何团队成员来审查合并请求（MR），或遵循Reviewer roulette的建议（如上所述适用于UX审查）。在分配前请再次确认该人员是否处于OOO状态。

要找到领域专家：

• 在合并请求审批小部件中，选择查看符合条件的审批人。此小部件显示每个代码库区域的推荐和必需审批。这些规则在Code Owners中定义。
• 查看与合并请求相关的阶段或组中工作的团队成员列表。
• 在工程项目页面或GitLab团队页面上查看团队成员的领域专长。领域是自我标识的，因此请使用您的判断将合并请求中的更改映射到相应领域。
• 查找对合并请求中的文件做出过贡献的团队成员。通过运行git log <file>查看日志。
• 查找审核过这些文件的团队成员。您可以通过以下方式找到相关合并请求：
  1. 使用git log <file>获取提交SHA。
  2. 导航至https://gitlab.com/gitlab-org/gitlab/-/commit/<SHA>。
  3. 选择显示的与此提交相关的合并请求。

Reviewer roulette

The Danger bot 会随机为合并请求似乎涉及的每个代码库区域挑选一名审阅者和一名维护者。它为开发人员审阅者提供建议，如果您认为其他人更适合，则应覆盖此建议。审批指南 可帮助选择领域专家。

我们仅对包含产品设计师团队的 MR 进行 UX 审查。这些团队的用户界面更改即使隐藏在功能标志后也需要进行 UX 审查。默认采用建议的 UX 审阅者。

它会从工程项目页面的列表中选择审阅者和维护者，其行为如下：

• 它不会选择以下人员的Slack或GitLab状态：
  • 包含字符串 OOO、PTO、Parental Leave、Friends and Family 或 Conference。
  • 表情符号属于以下类别之一：
    • 休假中 - 🌴 palm_tree，🏖️ beach，⛱ beach_umbrella，🏖 beach_with_umbrella，🌞 sun_with_face，🎡 ferris_wheel，🏙 cityscape
    • 生病请假 - 🌡️ thermometer，🤒 face_with_thermometer
• 重要提示：当状态表情符号出现在自由文本输入的状态消息中时不会被检测到。它们必须通过点击文本输入旁边的表情选择器，设置在您的 GitLab 状态表情符号中。
• 它不会选择已分配的评审数量等于或超过其选择的“评审限额”的人员。评审限额是人们一次能处理的最多评审数量。通过使用以下任一选项作为Slack或GitLab状态来设置评审限额：
  • 2️⃣ - two
  • 3️⃣ - three
  • 4️⃣ - four
  • 5️⃣ - five
• 最低评审限额为2️⃣。关于为何不能完全关闭评审的原因已在此问题中讨论。
• 针对不属于安全组下任何项目的默认分支的合并请求的评审请求不计入统计。这些MR通常是回溯版本，维护者或审阅者通常不需要太多时间进行评审。
• 对于相同的分支名称，它始终会选择相同的审阅者和维护者（除非他们的OOO状态发生变化，如第1点所述）。它会移除开头的ce-和ee-以及结尾的-ce和-ee，以便对回溯分支保持稳定。
• 其Slack或GitLab状态表情符号为Ⓜ :m:的人员仅在他们是维护者的项目中作为审阅者被建议。

Roulette仪表板包含：

• 过去7天和30天的分配事件。

• 每人当前分配的合并请求。

• 按不同标准排序。

• 手动评审轮盘。\n- 本地时间信息。\n\n有关更多信息，请查看 轮盘 README。\n\n### 审批指南\n\n如以下维护者职责部分所述，建议您让具有领域专业知识的维护者批准并合并您的合并请求。此处未涵盖第一审阅者的可选审批。但是，在将您的合并请求提交给维护者之前，应按照概述部分所述，由一名审阅者进行审查。\n\n| 如果您的合并请求包含 | 它必须由以下人员批准 |\n| ————————— | ——————– |\n| ~backend 变更 ¹ | 后端维护者。 |\n| ~database 迁移或对昂贵查询的变更 ² | 数据库维护者。有关更多详细信息，请参阅 数据库审核指南。 |\n| ~workhorse 变更 | Workhorse 维护者。 |\n| ~frontend 变更 ¹ | 前端维护者。 |\n| ~UX 面向用户的变化 ³ | 产品设计师。有关详细信息，请参阅 设计和用户界面指南。 |\n| 添加新的 JavaScript 库 ¹ | - 如果该库显著增加了 包大小，则需 前端设计系统成员。
  - 如果新库使用的许可证尚未获得 GitLab 使用许可，则需 法律部门成员。
  
  有关许可证兼容性的更多信息，请参见我们的 GitLab 许可证与兼容性文档。 |\n| 新依赖项或文件系统变更 | - 分发团队成员。有关如何与 分发团队 合作，请参见详情。
  - 对于 RubyGems，请申请 AppSec 审核。 |\n| ~documentation 或 ~UI text 变更 | 根据适当 DevOps 阶段组 中的分配情况，由 技术作家 进行处理。 |\n| 开发指南变更 | 遵循 审核流程 并相应获取批准。 |\n| 端到端 和 非端到端变更 ⁴ | 测试软件工程师。 |\n| 仅端到端变更 ⁴ 或 如果 MR 作者是 测试软件工程师 | 质量维护者。 |\n| 新增或更新的 应用限制 | 产品经理。 |\n| 分析工具（遥测或分析）变更 | 分析工具工程师。 |\n| 对 功能规范 的添加或修改 | 质量维护者 或 质量审阅者。 |\n| 向 GitLab 添加新服务（例如 Puma、Sidekiq、Gitaly） | 产品经理。有关向 GitLab 添加服务组件的过程，请参见 添加服务组件到 GitLab 的流程。 |

| 与身份验证相关的更改 | 管理：身份验证。查看组页面上的代码审查部分获取更多详情。已知需要团队审查的文件模式列在CODEOWNERS文件的Authentication部分中，修改这些文件的所有合并请求将在审批者部分列出该团队。 |

| 与自定义角色或策略相关的更改 | 管理：授权工程师。 |

1. 除JavaScript规范外，其他规范被视为~backend代码。Haml标记被视为~frontend代码。但是，Haml模板中的Ruby代码被视为~backend代码。如有疑问，同时请求前端和后端审查。

2. 如果您的合并请求可能引入昂贵的查询，我们鼓励您向数据库维护人员寻求指导。最有效的方法是在有问题的代码行上注释SQL查询，以便他们提供建议。

3. 用户可见的更改包括视觉变化（无论多么微小）以及对渲染DOM的更改，这会影响屏幕阅读器宣布内容的方式。没有专门的产品设计师的组不需要产品设计师批准功能更改，除非更改是社区贡献。

4. 端到端更改包括qa目录中的所有文件。

CODEOWNERS 审批

某些合并请求需要特定组的强制审批。 有关定义，请参阅.gitlab/CODEOWNERS。

.gitlab/CODEOWNERS中的强制部分应仅限于因以下原因而必要的情况：

• 合规性

• 可用性

• 安全性

添加强制部分时，您应该跟踪新强制部分对合并请求率的影响。 有关良好示例，请参见Verify问题。

在其他情况下不应使用强制部分，因为我们更倾向于责任而非僵化。

此外，单体结构的当前结构意味着合并请求很可能触及看似无关的部分。 多个强制审批意味着此类合并请求要求作者寻求审批，这效率不高。

改进工作如下：

• https://gitlab.com/groups/gitlab-org/-/epics/11624

• https://gitlab.com/gitlab-org/gitlab/-/issues/377326

接受检查清单

本检查清单鼓励合并请求（MR）的作者、评审者和维护者确认已分析变更对质量、性能、可靠性、安全性、可观察性和可维护性的高风险影响。

在软件工程中使用检查清单可以提高质量。本检查清单是一个简单的工具，用于支持和增强GitLab代码库贡献者的技能。

质量

有关进一步的质量指南，请参阅测试工程流程。

1. 您已根据代码审查指南自行审阅了此MR。

2. 代码符合软件设计指南。

3. 确保自动化测试存在并遵循测试金字塔。添加缺失的测试或创建记录测试差距的问题。

4. 您已考虑此变更对GitLab.com、Dedicated和自托管的技术影响。

5. 您已适当考虑此变更对系统前端、后端和数据库部分的影响，并相应应用了~ux、~frontend、~backend和~database标签。

6. 您已在所有支持的浏览器中测试了此MR，或确定无需进行此测试。

7. 您已确认此变更是跨更新向后兼容，或您已决定这不适用。

8. 您已正确分离EE内容（如果有）与FOSS。考虑在FOSS上下文中运行CI管道。

9. 您已考虑到现有数据可能出人意料地多样化。例如，如果添加新的模型验证，请考虑使其在现有数据上可选。

10. 您已修复与此MR相关的波动测试，或解释了为何可以忽略它们。波动测试的错误为Flaky test \'<path/to/test>\' was found in the list of files changed by this MR.，但可以在通过警告通过的作业中出现。

性能、可靠性与可用性

1. 你确信此合并请求不会损害性能，或者你已经请审阅者帮助评估性能影响。（合并请求性能指南）

2. 你已在合并请求描述中添加了数据库审阅者的信息，或者你认为没有必要。

• 此合并请求是否有与数据库相关的更改？

1. 你已考虑此变更的可用性和可靠性风险。

2. 你已基于未来预测的增长考虑可扩展性风险。

3. 你已考虑此变更对大型客户的影响——这些客户的数据量可能远超普通客户。

4. 你已考虑此变更对运行在最低配置系统上的客户的影响。

可观测性工具

1. 你已包含足够工具以通过可观测性促进调试和主动性能改进。参见添加功能标志、日志记录和工具的示例。

文档

1. 你已包含变更日志附注，或者认为不需要它们。

• 此合并请求是否需要变更日志？

1. 你已添加/更新文档，或决定此合并请求无需文档变更。

• 是否需要文档？

安全

1. 你已确认：若此合并请求包含对凭证或令牌的处理/存储、授权、认证方法或其他安全审查指南所述项目的变更，你已添加 ~security 标签并 @ 提及 @gitlab-com/gl-security/appsec。

2. 你已查阅内部应用安全审查文档，明确何时和如何请求安全审查；若此变更需审查，你已发起请求。

3. 若有因合并请求审批策略而阻塞合并请求的安全扫描结果：

   • 对真实阳性发现，应在合并前修正，从而移除审批策略要求的AppSec审批。

   • 对假阳性发现、需讨论风险接受的内容或存疑事项，请提及 @gitlab-com/gl-security/appsec。

部署

1. 你已考虑为此变更使用功能标志（因变更可能高风险）。

2. 若使用功能标志，你计划先在预生产环境测试，再推至生产；且已考虑先向部分生产客户推广，再全面 rollout。

• 何时使用功能标志

1. 你已按完成定义告知基础设施部门默认/新设置变更，或认定无需此举。

合规

1. 你已确认已应用正确合并请求类型标签。

合并请求作者的责任

寻找最优方案并落地的责任属于合并请求作者。作者或直接责任人（DRI）需全程作为被指派者关联合并请求。若无法自设被指派者，请让审阅者代劳。

请求维护者审核以批准合并前，他们应确信：

• 它切实解决了目标问题；
• 解决方式最为恰当；
• 满足全部需求；
• 无残留缺陷、逻辑问题、未覆盖边界场景或已知漏洞。

避免与评审者不必要的往返沟通的最佳方式，是对自己的合并请求执行自我审查，遵循代码审查指南。在此自我审查期间，尝试在MR中添加评论，说明做出决策或权衡的行，或在需要上下文解释以帮助评审者更轻松理解代码的地方。

为了达到对其解决方案所需的信心水平，作者应酌情让其他人员参与调研和实施过程。他们被鼓励联系领域专家讨论不同方案或获取实现评审，联系产品经理和UX设计师澄清困惑或验证最终结果是否符合预期，联系数据库专家获取数据模型或特定查询的输入，或联系任何其他开发者对解决方案进行深入评审。

如果你知道交付一个功能需要多个合并请求（例如，你创建了概念验证，且明确该功能将由10个以上的合并请求组成），请考虑识别具备必要理解的评审者和维护者（与他们共享背景信息）。然后将所有合并请求定向给这些评审者。寻找这些评审者的最佳DRI是EM或高级工程师。对于具有相同背景的多个合并请求，拥有稳定的评审对应方可以提高效率。

如果你的合并请求涉及多个领域（例如，动态分析和GraphQL），请向每个领域的专家寻求评审。

如果作者不确定合并请求是否需要领域专家的意见，则表明确实需要。没有它，他们不太可能对自己的解决方案有足够的信心。

在评审前，作者需在合并请求的差异（diff）中提交评论，提醒评审者注意重要事项以及需要进一步解释或关注的内容。可能值得评论的内容示例包括：

• 添加了新的lint规则（如RuboCop、JS等）。
• 添加了新库（如Ruby gem、JS库等）。
• 在不明显的情况下，提供父类或方法的链接。
• 为补充变更而执行的任何基准测试。
• 可能存在安全隐患的代码。

如果有项目、片段或其他资产是评审者验证解决方案所必需的，请在请求评审前确保他们能访问这些资产。

在分配评审者时，可以采取以下措施：

• 在MR中添加评论，说明你希望该评审者提供哪种类型的评审。
• 例如，如果一个MR修改了数据库查询并更新了后端代码，MR作者首先需要~backend评审和~database评审。在分配评审者时，作者会在MR中添加评论，告知每位评审者他们应评审哪个领域。
• 许多GitLab团队成员在多个领域都是领域专家，因此如果没有这种评论，有时会不清楚要求他们提供哪种类型的评审。
• 明确MR评审类型对MR作者来说很高效，因为他们能得到所需类型的评审；对MR评审者来说也很高效，因为他们立即知道要提供哪种类型的评审。
• Example 1
• Example 2

避免：

• 直接在源代码中添加TODO注释（上文提及），除非评审者要求你这样做。如果因可执行任务添加了TODO注释，请包含相关问题的链接。
• 仅解释代码做什么的注释。如果添加非TODO注释，它们应该解释为什么，而不是什么。
• 请求维护者为失败的测试的合并请求进行评审。如果测试失败而你仍需请求评审，请确保留下带有解释的评论。
• 通过邮件或Slack过度提及维护者（如果维护者可通过Slack联系）。如果不能为合并请求添加评审者，在评论中@提及维护者是可接受的；在其他所有情况下，添加评审者就足够了。

这节省了评审者的时间，并帮助作者更早发现错误。

评审者的责任

评审者负责评审所选方案的细节。

如果在评审响应SLO内无法评审已分配的合并请求：

1. 告知作者你无法评审。

使用 GitLab 评审工作负载仪表板 选择新的评审员。

将新评审员分配给合并请求。

这体现了 偏向行动 的理念，并确保高效的 MR 审查进度。

添加如下评论：

Hi <@mr-author>, 我目前无法进行评审，但我已为这个项目 [转动了轮盘](https://gitlab-org.gitlab.io/gitlab-roulette/)，它选择了 <@new-reviewer>。

@new-reviewer，您方便时能否评审此 MR？如果您也无法处理，请再次 [转动轮盘](https://gitlab-org.gitlab.io/gitlab-roulette/) 并选择并分配新的评审员，谢谢。

/assign_reviewer <@new-reviewer>
/unassign_reviewer me

审查合并请求 要彻底。

验证合并请求是否满足所有 贡献接受标准。

有些合并请求可能需要领域专家帮助处理具体细节。 如果评审员不是该领域的领域专家，可以采取以下任一操作：

• 审查合并请求，并拉入一位领域专家进行二次评审。这位专家可以是另一位评审员或维护者。

• 将评审任务交给他们认为更合适的其他评审员。

• 如果没有领域专家可用，则基于最佳努力进行评审。

如果合并请求存在以下情况，你应该引导作者将其拆分为更小的合并请求：

• 过大。

• 修复了多个问题。

• 实现了多个功能。

• 复杂度过高导致额外风险。

作者可以选择请求当前维护者和评审员审查拆分后的 MR，或请求一组新的维护者和评审员。

如果作者添加了本地验证步骤，请说明你是否执行了这些步骤，以便维护者了解它们是否完成以及结果如何。

当你确信其满足所有要求时，应执行以下操作：

• 选择 Approve（批准）。

• @ 提及作者以生成待办通知，并告知他们合并请求已被评审和批准。

• 向维护者请求评审。默认情况下，向具有 领域专业知识 的维护者请求评审；但如果没有此类人员，或者你认为合并请求不需要 领域专家 的评审，可遵循 评审轮盘 的建议。

维护者的职责

维护者负责整个 GitLab 代码库的整体健康、质量和一致性，涵盖各个领域和产品区域。

因此，他们的评审主要关注整体架构、代码组织、关注点分离、测试、DRY 原则、一致性和可读性等方面。

由于维护者的工作仅依赖于他们对整个 GitLab 代码库的了解，而非任何特定领域的知识，所以他们可以评审、批准和合并来自任何团队及任何产品区域的 MR。

维护者是确保合并请求接受标准得到合理满足的 DRI（直接责任人）。一般来说，质量是每个人的责任，但 MR 的维护者需负责 确保 该 MR 满足通用质量标准。

这包括 避免在后续问题中产生技术债务。

如果维护者认为某个 MR 规模较大，或需要 领域专家，维护者有权请求另一位评审员或维护者进行评审。以下是维护者在评审过程中主动这样做的示例：

• https://gitlab.com/gitlab-org/gitlab/-/merge_requests/82708#note_872325561
• https://gitlab.com/gitlab-org/gitlab/-/merge_requests/38003#note_387981596
• https://gitlab.com/gitlab-org/gitlab/-/merge_requests/14017#note_178828088

维护者会尽力在合并前评审所选解决方案的具体细节，但由于他们不一定具备 领域专业知识，若不投入不合理的时间，可能难以胜任此项工作。在这种情况下，他们会尊重作者和早期评审员的判断，优先专注于自身的主要职责。

如果一个开发者恰好也是维护者，且参与了某合并请求作为评审员，建议不要同时由他担任最终批准和合并该 MR 的维护者。

维护者应在合并前检查合并请求是否获得了所需审批人的批准。 如果仍在等待其他人的进一步批准，请 @ 提及作者并在评论中说明原因。

某些合并请求可能针对稳定分支。有关如何处理这些请求的概述，请参阅[补丁发布操作手册](https://gitlab.com/gitlab-org/release/docs/-/blob/master/general/patch/engineers.md)。

合并后，维护者应保持为合并请求中列出的审阅者。

### 内部使用审阅者功能

我们的代码审查流程内部使用了[Merge request reviews 功能](../user/project/merge_requests/reviews/_index.md)。以下是摘要，其他部分也有体现。

- 合并请求作者和 DRI 保持为 Assignee。
- 合并请求审阅者在审阅后仍保持为审阅者。
- 作者通过将用户分配为审阅者来[请求审阅](../user/project/merge_requests/reviews/_index.md#request-a-review)。
- 当作者做出更改并希望审阅者重新审阅时，作者会[重新请求审阅](../user/project/merge_requests/reviews/_index.md#re-request-a-review)。
- 审阅者使用[审阅功能](../user/project/merge_requests/reviews/_index.md#start-a-review)提交反馈。您可以在 MR 的任何评论上下文中选择**开始审阅**或**开始审阅**，而不是**立即添加评论**。

## 最佳实践

### 所有人

- 友善待人。
- 接受许多编程决策都是主观意见。讨论权衡取舍、你的偏好，并快速达成解决方案。
- 提问；不要下命令。（"你觉得把这个命名为`:user_id`怎么样？"）
- 要求澄清。（"我不明白。你能解释一下吗？"）
- 避免对代码有选择性所有权。（"我的"，"不是我的"，"你的"）
- 避免使用可能被视为涉及个人特质的术语。（"愚蠢的"，"笨的"）。假设每个人都很聪明且出于好意。
- 明确表达。记住人们并不总是能理解你在网上的意图。
- 谦虚。（"我不确定——我们查一下吧。"）
- 不要使用夸张的说法。（"总是"，"从不"，"没完没了"，"什么都没有"）
- 小心使用讽刺。我们所做的一切都是公开的；对你和长期同事来说似乎是善意的调侃，但对新加入项目的人来说可能会显得刻薄和不友好。
- 如果有太多"I didn't understand"（我不明白）或"Alternative solution:"（替代方案：）的评论，考虑进行一对一聊天或视频通话。发布后续评论总结一对一讨论的内容。
- 如果你向特定的人提问，总是在评论开头提到他们；如果他们的通知级别设置为"被提及"，这样可以确保他们看到，也让其他人知道他们不需要回应。

### 让合并请求更快通过的作者建议

1. 确保遵循最佳实践。
   - 编写高效的说明，添加截图、验证步骤等。
   - 阅读并及时回复`dangerbot`添加的所有评论。
   - 遵循[验收清单](#acceptance-checklist)。

1. 遵循 GitLab 模式，即使你认为有更好的方法。
   - 讨论通常会延迟代码合并。如果讨论变得过长，考虑遵循文档中的方法或维护者的建议，然后打开一个单独的 MR 来实现你的方法，作为我们最佳实践的一部分，并在那里进行讨论。

1. 考虑将大的合并请求拆分为较小的请求。大约 `200` 行是一个很好的目标。
   - 较小的 MR 减少了作者和审阅者的认知负担。
   - 审阅者倾向于先挑选较小的 MR 进行审阅（大量文件可能会让人望而却步）。
   - 对代码某一特定部分的讨论不会阻塞其他部分的代码合并。
   - 较小的 MR 通常更简单，你可以考虑跳过第一次审阅，直接[发送给维护者](#getting-your-merge-request-reviewed-approved-and-merged)，或者跳过其中一个建议的专业领域（例如前端或后端）。
   - 使用模拟对象是一种好的方法，尽管它们会在之后增加另一个 MR；用服务器请求替换模拟通常是一个快速的 MR 来审阅。
   - 确保任何带有模拟数据的 UI 都在[功能标志](feature_flags/_index.md)后面。
   - 将公共依赖项拉入第一个 MR 中，以避免过多的变基。
   - 对于顺序 MR，使用[堆叠差异](../user/project/merge_requests/stacked_diffs.md)。
   - 对于依赖的 MR（例如，`A` → `B` → `C`），让它们的分支相互指向，而不是指向 `master`。例如，让 `C` 指向 `B`，`B` 指向 `A`，`A` 指向 `master`。这样每个 MR 只会有对应的 `diff`。
   - ⚠️ 谨慎拆分 MR：过于小的 MR 会增加总审阅次数，可能导致相反的效果。

1. 最小化单个 MR 中的审阅者数量。
   - 示例：数据库审阅者也可以审阅后端或测试。全栈工程师可以同时审阅前端和后端。
   - 使用模拟可以使最初的 MR 仅包含 `frontend`，之后我们可以为服务器请求请求 `backend` 审阅（见上文"拆分 MR"）。

### 让你的合并请求获得审阅

请记住，代码审查是一个可能需要多次迭代的过程，审阅者可能在后续发现第一次没注意到的问题。

• 你的代码的第一个审阅者是你自己。在你执行首次推送你那闪亮的新分支之前，通读整个差异（diff）。这合理吗？你是否包含了一些与整体变更目的无关的内容？你是否忘记移除任何调试代码？

• 按照合并请求指南撰写详细描述。有些审阅者可能不熟悉产品功能或代码库区域。详尽的描述能帮助所有审阅者理解你的请求并有效测试。

• 如果你知道你的变更依赖于另一个先合并，请在描述中注明，并设置合并请求依赖。

• 感谢审阅者的建议。（“好主意。我会做这个修改。”）

• 别往心里去。评审的是代码，不是你本人。

• 解释代码存在的原因。（“之所以这样是因为这些原因。如果我把类/文件/方法/变量重命名会不会更清晰？”）

• 将无关的变更和重构提取到未来的合并请求/问题中。

• 尝试理解审阅者的视角。

• 回复每一条评论。

• 合并请求作者仅解决已完全处理的线程。如果有未回复的评论、开放的线程、建议、问题或其他内容，该线程应留给审阅者解决。

• 不应假设所有反馈都需要在合并请求（MR）合并前纳入推荐变更。是否需要纳入，还是应在当前 MR 合并后创建后续问题来解决反馈，由 MR 作者和审阅者共同判断。

• 根据之前的反馈轮次，将提交作为独立提交推送到分支。在分支准备好合并前不要压缩（squash）。审阅者应能阅读基于其之前反馈的单独更新。

• 准备进行下一轮评审时，向审阅者请求新评审。如果你没有请求评审的能力，可@提及审阅者代替。

请求评审

当你准备让合并请求被评审时，你应该通过选择审阅者来请求初始评审，依据审批指南。

当合并请求有多个需评审的区域时，建议你指定审阅者应评审哪个区域及处于哪个阶段（首轮或第二轮）。这将帮助符合多个区域审阅资格的团队成员明确自己被要求评审的区域。例如，当合并请求同时涉及backend和frontend问题时，你可以这样提及审阅者：@john_doe 能请你评审~backend部分吗？ 或 @jane_doe - 能否请你给这个 MR 做~frontend 维护者评审？

你也可以使用workflow::ready for review标签。这意味着你的合并请求已准备好评审，任何审阅者都可 picking。仅在无时间压力且确保合并请求已分配给审阅者的情况下才建议使用该标签。

重新请求评审时，点击重新请求评审图标（ ）位于审阅者姓名旁，或使用/request_review @user快捷操作。这能确保合并请求出现在审阅者合并请求主页面的待评审部分。

当你的合并请求收到首个审阅者的批准后，可转交给维护者。默认应选择具有领域专业知识的维护者；否则遵循“审阅者轮盘”（Reviewer Roulette）建议或使用ready for merge标签。

有时，维护者可能无法参与评审。他们可能不在办公室或满负荷工作。你可以也应该查看维护者个人资料中的可用性。如果轮盘推荐的维护者不可用，从该列表中选择其他人。

确保合并请求被评审是作者的责任。若合并请求长期处于ready for review状态，建议向特定审阅者请求评审。

自愿参与评审

有能力的GitLab工程师可以定期查看待审核的合并请求列表，并为任何他们想审核的合并请求添加自己作为审核者。

审核合并请求

理解为何需要进行此更改（修复错误、提升用户体验、重构现有代码）。然后：

• 尽量全面地进行审核，以减少迭代次数。
• 明确哪些想法你强烈支持，哪些不支持。
• 在解决问题的同时，找出简化代码的方法。
• 提供替代实现方案，但假设作者已经考虑过这些方案。（“你觉得在这里使用自定义验证器怎么样？”）
• 尝试理解作者的视角。
• 检出分支，并在本地测试更改。你可以决定执行多少手动测试。你的测试可能会带来添加自动化测试的机会。
• 如果不理解某段代码，说出来。其他人也很可能对它感到困惑。
• 确保作者清楚需要他们做什么来处理/解决建议。
• 考虑使用常规评论格式来表达你的意图。
• 对于非强制性建议，用(non-blocking)标记，这样作者就知道他们可以在合并请求中可选地解决，或在后续阶段跟进。当所有建议都是非阻塞时，将MR推进到下一阶段以减少异步周期。当你是一轮审核中的第一轮审核者时，将其转交给维护者审核。当你是最终批准的维护者时，从非阻塞建议生成后续任务，并合并或设置自动合并。作者随后可以选择通过实施非阻塞建议来取消自动合并，在MR合并后提供后续MR，或不实施这些建议。
• 有一个Chrome/Firefox插件，你可以用它应用常规评论前缀。
• 确保没有未解决的依赖项。检查关联问题是否有阻碍因素。如有必要，与作者澄清。如果被一个或多个未关闭的MR阻塞，设置MR依赖。
• 在一轮行内注释之后，发布总结性备注可能会有帮助，例如“看起来不错”或“有几件事需要处理”。
• 如果你的审核后有需要修改的地方，请告知作者。

合并合并请求

在决定合并之前：

• 设置里程碑。
• 确认已应用正确的MR类型标签。
• 考虑来自danger bot、代码质量和其他报告的警告和错误。除非能有力证明违规的合理性，否则应在合并前解决这些问题。如果MR合并时有失败的作业，必须发表评论。
• 如果MR包含质量和非质量相关变更，则应在质量相关变更获得软件测试工程师批准后，由负责面向用户的变更（后端、前端或数据库）的相关维护者合并该MR。

至少需要一个维护者批准MR才能合并。MR作者和向MR添加提交的人无权批准MR，必须寻找未参与该MR的维护者来批准。通常，最终所需的批准者应合并该MR。

最终批准者可能不会合并MR的场景：

• 批准者忘记在批准后设置自动合并。
• 批准者没意识到自己是最终批准者。
• 批准者设置了自动合并，但GitLab取消了该设置。

如果发生上述任一情况，若MR已获得所有必需的批准且作者拥有仓库的合并权限，则MR作者可以自行合并其MR。这也符合GitLab 行动倾向价值观。

制定此政策是为了满足GitLab 变更管理控制中的CHG-04控制要求。为了在gitlab-org/gitlab中实施此政策，我们启用了以下设置，以确保MR获得顶级CODEOWNERS维护者的批准：

• 防止作者批准。

• 防止添加提交的用户批准.

• 防止在合并请求中编辑审批规则.

• 向源分支添加提交时移除所有批准.

若要更新 gitlab-org/gitlab 项目中 CODEOWNERS 文件的所有者，请遵循 代码所有者审批手册章节 中说明的流程。

某些操作（例如本地 rebase 或应用建议）被视为与添加提交相同，可能会重置现有的批准。从 UI 或通过 /rebase 快速操作 进行 rebase 时，不会移除批准。

准备合并时：

• 当合并请求包含大量提交时，可考虑使用 Squash and merge 功能。合并代码时，只有当作者已设置此选项，或合并请求明显包含混乱的提交历史时，维护者才应使用 squash 功能——这样 squash 提交会比回过头与作者沟通更高效。否则，若 MR 仅包含少量提交，我们应尊重作者设置而不 squash 它们。

• 前往合并请求的 Pipelines 标签页，选择 运行流水线。然后在 概览 标签页上，启用 自动合并。需考虑以下信息：

  • 若 默认分支已损坏，则不应合并该合并请求，除非属于 非常特殊的情况。其他情况请遵循这些 手册说明。

  • 若最新流水线是在合并请求被批准前创建的，请启动新流水线以确保完整的 RSpec 套件已运行。仅当合并请求不含任何后端变更时，方可跳过此步骤。

  • 若 最近的 合并结果流水线 是 在不到 8 小时前创建的（稳定分支为 72 小时），则因合并请求已足够接近目标分支，无需启动新流水线即可合并。

  • 将 MR 设为自动合并后，你应接管此后发现问题的后续修订。

  • 对于已设置 Squash and merge 的合并请求，squashed 提交的默认提交消息取自合并请求标题。我们鼓励你在合并前 选择具有更有信息的提交消息的提交。

得益于 合并结果流水线，作者不再需要像以前那样频繁地 rebase 分支（仅在存在冲突时才需要），因为 Merge Results Pipeline 已纳入 main 的最新变更。这加快了审核/合并周期，因为维护者无需再要求最终 rebase——只需启动 MR 流水线并设置自动合并即可。这一步通过在流水线创建时测试 Merge Results 与最新 main 的兼容性，使我们非常接近实际的 Merge Trains 功能。

社区贡献

评审由广泛社区贡献者发起的合并请求时：

• 特别留意新的依赖项及依赖更新，例如 Ruby gems 和 Node 包。虽然 Gemfile.lock 或 yarn.lock 这类文件的变更看似微不足道，但可能导致获取恶意包。

• 审查链接和图片，尤其在文档 MR 中。

• 若有疑问，请在手动启动任何合并请求流水线前，让 @gitlab-com/gl-security/appsec 团队成员审查该合并请求。

• 仅当合并请求很可能纳入当前里程碑时，才设置里程碑。此举是为了避免关于何时合并的混淆，以及避免在尚未准备好时频繁移动里程碑。

接管社区合并请求

当合并请求（MR）需要进一步修改，但作者长时间未回应或无法完成时，GitLab可以接管。
一位GitLab工程师（通常是合并请求教练）将：

1. 在其MR中添加评论，说明你将接管以完成合并。
2. 为其MR添加标签~"coach will finish"。
3. 从主分支创建一个新功能分支。
4. 将其分支合并到你的新功能分支中。
5. 打开一个新的合并请求，将你的功能分支合并到主分支。
6. 从你的MR链接社区MR，并将其标记为~"Community contribution"。
7. 进行任何必要的最终调整，并提醒贡献者，让他们有机会审核你的更改，并告知他们其内容将被合并到主分支。
8. 确保内容符合所有合并请求指南。
9. 像对待任何合并请求一样遵循常规评审流程。

找到合适的平衡点

代码审查中最困难的事之一，是确定评审者对作者编写代码的干预深度。

• 学习如何找到合适的平衡点需要时间；这也是为何一些评审者在经历一段时间的合并请求评审后会成为维护者的原因。
• 发现 bug 很重要，但思考好的设计也同样关键。构建抽象和良好设计能隐藏复杂度，让未来变更更轻松。
• 强制执行和改进代码风格应主要通过自动化完成，而非依赖评审评论。
• 要求作者改变设计有时意味着完全重写贡献的代码。在做此事前先咨询另一位维护者或评审者是好习惯，但当您认为这很重要时，要有勇气行动。
• 出于迭代考量，若您的评审建议是非阻塞变更或个人偏好（非已记录或共识要求），可在返还作者前批准合并请求。这既能让作者在同意时实现建议，也能让其直接提交给维护者评审，从而减少整体合并时间。
• “做对事”和“当下做对事”有区别。理想中我们该做前者，但现实中也需要后者。例如安全修复需尽快发布，不应要求作者在紧急修复的 MR 中做重大重构。
• 今天做好往往优于明天完美。今天交付临时方案通常比明天做好更糟。若您找不到平衡点，可征求他人意见。

GitLab 特定注意事项

GitLab 被广泛使用。许多用户用我们的Omnibus 包，但也有用户用Docker 镜像、从源码安装，或其他安装方式。GitLab.com 自身是大型企业版实例，这带来以下影响：

1. 查询变更需测试，确保其在 GitLab.com 规模下不降低性能：

   1. 本地生成大量数据可辅助测试。
   2. 向 GitLab.com 请求查询计划是最可靠的验证方式。

2. 数据库迁移必须：

   1. 可逆。
   2. 在 GitLab.com 规模下高效——若不确定，让维护者在预发布环境测试迁移。
   3. 正确分类：
      • 常规迁移在新代码在该实例运行前执行。
      • 部署后迁移在新代码部署后运行（当实例配置为如此时）。
      • 批量后台迁移在 Sidekiq 中运行，用于那些超出部署后迁移时间限制的迁移（适配 GitLab.com 规模）。

3. Sidekiq Worker不能以向后不兼容方式变更：

   1. Sidekiq 队列在部署前不会清空，因此队列中存在前一版 GitLab 的 Worker。
   2. 若需更改方法签名，尝试分两版完成，并在首版中同时接受新旧参数。

4. 同样地，如果需要移除一个worker，先在一个版本中停止它被调度，然后在下一个版本中再将其移除。这允许现有的任务继续执行。

5. 别忘了，并非每个实例都会升级到每一个中间版本（有些人可能会从X.1.0直接升级到X.10.0，甚至尝试更大的升级！），因此如果接受旧格式成本不高的话，尽量宽松地接受。

6. 缓存值可能会在版本间持续存在。如果你正在更改缓存值返回的类型（例如，从字符串或nil变为数组），请同时更改缓存键。

7. 设置应作为最后手段添加。参见向GitLab Rails添加新设置。

8. 文件系统访问在云原生架构中是不可能的。确保我们支持对象存储以进行任何需要的文件存储操作。更多信息，请参阅上传文档。

客户关键合并请求

合并请求可能因被视为客户关键优先级而受益，因为这样做对业务有显著好处。

客户关键合并请求的特性：

• 开发部门的高级总监或更高职位人员必须批准该合并请求符合客户关键标准。或者，如果他们的两名直接下属批准，也可以视为通过。

• 负责人（DRI）会将customer-critical-merge-request标签应用到该合并请求上。

• 要求在做出此决定后立即让参与客户关键合并请求的评审者和维护者投入工作。

• 要求优先处理那些参与客户关键合并请求的人员的工作，以便他们有足够的时间专注于此事。

• 在处理客户关键合并请求时，必须遵守GitLab 价值观和流程，特别要注意家庭和朋友优先/工作第二、定义完成度、迭代以及按需发布的原则。

• 客户关键合并请求必须遵循技术决策优先级流程，不得降低安全性、引入数据丢失风险、降低可用性，也不得破坏现有功能。

• 对于客户关键请求，建议相关人员考虑同步协调（如Zoom、Slack）以及异步沟通（合并请求评论），如果他们认为这能减少合并所需时间，即使这可能牺牲效率。

• 客户关键合并请求合并后，必须完成回顾会议，旨在减少未来客户关键合并请求的频率。

排查失败的流水线

有些情况下，流水线会因与已做出的代码变更无关的原因而失败。以下列出了其中一些情况及潜在解决方案。

始终记住，你不需要通过流水线即可请求评审或帮助。如果你的流水线未通过且不知道原因，随时联系团队，通过在MR上留言@gitlab-bot help来寻求MR教练的帮助，或在社区Discord的contribute频道联系。

• 测试因看似与变更无关的原因失败：检查是否默认分支也出现这种情况。如果是这样，你就遇到了“损坏的主分支”，需要等待默认分支上的故障修复。之后，你可以要么变基你的分支，要么如果启用了合并结果流水线，只需运行另一个流水线。

• danger-review作业失败：检查你的MR是否有超过20个提交。如果是这样，变基并压缩它们以减少提交数量，否则尝试再次运行danger-review作业，可能只是临时故障。

示例

代码评审的方式可能会让新贡献者感到意外。以下是一些代码评审示例，可以帮助你了解预期内容。

“修改DiffNote以供设计复用”：其中包含了从关于换行的吹毛求疵到关于设计版本的推理，以及如果没有某个文件的先前版本时应如何比较它们（父版本vs空白shavs空树）的所有内容。

• “支持多行建议”：
  该合并请求本身是前端（FE）与后端（BE）的合作成果，并记录了作者对评审者的评论。其中有一些吹毛求疵的地方、一些信息查询问题，以及最后发现的一个安全漏洞。

• “允许项目包含多个仓库”：
  ZJ提到了这可能影响的其他项目（Workhorse），并提出了一些一致性改进建议。James的评论帮助我们提升了整体代码质量（使用委托、&.这类操作），使代码更健壮。

• “支持合并请求有多个负责人”：
  这是一个涉及代码库多个部分的合并请求协作的好例子。Nick指出了有趣的边缘情况，James Lopez也加入进来，提出了关于导入/导出功能的担忧。

致谢

主要基于thoughtbot代码审查指南。