Jump to content

Gerrit/代码审查

本頁使用了標題或全文手工轉換
From mediawiki.org
This page is a translated version of the page Gerrit/Code review and the translation is 35% complete.
Languages:

本指南旨在審查且合併維基媒體代碼儲存庫的貢獻內容，主要針對實際「執行」程式碼審查的開發人員所撰寫的。

目标

  • 提供快速審查以避免位元衰減及被遺棄感。 快速的回饋能激勵開發者持續貢獻，從而有助於擴大我們的貢獻者基礎。
  • 請保持友善。 提交的修補程式是貢獻者的饋贈。 審查過程將影響志願者對於整個專案的觀感。
  • 長遠來看，應鼓勵程式碼貢獻者同時也成為審查員。

寻找要审查的补丁

有些開發人員可能會以無人機要求你審查他們的程式碼。

有大量的程式碼需要審查。 如何將任務拆解成更易於管理的部份？

有幾種基本的模式：

按请求

Contributors can request code review in Gerrit by adding a reviewer and clicking "Add to attention set". This is a helpful way to indicate to a user that you wish them to do a review.

In the Gerrit UI patch contributors can request reviewer by adding their user name and clicking "Add to attention set"

Reviewers should therefore bookmark and monitor requests for code review in this form using the Gerrit URL query has:attention.

When a patch has your attention you can do either:

  • Provide a review for the patch as requested (which will move the attention back to the patch contributor)
  • If you are not the right person to review the patch, comment and/or suggest alternative reviewers using the "add to attention set" feature. You can use the Maintainers page to identify alternative reviewers.

按项目

If you are a maintainer, consider setting up email notifications for new patchsets in your projects (repositories) via "Watched Projects" in Gerrit. Alternatively you can add yourself to the Gerrit Reviewer Bot which will automatically add you as a reviewer to each new patchset.

  • Identify major pieces of work or sets of related revisions. A quick chronological review can be a good way to do this; or you can choose a repository with many open changesets.
  • Open all changeset pages as tabs in a browser window. Open relevant files in a text editor.
  • Review new or unfamiliar files in their entirety. Pick a changeset with a major change to the relevant file and review.

按作者

  • Pick an author with (many) open changesets, load them in Gerrit.
  • Work through the revisions chronologically, or proceed one topic/repository at a time.
  • This method allows the reviewer to get to know individual developers: their skills, faults and interests. The work has a sense of progress and continuity.

If someone already added you as a potential reviewer and you know you will not review the patch, remove yourself from the list of reviewers.[1]

New contributors to our projects

You can add (some of) the queries below to your menu by editing your user preferences. A "new contributor" is defined as a person who has contributed five or less changesets in total.

Chronological (and Reverse Chronological)

  • Start at the oldest open changeset, review until you finish the queue. Alternately, start at the latest revision and read each diff in turn until you reach the end. This approach is good for minor revisions, but requires constant switching between projects and their contexts.

审查清单

是我們要的嗎

  • 開門見山的問題是這個貢獻是否是個好的想法。 若該貢獻無助益或不符合專案方向與範圍，請說明並提供更好構想的反饋意見。[2]

概述

  • 所提交的程式碼應如預期運作，即程式碼中發現的任何錯誤皆應予修正。 （但請謹慎行事，切勿將過往開發者撰寫的程式碼歸咎到現任的開發者。）
  • 若此事相對簡單的話，則請維持向下相容以穩定介面。
  • 若需進行重大變更以實現顯著改進，請確保遵循穩定介面政策。
  • 請閱讀相關的bug報告或文檔。
  • 請你自己要熟悉任何相關的技術問題。請閱讀相關規格或手冊章節。

性能

设计

  • Does this change make the user experience better or worse for end users? If it has a user experience or visual design impact, consider consulting #wikimedia-design 在线 or the design mailing list, or one of the design maintainers.

风格

可读性

  • Functions should do what their names say. Choosing the correct verb is essential, a get*() function should get something, a set*() function should set something.
  • Variables names should use English, abbreviations should be avoided where possible.
  • Doc comments on functions are preferred.
  • Overly-complicated code should be simplified. This usually means making it longer. For instance:
    • Ternary operators (?:) may need to be replaced with if/else.
    • Long expressions may need to be broken up into several statements.
    • Clever use of operator precedence, shortcut evaluation, assignment expressions, etc. may need to be rewritten.
  • Duplication, whether within files or across files, should be avoided.
    • It is bad for readability since it's necessary to play "spot the difference" to work out what has changed. Reading many near-copies of some code necessarily takes longer than reading a single abstraction.
    • It is bad for maintainability, since if a bug (or missing feature) is found in the copied code, all instances of it have to be fixed.
    • Some new developers might copy large sections of code from other extensions or from the core, and change some minor details in it. If a developer seems to be writing code which is "too good" for their level of experience, try grep'ing the code base for some code fragments, to identify the source. Guide the developer towards either rewriting or refactoring.
    • Taking shortcuts can be counterproductive, since the amount of time spent figuring out the shortcut and verifying that it works could have been spent just typing out the original idea in full.

安全性

  • The reviewer should have read and understood the security guide and should be aware of the security issues discussed there.
  • There should not be the remotest possibility of arbitrary server-side code execution. This means that there should be no eval() or create_function(), and no /e modifier on preg_replace().
  • Check for text protocol injection issues (XSS, SQL injection, etc.) Insist on output-side escaping.
  • Check any write actions for CSRF.
  • Be wary of special entry points which may bypass the security provisions in WebStart.php.
  • Be wary of unnecessary duplication of security-relevant MW core functionality, such as using $_REQUEST instead of $wgRequest, or escaping SQL with addslashes() instead of $dbr->addQuotes().
  • Only if you work on ancient code: Check for register_globals issues, especially classic arbitrary inclusion vulnerabilities. (Register Globals has been removed in PHP 5.4.0 and MediaWiki ≥1.27 requires PHP ≥5.5.9.)
  • If in doubt, consider contacting the Wikimedia Security Team.

架构

  • Names which are in a shared (global) namespace should be prefixed (or otherwise specific to the extension in question) to avoid conflicts between extensions. This includes:
    • Global variables
    • Global functions
    • Class names
    • Message names
    • Table names
  • The aim of modularity is to separate concerns. Modules should not depend on each other in unexpected or complex ways. The interfaces between modules should be as simple as possible without resorting to code duplication.
  • Check against the Architecture Principles.

逻辑

  • 指出捷徑之所在、並要求開發人員做好本分。

完成審查

警告 警告： 请勿使用“Verified +2”這幾個字。 这是绕过测试的“强制合并”。 参见Gerrit/权限方针#合并而不审查

给予正面反馈

  • 如果你想帮助审查代码，但对做最终决定而感到不自在，你可以在Gerrit上使用Code-Review +1来表明你已“验证”和/或“检查”了代码。
  • 如果此修订是好的且通过了上面的所有测试，在Gerrit上将其标记为Code-Review +2。 如果你对某人的工作印象特别深刻，请在评论中表达出来。 当你用Code-Review +2标记了一次提交，你可以说：
    • 我已检查了此代码，且
    • 此代码有意义，且
    • 此代码正常工作且做了我们想做的一些事，且
    • 此代码不做任何我们不想它做的事，且
    • 此代码遵循我们的开发指引，且
    • 此代码在五年内仍有意义。

给予负面反馈

  • If the revision is trivial, broken and has no obvious value, mark the commit as "Code-Review -2"
  • If the revision has issues but also has some utility, or is showing signs of heading in the right direction, mark it Code-Review -1 and add a comment explaining what is wrong and how to fix it. Never mark something Code-Review -1 without adding a comment. If someone marks your code Code-Review -1 it means that your code is good, but needs improvement.

You have to weigh up the costs and benefits of each course of action. If you reject the change completely (Code-Review -2), then that change will be lost, and the developer may be discouraged. If you tolerate the fault, the end product will suffer. If you fix it yourself, then you're letting yourself get distracted, and perhaps you're making the developer believe that it is acceptable to submit low-quality code and then let someone else worry about the details.

General guidelines on comment style, especially when giving negative feedback:

  1. Focus your comments on the code and any objectively-observed behavior, not motivations; for example, don't state or imply assumptions about motivating factors like whether the developer was too lazy or unexperienced to do things right. Ask questions instead of making demands to foster a technical discussion: "What do you think about...?" "Did you consider...?" "Can you clarify...?"[3]
  2. Be empathetic and kind. Recognise that the developer has probably put a lot of work in their idea, and thank them for their contribution if you feel comfortable and sincere in doing so. "Why didn't you just..." provides a judgement, putting people on the defensive.[3] Be positive.
  3. Let them know where they can appeal your decision. For example, invite them to discuss the issue on mail:wikitech-l or on IRC.
  4. Be clear. Don't sugarcoat things so much that the central message is obscured.
  5. Most importantly, give the feedback quickly. Don't just leave negative feedback to someone else or hope they aren't persistent enough to get their contribution accepted.

联络

During the review you may have some questions or problems. Don't worry! We can try to help you.

For questions related to Wikimedia Gerrit and code review or specific patches, feel free to see the list of IRC Channels and choose a relevant one. See also the Communication page for additional platforms.

For example, for questions related to MediaWiki patches, feel free to join #mediawiki 在线.

If you have problems with the jenkins-bot, feel free to kindly contact #wikimedia-releng 在线.

参阅

参考

  1. ↑ Nemo_bis: "Number of reviewers"
  2. ↑ Sage Sharp：修補審查的溫柔藝術
  3. ↑ 3.0 3.1 Prior, D., "Cultivating a Code Review Culture" at RailsConf 2015
Retrieved from "https://www.mediawiki.org/w/index.php?title=Gerrit/Code_review/zh&oldid=8041788"