Page MenuHomestyx hydra
Files F228856

No OneTemporary
Actions

View Options

diff --git a/src/docs/flavortext/writing_reviewable_code.diviner b/src/docs/flavortext/writing_reviewable_code.diviner
index c0f8b59ec2..80e381e5db 100644
--- a/src/docs/flavortext/writing_reviewable_code.diviner
+++ b/src/docs/flavortext/writing_reviewable_code.diviner
@@ -1,160 +1,163 @@
@title Writing Reviewable Code
@group flavortext
Project recommendations on how to structure changes.
This document is purely advisory. Phabricator works with a variety of revision
control strategies, and diverging from the recommendations in this document
will not impact your ability to use it for code review and source management.
= Overview =
This document describes a strategy for structuring changes used successfully at
Facebook and in Phabricator. In essence:
- Each commit should be as small as possible, but no smaller.
- The smallest a commit can be is a single cohesive idea: don't make commits
so small that they are meaningless on their own.
- There should be a one-to-one mapping between ideas and commits:
each commit should build one idea, and each idea should be implemented by
one commit.
- Turn large commits into small commits by dividing large problems into
smaller problems and solving the small problems one at a time.
- Write sensible commit messages.
= Many Small Commits =
Small, simple commits are generally better than large, complex commits. They are
easier to understand, easier to test, and easier to review. The complexity of
understanding, testing and reviewing a change often increases faster than its
size: ten 200-line changes each doing one thing are often far easier to
understand than one 2,000 line change doing ten things. Splitting a change which
does many things into smaller changes which each do only one thing can decrease
the total complexity associated with accomplishing the same goal.
Each commit should do one thing. Generally, this means that you should separate
distinct changes into different commits when developing. For example, if you're
developing a feature and run into a preexisting bug, stash or checkpoint your
change, check out a clean HEAD/tip, fix the bug in one change, and then
merge/rebase your new feature on top of your bugfix so that you have two
changes, each with one idea ("add feature x", "fix a bug in y"), not one change
with two ideas ("add feature x and fix a bug in y").
(In Git, you can do this easily with local feature branches and commands like
`git rebase`, `git rebase -i`, and `git stash`, or with merges. In Mercurial,
you can use bookmarks or the queues extension. In SVN, there are few builtin
tools, but you can use multiple working copies or treat Differential like a
stash you access with `arc patch`.)
Even changes like fixing style problems should ideally be separated: they're
accomplishing a different goal. And it is far easier to review one 300-line
change which "converts tabs to spaces" plus one 30-line change which "implements
feature z" than one 330-line change which "implements feature z and also
converts a bunch of tabs to spaces".
Similarly, break related but complex changes into smaller, simpler components.
Here's a ridiculous analogy: if you're adding a new house, don't make one
5,000-line change which adds the whole house in one fell sweep. Split it apart
into smaller steps which are each easy to understand: start with the foundation,
then build the frame, etc. If you decided to dig the foundation with a shovel or
build the frame out of cardboard, it's both easier to miss and harder to correct
if the decisions are buried in 5,000 lines of interior design and landscaping.
Do it one piece at a time, providing enough context that the larger problem
can be understood but accomplishing no more with each step than you need to in
order for it to stand on its own.
The minimum size of a change should be a complete implementation of the simplest
subproblem which works on its own and expresses an entire idea, not just part
of an idea. You could mechanically split a 1,000-line change into ten 100-line
changes by choosing lines at random, but none of the individual changes would
make any sense and you would increase the collective complexity. The real goal
is for each change to have minimal complexity, line size is just a proxy that is
often well-correlated with complexity.
We generally follow these practices in Phabricator. The median change size for
Phabricator is 35 lines.
+See @{article:Differential User Guide: Large Changes} for information about
+reviewing big checkins.
+
= Write Sensible Commit Messages =
There are lots of resources for this on the internet. All of them say pretty much
the same thing; this one does too.
The single most important thing is: **commit messages should explain //why// you
are making the change**.
Differential attempts to encourage the construction of sensible commit messages,
but can only enforce structure, not content. Structurally, commit messages
should probably:
- Have a title, briefly describing the change in one line.
- Have a summary, describing the change in more detail.
- Maybe have some other fields.
The content is far more important than the structure. In particular, the summary
should explain //why// you're making the change and //why// you're choosing the
implementation you're choosing. The //what// of the change is generally
well-explained by the change itself. For example, this is obviously an awful
commit message:
COUNTEREXAMPLE
fix a bug
But this one is almost as bad:
COUNTEREXAMPLE
Allow dots in usernames
Change the regexps so usernames can have dots in them.
This is better than nothing but just summarizes information which can be
inferred from the text of the diff. Instead, you should provide context and
explain why you're making the change you're making, and why it's the right one:
lang=txt
Allow dots in usernames to support Google and LDAP auth
To prevent nonsense, usernames are currently restricted to A-Z0-9. Now that
we have Google and LDAP auth, a couple of installs want to allow "." too
since they have schemes like "abraham.lincoln@mycompany.com" (see Tnnn). There
are no technical reasons not to do this, so I opened up the regexps a bit.
We could mostly open this up more but I figured I'd wait until someone asks
before allowing "ke$ha", etc., because I personally find such names
distasteful and offensive.
This information can not be extracted from the change itself, and is much more
useful for the reviewer and for anyone trying to understand the change after the
fact.
An easy way to explain //why// is to reference other objects
(bugs/issues/revisions) which motivate the change.
Differential also includes a "Test Plan" field which is required by default.
There is a detailed description of this field in @{article:Differential User
Guide: Test Plans}. You can make it optional or disable it in the configuration,
but consider adopting it. Having this information can be particularly helpful
for reviewers.
Some things that people sometimes feel strongly about but which are probably not
really all that important in commit messages include:
- If/where text is wrapped.
- Maximum length of the title.
- Whether there should be a period or not in the title.
- Use of voice/tense, e.g. "fix"/"add" vs "fixes"/"adds".
- Other sorts of pedantry not related to getting the context and
reasons //why// a change is happening into the commit message.
- Although maybe the spelling and grammar shouldn't be egregiously bad?
Phabricator does not have guidelines for this stuff. You can obviously set
guidelines at your organization if you prefer, but getting the //why// into the
message is the most important part.
= Next Steps =
Continue by:
- reading recommendations on structuring revision control with
@{article:Recommendations on Revision Control}; or
- reading recommendations on structuring branches with
@{article:Recommendations on Branching}.
\ No newline at end of file
diff --git a/src/docs/userguide/differential_large_changes.diviner b/src/docs/userguide/differential_large_changes.diviner
index d9a58237a3..5d9f478297 100644
--- a/src/docs/userguide/differential_large_changes.diviner
+++ b/src/docs/userguide/differential_large_changes.diviner
@@ -1,54 +1,54 @@
@title Differential User Guide: Large Changes
@group userguide
Dealing with huge changesets, and when **not** to use Differential.
= Overview =
When you want code review for a given changeset, Differential is not always the
right tool to use. The rule of thumb is that you should only send changes to
Differential if you expect humans to review the actual differences in the source
code from the web interface. This should cover the vast majority of changes but,
for example, you usually should //not// submit changes like these through
Differential:
- Committing an entire open source project to a private repo somewhere so
you can fork it or link against it.
- Committing an enormous text datafile, like a list of every English word or a
dump of a database.
- - Making a trivial (e.g., find/replace) edit to 10,000 files.
+ - Making a trivial (e.g., find/replace or codemod) edit to 10,000 files.
You can still try submitting these kinds of changes, but you may encounter
problems getting them to work (database or connection timeouts, for example).
Differential is pretty fast and scalable, but at some point either it or the
browser will break down: you simply can't show nine million files on a webpage.
More importantly, in all these cases, the text of the changes won't be reviewed
by a human. The metadata associated with the change is what needs review (e.g.,
what are you checking in, where are you putting it, and why? Does the change
make sense? In the case of automated transformations, what script did you use?).
To get review for these types of changes, one of these strategies will usually
work better than trying to get the entire change into Differential:
- Send an email/AIM/IRC to your reviewer(s) like "Hey, I'm going to check in
the source for MySQL 9.3.1 to /full/path/to/whatever. The change is staged
in /home/whatever/path/somewhere if you want to take a look. Can I put your
name on the review?". This is best for straightforward changes. The reviewer
is not going to review MySQL's source itself, instead they are reviewing the
change metadata: which version are you checking in, why are you checking it
- in, and where are you putting it?
+ in, and where are you putting it? You won't be able to use "arc commit" or
+ "arc amend" to actually push the change. Just use "svn" or "git" and
+ manually edit the commit message instead. (It is normally sufficient to add
+ a "Reviewed By: <username>" field.)
- Create a Differential revision with only the metadata, like the script you
used to make automated changes or a text file explaining what you're doing,
and maybe a sample of some of the changes if they were automated. Include a
link to where the changes are staged so reviewers can look at the actual
changeset if they want to. This is best for more complicated changes, since
Differential can still be used for discussion and provide a permanent record
- others can refer to.
-
-In both cases, you won't be able to use "arc commit" or "arc amend" to actually
-push the change. Just use "svn" or "git" and manually edit the commit message
-instead. (It is normally sufficient to add a "Reviewed By: <username>" field.)
+ others can refer to. Once the revision is accepted, amend your local commit
+ (e.g. by `git commit --amend`) with the real change and push as usual.
These kinds of changes are generally rare and don't have much in common, which
is why there's no explicit support for them in Differential. If you frequently
run into cases which Differential doesn't handle, let us know what they are.

File Metadata

Mime Type
text/x-diff
Expires
Thu, Jul 24, 2:15 PM (1 d, 13 h)
Storage Engine
blob
Storage Format
Raw Data
Storage Handle
182739
Default Alt Text
(11 KB)

Event Timeline

styx copyright 2020 sirocyl / ellie (kymkdd) / ncommander / styx contributors
Use of other names and brands is not intended to reflect a claim of, or right to, use the name or brand.
styx is an operating system using the Linux kernel and components, and "styx Linux" as a phrase does not reflect the inclusion of the Linux brand or trademark in the styx project.
Linux® is the registered trademark of Linus Torvalds in the U.S. and other countries.
Terms and Conditions (TODO: fix this!)