blob: 6150c2089062196be0e6bc349ea47935e457dba3 [file] [log] [blame]
= Gerrit Code Review - Crafting Changes
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.
== Branch
Gerrit provides support for more than one version, which naturally
raises the question of which branch you should start your contribution
on. There are no hard and fast rules, but below we try to outline some
* Genuinely new and/or disruptive features, should generally start on
`master`. Also consider submitting a
link:dev-design-docs.html[design doc] beforehand to allow discussion
by the ESC and the community.
* Improvements of existing features should also generally go into
`master`. But we understand that if you cannot run `master`, it
might take a while until you could benefit from it. In that case,
implement the feature on master and, if you really need it on an
earlier `stable-*` branch, cherry-pick the change and build
Gerrit in your own environment.
* Bug-fixes should generally at least cover the oldest affected and
still supported version. If you're affected and run an even older
version, you're welcome to upload to that older version, even if
it is no longer officially supported, bearing in mind that
verification and release may happen only once merged upstream.
Regardless of the above, changes might get moved to a different branch
before being submitted or might get cherry-picked/re-merged to a
different branch even after they've landed.
For each of the above items, you'll find ad-hoc exceptions. The point
is: We'd much rather see your code and fixes than not see them.
== 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)
* Use the past tense when describing the status before this commit
* Include a `Bug: Issue <#>` line if fixing a Gerrit issue, or a
`Feature: Issue <#>` line if implementing a feature request.
* Include a `Change-Id` line
=== Setting up Vim for Git commit message
Git uses Vim as the default commit message editor. Put this into your
`$HOME/.vimrc` file to configure Vim for Git commit message formatting
and writing:
" Enable spell checking, which is not on by default for commit messages.
au FileType gitcommit setlocal spell
" Reset textwidth if you've previously overridden it.
au FileType gitcommit setlocal textwidth=72
=== 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
The `Change-Id` line is, as usual, created by a local git hook. To install it,
simply copy it from the checkout and make it executable:
cp ./gerrit-server/src/main/resources/com/google/gerrit/server/tools/root/hooks/commit-msg .git/hooks/
chmod +x .git/hooks/commit-msg
If you are working on core plugins, you will also need to install the
same hook in the submodules:
export hook=$(pwd)/.git/hooks/commit-msg
git submodule foreach 'cp -p "$hook" "$(git rev-parse --git-dir)/hooks/"'
To set up git's remote for easy pushing, run the following:
git remote add gerrit
The HTTPS access requires proper username and password; this can be obtained
by clicking the 'Obtain Password' link on the
Password tab of the user settings page,role=external,window=_blank].
Alternately, you may use the
link:[git-review,role=external,window=_blank] tool to submit changes
to Gerrit. If you do, it will set up the Change-Id hook and `gerrit` remote
for you. You will still need to do the HTTP access step.
== Style
This project has a policy of Eclipse's warning free code. Eclipse
configuration is added to git and we expect the changes to be
warnings free.
We do not ask you to use Eclipse for editing, obviously. We do ask you
to provide Eclipse's warning free patches only. If for some reasons, you
are not able to set up Eclipse and verify, that your patch hasn't
introduced any new Eclipse warnings, mention this in a comment to your
change, so that reviewers will do it for you. Yes, the way to go is to
extend gerrit CI to take care of this, but it's not yet implemented.
Gerrit follows the
link:[Google Java Style
To format Java source code, Gerrit uses the
tool (version 1.7), and to format Bazel BUILD, WORKSPACE and .bzl files the
tool (version 4.0.0). Unused dependencies are found and removed using the
build tool, a sibling of `buildifier`.
These tools automatically apply format according to the style guides; this
streamlines code review by reducing the need for time-consuming, tedious,
and contentious discussions about trivial issues like whitespace.
You may download and run `google-java-format` on your own, or you may
run `./tools/` to download a local copy and set up a
wrapper script. If you run your own copy, please use the same version,
as there may be slight differences between versions.
== Code Rules
=== Final
When to use `final` modifier and when not (in new code):
* final fields: marking fields as final forces them to be
initialized in the constructor or at declaration
* final static fields: clearly communicates the intent
* to use final variables in inner anonymous classes
* final classes: use when appropriate, e.g. API restriction
* final methods: similar to final classes
* local variables: it clutters the code, and makes the code less
readable. When copying old code to new location, finals should
be removed
* method parameters: similar to local variables
=== Optional / Nullable
* Optionals in arguments are discouraged (use @Nullable instead)
* Return types should be objects or Optionals of objects, but not null/nullable
== 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
* Next you should define static types, static members, and
static methods, in decreasing order of visibility (public to private).
* Finally instance types, instance members, then constructors,
and then instance methods.
* Some common exceptions are private helper static methods, which
might appear near the instance methods which they help (but may
also appear at the top).
* Getters and setters for the same instance field should usually
be near each other barring 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`, etc) +
Example: `@Assisted @Nullable final type varName`
* Prefer to open multiple AutoCloseable resources in the same
try-with-resources block instead of nesting the try-with-resources
blocks and increasing the indentation level more than necessary.
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:
* 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. Use a
try-with-resources statement to ensure that repository objects get
closed after use.
* Don't leave UI components, which can cause new actions to occur,
enabled during RPCs which update Git repositories, including NoteDb.
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
== Tests
* Tests for new code will greatly help your change get approved.
== Javadoc
* Javadocs for new code (especially public classes and
public/protected methods) will greatly help your change get
== 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 helps keep `git blame` more useful in the future
and it also makes `git revert` more useful.
* Use topics to link your separate changes together.
== Opportunistic Refactoring
Opportunistic Refactoring is a terminology
link:[used by Martin Fowler,role=external,window=_blank]
also known as the "boy scout rule" of the software developer:
"always leave the code behind in a better state than you found it."
In practice, this rule means you should not add technical debt in the code while
implementing a new feature or fixing a bug. If you or a reviewer find an
opportunity to clean up the code during implementation or review of your change,
take the time to do a little cleanup to improve the overall code base.
When approaching refactoring, keep in mind that changes should do one thing
(<<change-size,see change size section above>>). If a change you're making
requires cleanup/refactoring, it is best to do that cleanup in a preparatory and
separate change. Likewise, if during review for a functional change, an
opportunity for cleanup/refactoring is discovered, then it is preferable to do
the cleanup first in a separate change so as to improve the reviewability of the
functional change.
Reviewers should keep in mind the scope of the change under review and ensure
suggested refactoring is aligned with that scope.
Part of link:index.html[Gerrit Code Review]