@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.)
hq.recaptime.dev/wiki/Phorge
phorge
phabricator
1@title User Guide: Review vs Audit
2@group userguide
3
4Discusses the differences between "review" and "audit" workflows.
5
6Overview
7========
8
9Phorge supports two similar but separate code review workflows: "review"
10and "audit".
11
12Review occurs in **Differential**, before changes are published. You can learn
13more in @{article:Differential User Guide}.
14
15Audit occurs in **Diffusion**, after changes are published. You can learn more
16in @{article:Audit User Guide}.
17
18When this documentation discusses "unpublished changes", it refers to changes
19which are still subject to being reworked in response to feedback. In many
20workflows, these changes will only exist locally on the developer's machine,
21but some workflows push tentative or temporary changes into remotes. The step
22that "publishes" changes might be either pushing or merging them, depending on
23your workflow.
24
25Both the audit and review workflows are lightweight, asynchronous web-based
26workflows where reviewers or auditors inspect code independently, from their
27own machines -- not synchronous review sessions where authors and reviewers
28meet in person to discuss changes.
29
30Broadly, review is normally a //blocking// workflow: in review workflows,
31authors usually can not publish changes until review completes and reviewers
32are satisfied.
33
34In contrast, audit is normally a //nonblocking// workflow: in audit workflows,
35changes usually move forward by default.
36
37Advantages of Review
38====================
39
40Pre-publish review is significantly more powerful than post-publish auditing.
41You gain these advantages by requiring review //before// changes may be
42published:
43
44 - Authors have a strong incentive to craft small, well-formed changes that
45 will be readily understood, to explain them adequately, and to provide
46 appropriate test plans, test coverage and context.
47 - Reviewers have a real opportunity to make significant suggestions about
48 architecture or approach in review. These suggestions are less attractive
49 to adopt from audit, and may be much more difficult to adopt if significant
50 time has passed between publish and audit.
51 - Authors have a strong incentive to fix problems and respond to feedback
52 received during review because it blocks them. Authors have a much weaker
53 incentive to promptly address problems raised during audit.
54 - Authors can ask reviewers to apply and verify fixes before they are
55 published.
56 - Authors can easily pursue feedback early, and get course corrections on
57 approach or direction.
58 - Reviewers are better prepared to support a given change once it is in
59 production, having already had a chance to become familiar with and reason
60 through the code.
61 - Reviewers are able to catch problems which automated tests may have
62 difficulty detecting. For example, human reviewers are able to reason about
63 performance problems that tests can easily miss because they run on
64 small datasets and stub out service calls.
65 - Communicating about changes //before// they happen generally leads to better
66 preparation for their effects.
67
68The theoretical cost of review is that it slows down development by introducing
69a blocking step into the process and generally wastes developer time that could
70be better spent developing. This is less true than it appears, because the costs
71are low and pay for themselves in other ways:
72
73 - Differential is fast and provides a lightweight process for submitting
74 code for review and for performing review.
75 - Authors are free to pursue other changes while code is being reviewed. With
76 appropriate change management (like local branching in Git) they can even
77 pursue dependent changes easily. Authors should rarely if ever be blocked on
78 review, even though an individual change is blocked until it is approved.
79 - The workflow as a whole is lightweight and, with skillful reviewers,
80 effective at identifying bugs. It is generally faster to fix bugs in review
81 than in production.
82 - More importantly, it is effective at identifying problems with architecture
83 and approach. These are free to fix in review ("don't do this, it is a bad
84 idea") and may be very time consuming to fix in production. No matter how
85 good your test suite is, it can't identify solutions which are poor because
86 of missing context, or miscommunication, or which are simply bad ideas.
87 - Changes which are too large or too complicated to be reviewed quickly are
88 often //too large and too complicated, period//. Nearly all large changes
89 can be split apart into small, independent pieces which are easier to
90 understand and test. Review tends to encourage smaller and better-factored
91 changes.
92 - Review can be integrated with static analysis which can detect (and,
93 in many cases, correct) mechanical problems with code like syntax,
94 formatting, naming conventions, style problems, misspellings, and some
95 program errors. This reduces the amount of time it takes to review code,
96 and means reviewers can focus on actual problems with the code rather than
97 minor stylistic issues.
98 - Review creates a permanent record of context and intent which explains why
99 a change was made, generally with much more information than commit messages
100 alone (authors have an incentive to properly explain a change when sending
101 it for review). This makes it easier to understand code later, and to
102 respond appropriately when it breaks.
103 - With `arc patch`, it is roughly as easy to pull a change out of Differential
104 as it is to pull it out of the remote.
105
106Advantages of Audit
107===================
108
109Post-publish audit is a less powerful workflow than pre-publish review, but can
110supplement review and is better than nothing on its own. If you are unpersuaded
111by the arguments above (or work on a team that is unswayed), audits provide
112some of the benefits of review with less friction:
113
114 - Audits are driven entirely by Phorge: users do not need to install
115 `arc`.
116 - Audits require little adjustment to existing workflows and little training.
117 - Audits are completely nonblocking, and send fewer notifications than review.
118 - Even if you have review, audits can be useful as a supplement to keep tabs
119 on lower-importance changes or raise issues that are discovered after
120 review.
121
122Recommendations
123===============
124
125Here are super biased recommendations from developers of code review software:
126
127 - If you can do review, do it. Supplement it with audits for less important
128 changes as your organization scales.
129 - If you can't do review immediately, set up audits and try to transition
130 toward review. Some types of changes (like tentative changes or requests
131 for feedback about code) are a naturally good fit for review and can serve
132 as a stepping stone toward broader acceptance. Greater familiarity with the
133 toolset may also foster more acceptance toward review, and the value of
134 review may become more obvious as the organization scales (e.g., once you
135 get interns).
136 - If you aren't interested in review, just do audits. You can always
137 change your mind later. But consider review! It's really good, we promise!
138
139Next Steps
140==========
141
142 - Learn more about reviews in @{article:Differential User Guide}; or
143 - learn more about audits in @{article:Audit User Guide}.