diff --git a/Documentation/dev-community.txt b/Documentation/dev-community.txt index 12fd5c1f26..36d57a83b3 100644 --- a/Documentation/dev-community.txt +++ b/Documentation/dev-community.txt @@ -23,6 +23,7 @@ Gerrit is developed as a self-hosting open source project: * link:dev-cla.html[Contributor License Agreement] * link:dev-contributing.html[Contributing to Gerrit] * link:dev-readme.html[Developer Setup] +* link:dev-crafting-changes.html[Crafting Changes] [[plugin-development]] == Plugin Development diff --git a/Documentation/dev-contributing.txt b/Documentation/dev-contributing.txt index a174504e5f..7cd135d10e 100644 --- a/Documentation/dev-contributing.txt +++ b/Documentation/dev-contributing.txt @@ -11,9 +11,9 @@ 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 +review of your change, review these link:dev-crafting-changes.html[ +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 @@ -37,271 +37,6 @@ 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, 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 -==== - - -[[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]. - -[[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.20.0). -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 - -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 - -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. 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 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. - * ...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. - == Finding starter projects to work on We have created a diff --git a/Documentation/dev-crafting-changes.txt b/Documentation/dev-crafting-changes.txt new file mode 100644 index 0000000000..ef96e6b80b --- /dev/null +++ b/Documentation/dev-crafting-changes.txt @@ -0,0 +1,271 @@ += 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]. + +[[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.20.0). +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. 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 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. + * ...and so is Guava (previously known as Google Collections). + +[[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 +---------