[Summary view] [Print] [Text view]

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