← Back to branch summary

~bzr-pqm/bzr/bzr.dev

« back to all changes in this revision

Viewing changes to doc/developers/code-review.txt

  • Committer: Martin Pool
  • Date: 2010-05-14 13:34:22 UTC
  • mto: This revision was merged to the branch mainline in revision 5252.
  • Revision ID: mbp@canonical.com-20100514133422-zcr5tt9vwy5ww1jj
Clean up and improve code review and contribution guidelines. 

Detangle "how to contribute code" from "how to review contributions".

Show diffs side-by-side

added added

removed removed

Lines of Context:
doc/developers/code-review.txt
1
 
The Code Review Process
2
 
#######################
3
 
 
4
 
All code changes coming in to Bazaar are reviewed by someone else.
 
1
Reviewing proposed changes to Bazaar
 
2
####################################
 
3
 
 
4
All non-trivial code changes coming in to Bazaar are reviewed by someone else.
 
5
 
 
6
Anyone is welcome to review any patch.  You don't need to have a full 
 
7
understanding of the codebase to find problems in the code, the documentation,
 
8
or the concept of the patch.
 
9
 
5
10
Normally changes by core contributors are reviewed by one other core
6
11
developer, and changes from other people are reviewed by two core
7
 
developers.  Use intelligent discretion if the patch is trivial.
8
 
 
9
 
Good reviews do take time. They also regularly require a solid
10
 
understanding of the overall code base. In practice, this means a small
11
 
number of people often have a large review burden - with knowledge comes
12
 
responsibility. No one likes their merge requests sitting in a queue going
13
 
nowhere, so reviewing sooner rather than later is strongly encouraged.
14
 
 
15
 
 
16
 
 
17
 
Review cover letters
18
 
====================
19
 
 
20
 
Please put a "cover letter" on your merge request explaining:
21
 
 
22
 
* the reason **why** you're making this change
23
 
 
24
 
* **how** this change achieves this purpose
25
 
 
26
 
* anything else you may have fixed in passing
27
 
 
28
 
* anything significant that you thought of doing, such as a more
29
 
  extensive fix or a different approach, but didn't or couldn't do now
30
 
 
31
 
A good cover letter makes reviewers' lives easier because they can decide
32
 
from the letter whether they agree with the purpose and approach, and then
33
 
assess whether the patch actually does what the cover letter says.
34
 
Explaining any "drive-by fixes" or roads not taken may also avoid queries
35
 
from the reviewer.  All in all this should give faster and better reviews.
36
 
Sometimes writing the cover letter helps the submitter realize something
37
 
else they need to do.  The size of the cover letter should be proportional
38
 
to the size and complexity of the patch.
 
12
developers.  Use intelligent discretion about whether if the patch is trivial.
 
13
 
 
14
No one likes their merge requests sitting in a queue going nowhere: this 
 
15
is pure waste.  We prioritize reviewing existing proposals.  
 
16
Canonical dedicates some staff time to providing prompt helpful reviews.  
 
17
(See <http://wiki.bazaar.canonical.com/PatchPilot/>.)
 
18
 
 
19
From late 2009 on, we do all our code reviews through Launchpad's 
 
20
merge proposal interface.
39
21
 
40
22
 
41
23
Reviewing proposed changes
42
24
==========================
43
25
 
44
 
Anyone is welcome to review code, and reply to the thread with their
45
 
opinion or comments.
46
 
 
47
 
The simplest way to review a proposed change is to just read the patch on
48
 
the list or in Bundle Buggy.  For more complex changes it may be useful
49
 
to make a new working tree or branch from trunk, and merge the proposed
50
 
change into it, so you can experiment with the code or look at a wider
51
 
context.
52
 
 
53
26
There are three main requirements for code to get in:
54
27
 
55
28
* Doesn't reduce test coverage: if it adds new methods or commands,
72
45
 
73
46
It is easy for reviews to make people notice other things which should be
74
47
fixed but those things should not hold up the original fix being accepted.
75
 
New things can easily be recorded in the Bug Tracker instead.
 
48
New things can easily be recorded in the bug tracker instead.
76
49
 
77
50
It's normally much easier to review several smaller patches than one large
78
51
one.  You might want to use ``bzr-loom`` to maintain threads of related
109
82
Reviews on Launchpad
110
83
====================
111
84
 
112
 
From May 2009 on, we prefer people to propose code reviews through
113
 
Launchpad.
114
 
 
115
 
 * <https://launchpad.net/+tour/code-review>
116
 
 
117
 
 * <https://help.launchpad.net/Code/Review>
118
 
 
119
85
Anyone can propose or comment on a merge proposal just by creating a
120
86
Launchpad account.
121
87
 
122
 
There are two ways to create a new merge proposal: through the web
123
 
interface or by email.
124
 
 
125
 
 
126
 
Proposing a merge through the web
127
 
---------------------------------
128
 
 
129
 
To create the proposal through the web, first push your branch to Launchpad.
130
 
For example, a branch dealing with documentation belonging to the Launchpad
131
 
User mbp could be pushed as ::
132
 
 
133
 
  bzr push lp:~mbp/bzr/doc
134
 
 
135
 
Then go to the branch's web page, which in this case would be
136
 
<https://code.launchpad.net/~mbp/bzr/doc>.  You can simplify this step by just
137
 
running ::
138
 
 
139
 
  bzr lp-open
140
 
 
141
 
You can then click "Propose for merging into another branch", and enter your
142
 
cover letter (see above) into the web form.  Typically you'll want to merge
143
 
into ``~bzr/bzr/trunk`` which will be the default; you might also want to
144
 
nominate merging into a release branch for a bug fix.  There is the option to
145
 
specify a specific reviewer or type of review, and you shouldn't normally
146
 
change those.
147
 
 
148
 
Submitting the form takes you to the new page about the merge proposal
149
 
containing the diff of the changes, comments by interested people, and
150
 
controls to comment or vote on the change.
151
 
 
152
 
Proposing a merge by mail
153
 
-------------------------
154
 
 
155
 
To propose a merge by mail, send a bundle to ``merge@code.launchpad.net``.
156
 
 
157
 
You can generate a merge request like this::
158
 
 
159
 
  bzr send -o bug-1234.diff
160
 
 
161
 
``bzr send`` can also send mail directly if you prefer; see the help.
162
 
 
163
 
Reviewing changes
164
 
-----------------
165
 
 
166
88
From <https://code.launchpad.net/bzr/+activereviews> you can see all
167
89
currently active reviews, and choose one to comment on.  This page also
168
90
shows proposals that are now approved and should be merged by someone with
169
91
PQM access.
170
92
 
 
93
<https://help.launchpad.net/Code/Review> explains the various merge proposal
 
94
states.  Note that we don't use state *Approved* until the patch is completely
 
95
ready to merge.
 
96
 
 
97
 
 
98
Landing approved changes
 
99
========================
 
100
 
 
101
Once a merge proposal is approved and finished, it's sent to PQM (the patch
 
102
queue manager) which will automatically test and integrate it.  The recommended
 
103
way to start this off is by running the ``feed-pqm`` script from 
 
104
<https://launchpad.net/hydrazine/>.