presentations/gerrit-and-code-review-best-practices/gerrit-and-code-review-best-practices-slides.txt - training/gerrit - Git at Google

 [[title-page]]
 == Gerrit and Code Review Best Practices

 == Gerrit and Code Review Best Practices

 <<<
 [[agenda-item]]
 === 1. Best Practices - Making Changes

 <<<
 [[agenda-item]]
 === 2. Best Practices - Reviewing Changes

 <<<
 [[agenda-item]]
 === 3. Best Practices - Project Setup

 [[title-page]]
 == Best Practices - Making Changes

 == Making Changes
 each change should add something *usable*

 [role="incremental"]
 --
 ----
   change = complete feature
 ----

 *or*

 ----
   change = bug-fix
 ----
 --

 [role="incremental"]
 => Have a releasable state after every change.

 == Making Changes
 each change should focus on only *one thing*

 [role="incremental"]
 * do not mix features, bug-fixes

 [role="incremental"]
 ****
 `Q:` Why is this bad?

 [role="incremental"]
 `A:`
 [role="incremental"]
 * You need the bug-fix in another branch. +
   -> `git cherry-pick` is not easy
 * The feature broke something. +
   -> `git revert` cannot be used
 * It's more difficult to review.
 ****

 == Making Changes
 push only changes that are *ready*

 [role="incremental"]
 ****
 `Q:` Who wants to merge unfinished changes?
 ****

 [role="incremental"]
 ****
 `Q:` Who wants to review unfinished changes?
 [role="incremental"]
 --
 `A:` It is unclear for reviewers

 * what is an issue and
 * what is simply not done yet
 --
 ****

 == Making Changes
 mark *unfinished and experimental changes* as `[RFC]` (Request For
 Comments), +
 but tell reviewers which feedback you expect

 image:../../img/rfc.png[RFC]

 Examples:

 * https://gerrit-review.googlesource.com/#/c/24202/
 * https://gerrit-review.googlesource.com/#/c/24064/

 == Making Changes
 write *good commit message*

 [role="incremental"]
 * make the change attractive to reviewers

 [role="incremental"]
 ****
 `Q:` Who wants to review this change?

 image:../../img/bad-commit-message.png[Bad Commit Message]
 ****

 == Making Changes
 write *good commit message*

 * make the change attractive to reviewers

 ****
 `Q:` This one looks more interesting, doesn't it?
 image:../../img/good-commit-message.png[Good Commit Message]
 ****

 == Making Changes
 write *good commit message*

 * have precise subjects
 +
 image:../../img/git-log-oneline.png[git log --onoline]

 == Making Changes
 explain the *motivation* for the change

 [role="incremental"]
 * what was changed can be seen from the diff

 [role="incremental"]
 ****
 `Q:` Why is this commit message bad?

 image:../../img/no-motivation.png[Commit Message without motivation]
 [role="incremental"]
 `A:` No info about *why* this had to be disabled. +
 &#160;&#160;&#160;&#160;&#160;&#160;`git blame` gets less useful.
 ****

 == Making Changes
 prefer *small* changes

 [role="incremental"]
 ****
 `Q:` Who wants to review this change?

 image:../../img/huge-change.png[Huge Change]
 ****

 == Making Changes
 make big features as *series of small changes*

 [role="incremental"]
 * each change adds something *usable*
 * mark the change series as a *topic*

 [role="incremental"]
 --
 Example:

 image:../../img/topic.png[Topic]

 https://gerrit-review.googlesource.com/#/q/status:merged+project:gerrit+branch:master+topic:ssh-list-groups,n,z
 --

 == Making Changes
 split refactoring from feature

 [role="incremental"]
 * makes change *smaller*
 * the refactoring change is *"easier"* to review
 ** even without knowing anything about the change, +
 one can check if the old and new code do the same

 == Making Changes
 provide *screenshots* for UI changes

 image:../../img/screenshot-attached.png[Screenshot Attached]

 Example:

 https://gerrit-review.googlesource.com/#/c/36460/

 == Making Changes
 reply to *every* comment

 [role="incremental"]
 * use `Reply Done` if issue was addressed
 +
 image:../../img/done.png[Done Comment]

 * write reply why comment was ignored
 +
 image:../../img/reply.png[Comment Reply]

 == Making Changes - Summary

 ****
 Better *one developer* invests time to polish a change +
 than *n reviewers* waste time on review.
 ****

 [[title-page]]
 == Best Practices - Reviewing Changes

 == Reviewing Changes
 review *each* change

 [role="incremental"]
 --
 `Q:` Looks good!?

 image:../../img/small-change-1.png[Small Change]
 --

 == Reviewing Changes
 review *each* change

 --
 `Q:` And now?

 image:../../img/small-change-2.png[Small Change]
 --

 == Reviewing Changes
 *everyone* should do reviews

 [role="incremental"]
 * helps to distribute knowledge
 * share the review effort

 [role="incremental"]
 *follow reviews* of others

 [role="incremental"]
 * learn from it

 == Reviewing Changes
 *cooperate* to find best solution

 [role="incremental"]
 * no fault finding, no finger pointing
 * propose improvements
 +
 image:../../img/improvement.png[Improvement]

 == Reviewing Changes
 mark *optional* comments as `[optional]`

 image:../../img/optional-comment.png[Optional Comment]

 [role="incremental"]
 --
 comment on *nits*

 image:../../img/nit-comment.png[Nit Comment]
 --

 == Reviewing Changes
 *try out* changes

 [role="incremental"]
 * fetch the change
 * try to break the feature
 * if something doesn't work, +
   find the error in the code and +
   *comment inline*
 * vote in *Verified*

 == Reviewing Changes
 issue in *merged code*

 [role="incremental"]
 * upload a change that fixes the issue
 * find the change that introduced the issue +
   and comment on it
 * report the issue in the issue tracker system

 == Reviewing Changes
 *Code Review* Voting

 [role="incremental"]
 * +2: fully understood, all correct, looks good
 * +1: looks good, don't see anything wrong
 * 0: optional comment, just a question
 * -1: feature is good, but there are issues that should be fixed
 * -2: veto, must not be merged

 == Reviewing Changes
 do *not* self approve changes

 [role="incremental"]
 * 4-eyes principle
 * may be enforced via
   link:https://gerrit-review.googlesource.com/Documentation/prolog-cookbook.html#_example_8_make_change_submittable_only_if_tt_code_review_2_tt_is_given_by_a_non_author[
   Prolog submit rule]
 +
 image:../../img/need-non-author-code-review.png[Need Non Author Code Review]

 == Reviewing Changes
 review takes *time*

 [role="incremental"]
 * days, weeks or even months
 * merge changes only when they are ready
 * some changes may never be merged

 == Reviewing Changes - Summary

 ****
 Being a good reviewer is a different *qualification* than being a good
 developer.
 ****

 [role="incremental"]
 ****
 Code Review works!

 https://gerrit-review.googlesource.com/#/c/30171/
 ****

 [[title-page]]
 == Best Practices - Project Setup

 == Project Setup
 *automate* verification

 [role="incremental"]
 * link:https://wiki.jenkins-ci.org/display/JENKINS/Gerrit+Trigger[
   Gerrit Trigger] Plugin for Jenkins
 +
 image:../../img/jenkins-voter-1.png[Jenkins Voter]
 +
 image:../../img/jenkins-voter-2.png[Jenkins Voter]

 == Project Setup
 * define *code styles*
 * agree on line endings
 * have a *contributor guide*
 +
 https://gerrit-review.googlesource.com/Documentation/dev-contributing.html

 == Project Setup
 not everyone needs *submit* privileges

 [role="incremental"]
 * committer vs. contributor
 * committer election process

 == Project Setup
 use *text format* for documentation

 [role="incremental"]
 * e.g. link:http://www.methods.co.nz/asciidoc/userguide.html[ASCIIDOC]
 * makes documentation changes reviewable
 * version documentation together with the code
 +
 image:../../img/docu-change.png[Documentation Change]

 * vote `-1` if documentation is missing
 +
 image:../../img/missing-documentation.png[Missing Documentation]

 == Project Setup
 customize *submit rules* via
 link:https://gerrit-review.googlesource.com/Documentation/prolog-cookbook.html[
 Prolog]

 == Project Setup
 choose right *submit type*

 [role="incremental"]
 * merge/rebase effort vs. risk to break main build
 * enable `Automatically resolve conflicts`
 +
 image:../../img/automatically-resolve-conflicts.png[Automatically Resolve
 Conflicts]

 == Project Setup
 do *not* version large binaries

 [role="incremental"]
 * results in bad performance
 * use Maven repo, Nexus to store build artefacts
 * link:https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#receive.maxObjectSizeLimit[
   configure max object size] to make push fail

 [role="incremental"]
 find *right granularity* for the repositories

 [role="incremental"]
 * avoid too big repositories
 * version together what needs to be released together

 == Project Setup
 *learn* Git/Gerrit tooling and workflow

 [role="incremental"]
 * offer Git/Gerrit trainings
 * offer support via mailing list
	[[title-page]]
	== Gerrit and Code Review Best Practices

	== Gerrit and Code Review Best Practices

	<<<
	[[agenda-item]]
	=== 1. Best Practices - Making Changes

	<<<
	[[agenda-item]]
	=== 2. Best Practices - Reviewing Changes

	<<<
	[[agenda-item]]
	=== 3. Best Practices - Project Setup

	[[title-page]]
	== Best Practices - Making Changes

	== Making Changes
	each change should add something usable

	[role="incremental"]
	--
	----
	change = complete feature
	----

	or

	----
	change = bug-fix
	----
	--

	[role="incremental"]
	=> Have a releasable state after every change.

	== Making Changes
	each change should focus on only one thing

	[role="incremental"]
	* do not mix features, bug-fixes

	[role="incremental"]
	****
	`Q:` Why is this bad?

	[role="incremental"]
	`A:`
	[role="incremental"]
	* You need the bug-fix in another branch. +
	-> `git cherry-pick` is not easy
	* The feature broke something. +
	-> `git revert` cannot be used
	* It's more difficult to review.
	****

	== Making Changes
	push only changes that are ready

	[role="incremental"]
	****
	`Q:` Who wants to merge unfinished changes?
	****

	[role="incremental"]
	****
	`Q:` Who wants to review unfinished changes?
	[role="incremental"]
	--
	`A:` It is unclear for reviewers

	* what is an issue and
	* what is simply not done yet
	--
	****

	== Making Changes
	mark unfinished and experimental changes as `[RFC]` (Request For
	Comments), +
	but tell reviewers which feedback you expect

	image:../../img/rfc.png[RFC]

	Examples:

	* https://gerrit-review.googlesource.com/#/c/24202/
	* https://gerrit-review.googlesource.com/#/c/24064/

	== Making Changes
	write good commit message

	[role="incremental"]
	* make the change attractive to reviewers

	[role="incremental"]
	****
	`Q:` Who wants to review this change?

	image:../../img/bad-commit-message.png[Bad Commit Message]
	****

	== Making Changes
	write good commit message

	* make the change attractive to reviewers

	****
	`Q:` This one looks more interesting, doesn't it?
	image:../../img/good-commit-message.png[Good Commit Message]
	****

	== Making Changes
	write good commit message

	* have precise subjects
	+
	image:../../img/git-log-oneline.png[git log --onoline]

	== Making Changes
	explain the motivation for the change

	[role="incremental"]
	* what was changed can be seen from the diff

	[role="incremental"]
	****
	`Q:` Why is this commit message bad?

	image:../../img/no-motivation.png[Commit Message without motivation]
	[role="incremental"]
	`A:` No info about why this had to be disabled. +
	`git blame` gets less useful.
	****

	== Making Changes
	prefer small changes

	[role="incremental"]
	****
	`Q:` Who wants to review this change?

	image:../../img/huge-change.png[Huge Change]
	****

	== Making Changes
	make big features as series of small changes

	[role="incremental"]
	* each change adds something usable
	* mark the change series as a topic

	[role="incremental"]
	--
	Example:

	image:../../img/topic.png[Topic]

	https://gerrit-review.googlesource.com/#/q/status:merged+project:gerrit+branch:master+topic:ssh-list-groups,n,z
	--

	== Making Changes
	split refactoring from feature

	[role="incremental"]
	* makes change smaller
	* the refactoring change is "easier" to review
	** even without knowing anything about the change, +
	one can check if the old and new code do the same

	== Making Changes
	provide screenshots for UI changes

	image:../../img/screenshot-attached.png[Screenshot Attached]

	Example:

	https://gerrit-review.googlesource.com/#/c/36460/

	== Making Changes
	reply to every comment

	[role="incremental"]
	* use `Reply Done` if issue was addressed
	+
	image:../../img/done.png[Done Comment]

	* write reply why comment was ignored
	+
	image:../../img/reply.png[Comment Reply]

	== Making Changes - Summary

	****
	Better one developer invests time to polish a change +
	than n reviewers waste time on review.
	****

	[[title-page]]
	== Best Practices - Reviewing Changes

	== Reviewing Changes
	review each change

	[role="incremental"]
	--
	`Q:` Looks good!?

	image:../../img/small-change-1.png[Small Change]
	--

	== Reviewing Changes
	review each change

	--
	`Q:` And now?

	image:../../img/small-change-2.png[Small Change]
	--

	== Reviewing Changes
	everyone should do reviews

	[role="incremental"]
	* helps to distribute knowledge
	* share the review effort

	[role="incremental"]
	follow reviews of others

	[role="incremental"]
	* learn from it

	== Reviewing Changes
	cooperate to find best solution

	[role="incremental"]
	* no fault finding, no finger pointing
	* propose improvements
	+
	image:../../img/improvement.png[Improvement]

	== Reviewing Changes
	mark optional comments as `[optional]`

	image:../../img/optional-comment.png[Optional Comment]

	[role="incremental"]
	--
	comment on nits

	image:../../img/nit-comment.png[Nit Comment]
	--

	== Reviewing Changes
	try out changes

	[role="incremental"]
	* fetch the change
	* try to break the feature
	* if something doesn't work, +
	find the error in the code and +
	comment inline
	* vote in Verified

	== Reviewing Changes
	issue in merged code

	[role="incremental"]
	* upload a change that fixes the issue
	* find the change that introduced the issue +
	and comment on it
	* report the issue in the issue tracker system

	== Reviewing Changes
	Code Review Voting

	[role="incremental"]
	* +2: fully understood, all correct, looks good
	* +1: looks good, don't see anything wrong
	* 0: optional comment, just a question
	* -1: feature is good, but there are issues that should be fixed
	* -2: veto, must not be merged

	== Reviewing Changes
	do not self approve changes

	[role="incremental"]
	* 4-eyes principle
	* may be enforced via
	link:https://gerrit-review.googlesource.com/Documentation/prolog-cookbook.html#_example_8_make_change_submittable_only_if_tt_code_review_2_tt_is_given_by_a_non_author[
	Prolog submit rule]
	+
	image:../../img/need-non-author-code-review.png[Need Non Author Code Review]

	== Reviewing Changes
	review takes time

	[role="incremental"]
	* days, weeks or even months
	* merge changes only when they are ready
	* some changes may never be merged

	== Reviewing Changes - Summary

	****
	Being a good reviewer is a different qualification than being a good
	developer.
	****

	[role="incremental"]
	****
	Code Review works!

	https://gerrit-review.googlesource.com/#/c/30171/
	****

	[[title-page]]
	== Best Practices - Project Setup

	== Project Setup
	automate verification

	[role="incremental"]
	* link:https://wiki.jenkins-ci.org/display/JENKINS/Gerrit+Trigger[
	Gerrit Trigger] Plugin for Jenkins
	+
	image:../../img/jenkins-voter-1.png[Jenkins Voter]
	+
	image:../../img/jenkins-voter-2.png[Jenkins Voter]

	== Project Setup
	* define code styles
	* agree on line endings
	* have a contributor guide
	+
	https://gerrit-review.googlesource.com/Documentation/dev-contributing.html

	== Project Setup
	not everyone needs submit privileges

	[role="incremental"]
	* committer vs. contributor
	* committer election process

	== Project Setup
	use text format for documentation

	[role="incremental"]
	* e.g. link:http://www.methods.co.nz/asciidoc/userguide.html[ASCIIDOC]
	* makes documentation changes reviewable
	* version documentation together with the code
	+
	image:../../img/docu-change.png[Documentation Change]

	* vote `-1` if documentation is missing
	+
	image:../../img/missing-documentation.png[Missing Documentation]

	== Project Setup
	customize submit rules via
	link:https://gerrit-review.googlesource.com/Documentation/prolog-cookbook.html[
	Prolog]

	== Project Setup
	choose right submit type

	[role="incremental"]
	* merge/rebase effort vs. risk to break main build
	* enable `Automatically resolve conflicts`
	+
	image:../../img/automatically-resolve-conflicts.png[Automatically Resolve
	Conflicts]

	== Project Setup
	do not version large binaries

	[role="incremental"]
	* results in bad performance
	* use Maven repo, Nexus to store build artefacts
	* link:https://gerrit-review.googlesource.com/Documentation/config-gerrit.html#receive.maxObjectSizeLimit[
	configure max object size] to make push fail

	[role="incremental"]
	find right granularity for the repositories

	[role="incremental"]
	* avoid too big repositories
	* version together what needs to be released together

	== Project Setup
	learn Git/Gerrit tooling and workflow

	[role="incremental"]
	* offer Git/Gerrit trainings
	* offer support via mailing list