@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 Writing Reviewable Code
2@group review
3
4Project recommendations on how to structure changes.
5
6This document is purely advisory. Phorge works with a variety of revision
7control strategies, and diverging from the recommendations in this document
8will not impact your ability to use it for code review and source management.
9
10= Overview =
11
12This document describes a strategy for structuring changes used successfully at
13Facebook and in Phorge. In essence:
14
15 - Each commit should be as small as possible, but no smaller.
16 - The smallest a commit can be is a single cohesive idea: don't make commits
17 so small that they are meaningless on their own.
18 - There should be a one-to-one mapping between ideas and commits:
19 each commit should build one idea, and each idea should be implemented by
20 one commit.
21 - Turn large commits into small commits by dividing large problems into
22 smaller problems and solving the small problems one at a time.
23 - Write sensible commit messages.
24
25= Many Small Commits =
26
27Small, simple commits are generally better than large, complex commits. They are
28easier to understand, easier to test, and easier to review. The complexity of
29understanding, testing and reviewing a change often increases faster than its
30size: ten 200-line changes each doing one thing are often far easier to
31understand than one 2,000 line change doing ten things. Splitting a change which
32does many things into smaller changes which each do only one thing can decrease
33the total complexity associated with accomplishing the same goal.
34
35Each commit should do one thing. Generally, this means that you should separate
36distinct changes into different commits when developing. For example, if you're
37developing a feature and run into a preexisting bug, stash or checkpoint your
38change, check out a clean HEAD/tip, fix the bug in one change, and then
39merge/rebase your new feature on top of your bugfix so that you have two
40changes, each with one idea ("add feature x", "fix a bug in y"), not one change
41with two ideas ("add feature x and fix a bug in y").
42
43(In Git, you can do this easily with local feature branches and commands like
44`git rebase`, `git rebase -i`, and `git stash`, or with merges. In Mercurial,
45you can use bookmarks or the queues extension. In SVN, there are few builtin
46tools, but you can use multiple working copies or treat Differential like a
47stash you access with `arc patch`.)
48
49Even changes like fixing style problems should ideally be separated: they're
50accomplishing a different goal. And it is far easier to review one 300-line
51change which "converts tabs to spaces" plus one 30-line change which "implements
52feature z" than one 330-line change which "implements feature z and also
53converts a bunch of tabs to spaces".
54
55Similarly, break related but complex changes into smaller, simpler components.
56Here's a ridiculous analogy: if you're adding a new house, don't make one
575,000-line change which adds the whole house in one fell sweep. Split it apart
58into smaller steps which are each easy to understand: start with the foundation,
59then build the frame, etc. If you decided to dig the foundation with a shovel or
60build the frame out of cardboard, it's both easier to miss and harder to correct
61if the decisions are buried in 5,000 lines of interior design and landscaping.
62Do it one piece at a time, providing enough context that the larger problem
63can be understood but accomplishing no more with each step than you need to in
64order for it to stand on its own.
65
66The minimum size of a change should be a complete implementation of the simplest
67subproblem which works on its own and expresses an entire idea, not just part
68of an idea. You could mechanically split a 1,000-line change into ten 100-line
69changes by choosing lines at random, but none of the individual changes would
70make any sense and you would increase the collective complexity. The real goal
71is for each change to have minimal complexity, line size is just a proxy that is
72often well-correlated with complexity.
73
74We generally follow these practices in Phorge. The median change size for
75Phorge is 35 lines.
76
77= Write Sensible Commit Messages =
78
79There are lots of resources for this on the internet. All of them say pretty
80much the same thing; this one does too.
81
82The single most important thing is: **commit messages should explain //why// you
83are making the change**.
84
85Differential attempts to encourage the construction of sensible commit messages,
86but can only enforce structure, not content. Structurally, commit messages
87should probably:
88
89 - Have a title, briefly describing the change in one line.
90 - Have a summary, describing the change in more detail.
91 - Maybe have some other fields.
92
93The content is far more important than the structure. In particular, the summary
94should explain //why// you're making the change and //why// you're choosing the
95implementation you're choosing. The //what// of the change is generally
96well-explained by the change itself. For example, this is obviously an awful
97commit message:
98
99 COUNTEREXAMPLE
100 fix a bug
101
102But this one is almost as bad:
103
104 COUNTEREXAMPLE
105 Allow dots in usernames
106
107 Change the regexps so usernames can have dots in them.
108
109This is better than nothing but just summarizes information which can be
110inferred from the text of the diff. Instead, you should provide context and
111explain why you're making the change you're making, and why it's the right one:
112
113 lang=txt
114 Allow dots in usernames to support Google and LDAP auth
115
116 To prevent nonsense, usernames are currently restricted to A-Z0-9. Now that
117 we have Google and LDAP auth, a couple of installs want to allow "." too
118 since they have schemes like "abraham.lincoln@mycompany.com" (see Tnnn). There
119 are no technical reasons not to do this, so I opened up the regexps a bit.
120
121 We could mostly open this up more but I figured I'd wait until someone asks
122 before allowing "ke$ha", etc., because I personally find such names
123 distasteful and offensive.
124
125This information can not be extracted from the change itself, and is much more
126useful for the reviewer and for anyone trying to understand the change after the
127fact.
128
129== Referencing Other Objects ==
130
131An easy way to explain //why// is to reference other objects
132(bugs/issues/revisions) which motivate the change.
133
134Commit messages support special syntax you can use in your commit message to
135cause effects on related items:
136
137 - `Ref T123`: Attach a revision or a commit to a task. You can also specify
138 several tasks, optionally using commas: `Ref T123 T124` or `Ref T123, T124`.
139 - `Fixes T123`: Close a task when pushing a commit.
140 - `Reverts rXabcdef`: Mark a commit as reverting something.
141 - `Depends on D123`: Mark a revision as depending on another revision.
142
143Similar syntax exists for each status you may want to set on a related task.
144Define the syntax to cause an effect via the `prefixes` key of a task status
145configured via the configuration option `maniphest.statuses`.
146
147You can also attach a revision to a task `T123` by diffing it from a branch name
148like `T123-newfeature` with `arc`.
149
150== Test Plans ==
151
152Differential also includes a "Test Plan" field which is required by default.
153There is a detailed description of this field in @{article:Differential User
154Guide: Test Plans}. You can make it optional or disable it in the configuration,
155but consider adopting it. Having this information can be particularly helpful
156for reviewers.
157
158== Social Constructs ==
159
160Some things that people sometimes feel strongly about but which are probably not
161really all that important in commit messages include:
162
163 - If/where text is wrapped.
164 - Maximum length of the title.
165 - Whether there should be a period or not in the title.
166 - Use of voice/tense, e.g. "fix"/"add" vs "fixes"/"adds".
167 - Other sorts of pedantry not related to getting the context and
168 reasons //why// a change is happening into the commit message.
169 - Although maybe the spelling and grammar shouldn't be egregiously bad?
170
171Phorge does not have guidelines for this stuff. You can obviously set
172guidelines at your organization if you prefer, but getting the //why// into the
173message is the most important part.
174
175= Next Steps =
176
177Continue by:
178
179 - reading recommendations on structuring revision control with
180 @{article:Recommendations on Revision Control}; or
181 - reading recommendations on structuring branches with
182 @{article:Recommendations on Branching}.