Code review tips - Revision history

Cwebber: /* How to add add review notes on a bug? */

2013-06-04T16:09:21Z

How to add add review notes on a bug?

← Older revision		Revision as of 16:09, 4 June 2013
Line 42:		Line 42:
	git checkout joar-fiddlybooks		git checkout joar-fiddlybooks

	== How to ~~add~~ add review notes on a bug? ==		== How to add review notes on a bug? ==

	Since we aren't using a review tool like reviewboard or the like, and it's easiest to keep our information in one place, ,it's best to actually add comments on the ticket itself. You can just talk about sections of the code like so on a ticket:		Since we aren't using a review tool like reviewboard or the like, and it's easiest to keep our information in one place, ,it's best to actually add comments on the ticket itself. You can just talk about sections of the code like so on a ticket:

Cwebber: /* Reviewing the whole branch as one diff */

2013-06-04T15:51:21Z

Reviewing the whole branch as one diff

← Older revision		Revision as of 15:51, 4 June 2013
Line 27:		Line 27:
	But what if you want to review everything that's changed at once, in one big diff?		But what if you want to review everything that's changed at once, in one big diff?

	Diffing with master is not really ideal... what happens if there are new commits in master? You'll see way more in the diff than is useful. ~~Add this to your {{Cmd\|~/.bashrc}}~~ ~~file~~:		Diffing with master as in {{Cmd\|git diff master joar/fiddlyhooks}} is not really ideal... what happens if there are new commits in master? You'll see way more in the diff than is useful. Instead, do:

		⚫	git diff master...joar/fiddlybooks
	function git-diff-commonbase() {
	git diff `git merge-base $2 $1` $2
	}

		⚫	Note the three periods. This will show a diff between master and joar/fiddlybooks that's only the things that have changed since master.
	Now open a new terminal, change to your mediagoblin directory, and try running this:

⚫	git-diff~~-commonbase~~ master joar/fiddlybooks

⚫	This will show a diff between master and joar/fiddlybooks that's only the things that have changed since master.

	== When in doubt, check it out ==		== When in doubt, check it out ==

Cwebber: /* Basic local code review */

2013-06-04T15:44:59Z

Basic local code review

@@ Line 47: / Line 47: @@
   git branch joar/fiddlybooks joar-fiddlybooks
   git checkout joar-fiddlybooks
 = Crazy emacs code review workflow =

Cwebber: /* Basic local code review */

2013-06-04T15:37:38Z

Basic local code review

← Older revision		Revision as of 15:37, 4 June 2013
Line 38:		Line 38:

	This will show a diff between master and joar/fiddlybooks that's only the things that have changed since master.		This will show a diff between master and joar/fiddlybooks that's only the things that have changed since master.

			== When in doubt, check it out ==

			Of course, if you really want to see how a branch works, don't be afraid to do a local checkout of it.

			It might be a good idea if you're checking out someone else's branch to make a "tracking branch" so that if you make changes, they aren't lost. Putting the remote in the name makes it easier to remember where it came from:

			git branch joar/fiddlybooks joar-fiddlybooks
			git checkout joar-fiddlybooks

	= Crazy emacs code review workflow =		= Crazy emacs code review workflow =

Cwebber: /* Crazy emacs code review workflow */

2013-06-04T15:35:40Z

Crazy emacs code review workflow

← Older revision		Revision as of 15:35, 4 June 2013
Line 40:		Line 40:

	= Crazy emacs code review workflow =		= Crazy emacs code review workflow =

			If you're an emacs user, [https://github.com/magit/magit Magit] is super great. Some tips:

			* If you're interested in seeing all of the branches that have new commits, you can use the built-in magit-wazzup.
			* Chris Webber wrote something based on magit-wazzup for people who are reviewing an insane amount of branches, called [http://gitorious.org/magit-review magit-review]. It's like magit-wazzup except you can move things in and out of review states and tell magit-review to ignore certain branches.
			* When viewing things in the git diff view, if you're on a file that you'd like to see what it looked like before and after the commits, hit "e" for magit-ediff. Note: ediff takes a bit to learn, and by default it uses a crappy external frame with a horizontal split (you can switch to vertical by hitting "\|"). Here are some better default settings for ediff:

			; Don't break out a separate frame for ediff
			(setq ediff-window-setup-function 'ediff-setup-windows-plain)

			; Horizontal splitting really ought to be the default, honestly.
			(setq ediff-split-window-function 'split-window-horizontally)

Cwebber: /* Reviewing commits local to this branch */

2013-06-04T15:23:31Z

Reviewing commits local to this branch

← Older revision		Revision as of 15:23, 4 June 2013
Line 11:		Line 11:
	== Reviewing commits local to this branch ==		== Reviewing commits local to this branch ==

	First, install ~~{{Cmd\|~~tig}}. tig is a command line git history browser.		First, install tig. tig is a command line git history browser.

	You could view the branch:		You could view the branch:

Cwebber: Added a basic workflow

2013-06-04T15:23:14Z

Added a basic workflow

New page

So you want to help with code review. Great! We have a [http://issues.mediagoblin.org/report/10 review queue] and if you have enough code review experience, maybe you'd like to help us there.

We don't use github though, and for the most part it's probably easiest to do "local" code review: pull down the remote of the person's branch you're reviewing, then use local tools to read through things.

For this set of examples we'll assume you pulled down Joar Wandborg's repository as "joar", and are reviewing the branch "fiddlybooks"

= Basic local code review =

This gives a set of suggestions on how to do code review for sane, command-line running people. (Crazy Chris Webber emacs using suggestions come later ;))

== Reviewing commits local to this branch ==

First, install {{Cmd|tig}}. tig is a command line git history browser.

You could view the branch:

tig joar/fiddlybooks

That will show you the whole history. But what if you want to *just* show the commits that are new to this branch?

tig master..joar/fiddlybooks

This will show you a narrowed log that only contains the branch that are new to this branch and not found in master. This means even if things are changing in master or it's been a while or there's been a bit of merging back and forth, this will make it much easier to read the history!

== Reviewing the whole branch as one diff ==

But what if you want to review *everything* that's changed at once, in one big diff?

Diffing with master is not really ideal... what happens if there are new commits in master? You'll see way more in the diff than is useful. Add this to your {{Cmd|~/.bashrc}} file:

function git-diff-commonbase() {
git diff `git merge-base $2 $1` $2
}

Now open a new terminal, change to your mediagoblin directory, and try running this:

git-diff-commonbase master joar/fiddlybooks

This will show a diff between master and joar/fiddlybooks that's *only* the things that have changed since master.

= Crazy emacs code review workflow =