245 lines
10 KiB
Plaintext
245 lines
10 KiB
Plaintext
Gerrit Code Review - Contributing
|
|
=================================
|
|
|
|
Gerrit is developed as a self-hosting open source project and
|
|
very much welcomes contributions from anyone with a contributor's
|
|
agreement on file with the project.
|
|
|
|
* https://gerrit-review.googlesource.com/
|
|
|
|
The Contributor License Agreements:
|
|
|
|
* https://gerrit-review.googlesource.com/static/cla_individual.html
|
|
* https://gerrit-review.googlesource.com/static/cla_corporate.html
|
|
|
|
As Gerrit is a code review tool, naturally contributions will
|
|
be reviewed before they will get submitted to the code base. To
|
|
start your contribution, please make a git commit and upload it
|
|
for review to the main Gerrit review server. To help speed up the
|
|
review of your change, review these guidelines before submitting
|
|
your change. You can view the pending Gerrit contributions and
|
|
their statuses here:
|
|
|
|
* https://gerrit-review.googlesource.com/#/q/status:open+project:gerrit,n,z
|
|
|
|
Depending on the size of that list it might take a while for
|
|
your change to get reviewed. Naturally there are fewer
|
|
approvers than contributors; so anything that you can do to
|
|
ensure that your contribution will undergo fewer revisions
|
|
will speed up the contribution process. This includes helping
|
|
out reviewing other people's changes to relieve the load from
|
|
the approvers. Even if you are not familiar with Gerrit's
|
|
internals, it would be of great help if you can download, try
|
|
out, and comment on new features. If it works as advertised,
|
|
say so, and if you have the priviliges to do so, go ahead
|
|
and give it a +1 Verified. If you would find the feature
|
|
useful, say so and give it a +1 code review.
|
|
|
|
And finally, the quicker you respond to the comments of your
|
|
reviewers, the quicker your change might get merged! Try to
|
|
reply to every comment after submitting your new patch,
|
|
particularly if you decided against making the suggested change.
|
|
Reviewers don't want to seem like nags and pester you if you
|
|
haven't replied or made a fix, so it helps them know if you
|
|
missed it or decided against it.
|
|
|
|
|
|
Review Criteria
|
|
---------------
|
|
|
|
Here are some hints as to what approvers may be looking for
|
|
before approving or submitting changes to the Gerrit project.
|
|
Let's start with the simple nit picky stuff. You are likely
|
|
excited that your code works; help us share your excitement
|
|
by not distracting us with the simple stuff. Thanks to Gerrit,
|
|
problems are often highlighted and we find it hard to look
|
|
beyond simple spacing issues. Blame it on our short attention
|
|
spans, we really do want your code.
|
|
|
|
|
|
Commit Message
|
|
--------------
|
|
|
|
It is essential to have a good commit message if you want your
|
|
change to be reviewed.
|
|
|
|
* Keep lines no longer than 72 chars
|
|
* Start with a short one line summary
|
|
* Followed by a blank line
|
|
* Followed by one or more explanatory paragraphs
|
|
* Use the present tense (fix instead of fixed)
|
|
* Include a Bug: Issue <#> line if fixing a Gerrit issue
|
|
* Include a Change-Id line
|
|
|
|
|
|
A sample good Gerrit commit message:
|
|
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
|
|
====
|
|
Add sample commit message to guidelines doc
|
|
|
|
The original patch set for the contributing guidelines doc did not
|
|
include a sample commit message, this new patchset does. Hopefully this
|
|
makes things a bit clearer since examples can sometimes help when
|
|
explanations don't.
|
|
|
|
Note that the body of this commit message can be several paragraphs, and
|
|
that I word wrap it at 72 characters. Also note that I keep the summary
|
|
line under 50 characters since it is often truncated by tools which
|
|
display just the git summary.
|
|
|
|
Bug: Issue 98765605
|
|
Change-Id: Ic4a7c07eeb98cdeaf44e9d231a65a51f3fceae52
|
|
====
|
|
|
|
|
|
Style
|
|
-----
|
|
|
|
The basic coding style is covered by the tools/GoogleFormat.xml
|
|
doc, see the link:dev-eclipse.html#Formatting[Eclipse Setup]
|
|
for that.
|
|
|
|
Highlighted/additional styling notes:
|
|
|
|
* It is generally more important to match the style of the nearby
|
|
code which you are modifying than it is to match the style
|
|
in the formatting guidelines. This is especially true within the
|
|
same file.
|
|
* Review your change in Gerrit to see if it highlights
|
|
mistakingly deleted/added spaces on lines, trailing spaces.
|
|
* Line length should be 80 or less, unless the code reads
|
|
better with something slightly longer. Shorter lines not only
|
|
help reviewers who may use a tablet to review the code, but future
|
|
contributors may also like to open several editors side by
|
|
side while editing new changes.
|
|
* Use 2 spaces for indent (no tabs)
|
|
* Use brackets in all ifs, spaces before/after if parens.
|
|
* Use /** */ style Javadocs for variables.
|
|
|
|
Additionally, you will notice that most of the newline spacing
|
|
is fairly consistent throughout the code in Gerrit, it helps to
|
|
stick to the blank line conventions. Here are some specific
|
|
examples:
|
|
|
|
* Keep a blank line between all class and method declarations.
|
|
* Do not add blank lines at the beginning or end of class/methods.
|
|
* Put a blank line between external import sources, but not
|
|
between internal ones.
|
|
|
|
|
|
Code Organization
|
|
-----------------
|
|
|
|
Do your best to organize classes and methods in a logical way.
|
|
Here are some guidelines that Gerrit uses:
|
|
|
|
* Ensure a standard copyright header is included at the top
|
|
of any new files (copy it from another file, update the year).
|
|
* Always place loggers first in your class!
|
|
* Define any static interfaces next in your class.
|
|
* Define non static interfaces after static interfaces in your
|
|
class.
|
|
* Next you should define static types and members.
|
|
* Finally instance members, then constuctors, and then instance
|
|
methods.
|
|
* Some common exceptions are private helper static methods which
|
|
might appear near the instance methods which they help.
|
|
* Getters and setters for the same instance field should usually
|
|
be near each other baring a good reason not to.
|
|
* If you are using assisted injection, the factory for your class
|
|
should be before the instance members.
|
|
* Annotations should go before language keywords (final, private...) +
|
|
Example: @Assisted @Nullable final type varName
|
|
* Imports should be mostly alphabetical (uppercase sorts before
|
|
all lowercase, which means classes come before packages at the
|
|
same level).
|
|
|
|
Wow that's a lot! But don't worry, you'll get the habit and most
|
|
of the code is organized this way already; so if you pay attention
|
|
to the class you are editing you will likely pick up on it.
|
|
Naturally new classes are a little harder; you may want to come
|
|
back and consult this section when creating them.
|
|
|
|
|
|
Design
|
|
------
|
|
|
|
Here are some design level objectives that you should keep in mind
|
|
when coding:
|
|
|
|
* ORM entity objects should match exactly one row in the database.
|
|
* Most client pages should perform only one RPC to load so as to
|
|
keep latencies down. Exceptions would apply to RPCs which need
|
|
to load large data sets if splitting them out will help the
|
|
page load faster. Generally page loads are expected to complete
|
|
in under 100ms. This will be the case for most operations,
|
|
unless the data being fetched is not using Gerrit's caching
|
|
infrastructure. In these slower cases, it is worth considering
|
|
mitigating this longer load by using a second RPC to fill in
|
|
this data after the page is displayed (or alternatively it might
|
|
be worth proposing caching this data).
|
|
* @Inject should be used on constructors, not on fields. The
|
|
current exceptions are the ssh commands, these were implemented
|
|
earlier in Gerrit's development. To stay consistent, new ssh
|
|
commands should follow this older pattern; but eventually these
|
|
should get converted to eliminate this exception.
|
|
* Don't leave repository objects (git or schema) open. A .close()
|
|
after every open should be placed in a finally{} block.
|
|
* Don't leave UI components, which can cause new actions to occur,
|
|
enabled during RPCs which update the DB. This is to prevent
|
|
people from submitting actions more than once when operating
|
|
on slow links. If the action buttons are disabled, they cannot
|
|
be resubmitted and the user can see that Gerrit is still busy.
|
|
* GWT EventBus is the new way forward.
|
|
* ...and so is Guava (previously known as Google Collections).
|
|
|
|
|
|
Tests
|
|
-----
|
|
|
|
* Tests for new code will greatly help your change get approved.
|
|
|
|
|
|
Change Size/Number of Files Touched
|
|
-----------------------------------
|
|
|
|
And finally, I probably cannot say enough about change sizes.
|
|
Generally, smaller is better, hopefully within reason. Do try to
|
|
keep things which will be confusing on their own together,
|
|
especially if changing one without the other will break something!
|
|
|
|
* If a new feature is implemented and it is a larger one, try to
|
|
identify if it can be split into smaller logical features; when
|
|
in doubt, err on the smaller side.
|
|
* Separate bug fixes from feature improvements. The bug fix may
|
|
be an easy candidate for approval and should not need to wait
|
|
for new features to be approved. Also, combining the two makes
|
|
reviewing harder since then there is no clear line between the
|
|
fix and the feature.
|
|
* Separate supporting refactoring from feature changes. If your
|
|
new feature requires some refactoring, it helps to make the
|
|
refactoring a separate change which your feature change
|
|
depends on. This way, reviewers can easily review the refactor
|
|
change as a something that should not alter the current
|
|
functionality, and feel more confident they can more easily
|
|
spot errors this way. Of course, it also makes it easier to
|
|
test and locate later on if an unfortunate error does slip in.
|
|
Lastly, by not having to see refactoring changes at the same
|
|
time, it helps reviewers understand how your feature changes
|
|
the current functionality.
|
|
* Separate logical features into separate changes. This
|
|
is often the hardest part. Here is an example: when adding a
|
|
new ability, make separate changes for the UI and the ssh
|
|
commands if possible.
|
|
* Do only what the commit message describes. In other words, things which
|
|
are not strictly related to the commit message shouldn't be part of
|
|
a change, even trivial things like externalizing a string somewhere
|
|
or fixing a typo. This help keep "git blame" more useful in the future
|
|
and it also makes "git revert" more useful.
|
|
* Use topic branches to link your separate changes together.
|
|
|
|
|
|
GERRIT
|
|
------
|
|
Part of link:index.html[Gerrit Code Review]
|