Extract recommendations how to craft changes into a separate doc page
dev-contributing.txt should focus on describing the contribution
processes. At the moment we only have a single contribution process
(push a change and wait for review), but we plan to establish a new
contribution process with mentor guidance in addition. This new process
should be described on dev-contributing.txt too. All recommendations how
to craft changes are generally applicable, independent of which
contribution process is chosen, hence it makes sense to have them on a
separate page.
Signed-off-by: Edwin Kempin <ekempin@google.com>
Change-Id: Ia08da4ccd929c078761efd6ffe87a8883cc95118
diff --git a/Documentation/dev-crafting-changes.txt b/Documentation/dev-crafting-changes.txt
new file mode 100644
index 0000000..ef96e6b
--- /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
+---------
