373 lines
15 KiB
Plaintext
373 lines
15 KiB
Plaintext
= Gerrit Code Review - Contributing
|
|
|
|
== Introduction
|
|
Gerrit is developed as a
|
|
link:https://gerrit-review.googlesource.com/[self-hosting open source project]
|
|
and very much welcomes contributions from anyone with a contributor's
|
|
agreement on file with the project.
|
|
|
|
== Contributor License Agreement
|
|
A Contributor License Agreement must be completed before contributions
|
|
are accepted. To view and accept the agreements do the following:
|
|
|
|
* Click 'Sign In' at the top right corner of https://gerrit-review.googlesource.com/
|
|
* Sign In with your Google account
|
|
* After signing in, go to the
|
|
link:https://gerrit-review.googlesource.com/#/settings/agreements[Agreements]
|
|
tab on the settings page
|
|
* Click 'New Contributor Agreement' and follow the instructions
|
|
|
|
For reference, the actual agreements are linked below
|
|
|
|
* link:https://cla.developers.google.com/about/android-individual[Individual Agreement]
|
|
* link:https://source.android.com/source/cla-corporate.pdf[Corporate Agreement]
|
|
|
|
== Code Review
|
|
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
|
|
link:https://gerrit-review.googlesource.com/#/q/status:open+project:gerrit[here].
|
|
|
|
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 privileges 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]]
|
|
=== 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
|
|
* 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 https://gerrit.googlesource.com/gerrit
|
|
====
|
|
|
|
The HTTPS access requires proper username and password; this can be obtained
|
|
by clicking the 'Obtain Password' link on the
|
|
link:https://gerrit-review.googlesource.com/#/settings/http-password[HTTP
|
|
Password tab of the user settings page].
|
|
|
|
|
|
=== 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
|
|
mistakenly 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.
|
|
|
|
When to use `final` modifier and when not (in new code):
|
|
|
|
Always:
|
|
|
|
* final fields: marking fields as final forces them to be
|
|
initialised in the constructor or at declaration
|
|
* final static fields: clearly communicates the intent
|
|
* to use final variables in inner anonymous classes
|
|
|
|
Optional:
|
|
|
|
* final classes: use when appropriate, e.g. API restriction
|
|
* final methods: similar to final classes
|
|
|
|
Never:
|
|
|
|
* local variables: it clutters the code, and make the code less
|
|
readable. When copying old code to new location, finals should
|
|
be removed
|
|
* method parameters: similar to local variables
|
|
|
|
=== 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, members, and methods, in
|
|
decreasing order of visibility (public to private).
|
|
* Finally 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`
|
|
* The `@Inject`-ed constructor arguments should be listed one per line.
|
|
* Imports should be mostly alphabetical (uppercase sorts before
|
|
all lowercase, which means classes come before packages at the
|
|
same level).
|
|
* 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:
|
|
|
|
* 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 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.
|
|
|
|
[[process]]
|
|
== Process
|
|
|
|
=== Backporting to stable branches
|
|
|
|
From time to time bug fix releases are made for existing stable branches.
|
|
|
|
Developers concerned with stable branches are encouraged to backport or push
|
|
patchsets to these branches, even if no new release is planned.
|
|
|
|
Fixes that are known to be needed for a particular release should be pushed
|
|
for review on that release's stable branch. It will then be included in
|
|
the master branch when the stable branch is merged back.
|
|
|
|
=== Updating to new version of GWT
|
|
|
|
When updating to a new version of GWT, there are several things that also need
|
|
to be updated or at least checked.
|
|
|
|
* Update common and plugin dependencies in `tools/gwt-constants.defs`.
|
|
* Update to the same GWT version in the cookbook plugin and optionally in other
|
|
plugins that have a dependency on GWT.
|
|
* Update the GWT version in the archetype metadata in the
|
|
`gerrit-plugin-gwt-archetype`.
|
|
* Update to the same GWT version in the `gwtjsonrpc` project, and release a
|
|
new version.
|
|
|
|
=== Updating to new version of CodeMirror
|
|
|
|
* Clone the git from https://github.com/codemirror/CodeMirror
|
|
* Checkout the version needed
|
|
* If the needed version is not a tagged version, use `git describe` to determine
|
|
the version number:
|
|
+
|
|
----
|
|
git describe --tags
|
|
----
|
|
|
|
* Create the release zip file:
|
|
+
|
|
----
|
|
git archive --format=zip --prefix=codemirror-4.10.0-6-gd0a2dda/ d0a2dda > codemirror-4.10.0-6-gd0a2dda.zip
|
|
----
|
|
|
|
* Determine the sha1 hash of the zip file:
|
|
+
|
|
----
|
|
openssl sha1 4.10.0-6-gd0a2dda.zip
|
|
----
|
|
|
|
* Upload the zip file to the
|
|
link:https://console.developers.google.com/project/164060093628/storage/gerrit-maven/[
|
|
gerrit-maven] storage bucket
|
|
|
|
GERRIT
|
|
------
|
|
Part of link:index.html[Gerrit Code Review]
|
|
|
|
SEARCHBOX
|
|
---------
|