| = 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. |
| |
| [[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, or a |
| `Feature: Issue <#>` line if implementing a feature request. |
| * Include a `Change-Id` line |
| |
| [[vim-setup]] |
| === 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 |
| ==== |
| |
| |
| [[git-commit-settings]] |
| === 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]. |
| |
| Alternately, you may use the |
| link:https://pypi.org/project/git-review/[git-review] 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]] |
| == 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 generally follows the |
| link:https://google.github.io/styleguide/javaguide.html[Google Java Style |
| Guide]. |
| |
| To format Java source code, Gerrit uses the |
| link:https://github.com/google/google-java-format[`google-java-format`] |
| tool (version 1.7), and to format Bazel BUILD, WORKSPACE and .bzl files the |
| link:https://github.com/bazelbuild/buildtools/tree/master/buildifier[`buildifier`] |
| tool (version 0.29.0). Unused dependencies are found and removed using the |
| link:https://github.com/bazelbuild/buildtools/tree/master/unused_deps[`unused_deps`] |
| 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/setup_gjf.sh` 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. |
| |
| When considering the style beyond just formatting rules, it is often |
| more important to match the style of the nearby code which you are |
| modifying than it is to match the style guide exactly. This is |
| especially true within the same file. |
| |
| 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. |
| |
| When to use `final` modifier and when not (in new code): |
| |
| Always: |
| |
| * 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 |
| |
| Optional: |
| |
| * final classes: use when appropriate, e.g. API restriction |
| * final methods: similar to final classes |
| |
| Never: |
| |
| * 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 |
| |
| [[code-organization]] |
| == 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, 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]] |
| == 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 |
| busy. |
| |
| [[tests]] |
| == Tests |
| |
| * Tests for new code will greatly help your change get approved. |
| |
| [[change-size]] |
| == 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. |
| |
| GERRIT |
| ------ |
| Part of link:index.html[Gerrit Code Review] |
| |
| SEARCHBOX |
| --------- |