code reviews vs pull requests
TRANSCRIPT
@kannonboy
Code review at Atlassian
2006
2007Atlassian acquires Cenqua, starts using Crucible @kannonboy
@kannonboy
2006
2007Atlassian acquires Cenqua, starts using Crucible
2010 Atlassian acquires Bitbucket
@kannonboy
@kannonboy
2007Atlassian acquires Cenqua, starts using Crucible
2010 Atlassian acquires Bitbucket
2011 Work starts on “Caviar”!@kannonboy
@kannonboy
2010 Atlassian acquires Bitbucket
2011 Work starts on “Caviar”
2012 Stash 1.0 ships
!
Stash 2.0 ships with
@kannonboy
@kannonboy
2011 Work starts on “Caviar”
2012 Stash 1.0 ships
!
Stash 2.0 ships with pull requests
Comment drift
Unread status@kannonboy
@kannonboy
2011
2012 Stash 1.0 ships
!
Stash 2.0 ships with pull requests
Merge checks
Comment drift
Unread status
Conflict markers
Build status integration
Branch permissions
@kannonboy
@kannonboy
Stash renamed to Bitbucket Server2015
Merge checks
Comment drift
Conflict markers
Build status integration
Reviewer suggestions
Branch permissions
Comment likes
Enhanced branch permissions
Iterative review
Merge strategies
Reviewer statuses
Today@kannonboy
@kannonboy
Stash renamed to Bitbucket Server2015
Comment likes
Enhanced branch permissions
Iterative review
Merge strategies
Reviewer statuses
Today
@kannonboy
@kannonboy
T Y P E S O F R E V I E W
Y O U N E E D C O D E R E V I E W
C O D E R E V I E W @ AT L A S S I A N
T I P S F O R T E A M S
Agenda
human judgement needed
?
“what-evs”
bad API decision
O(n!) algorithm
technical debt
Automated tests
@kannonboy
Reviews and releases
10
20
30
40
Daily Weekly Monthly Quarterly Yearly
Code Review No Code Review
Source: Atlassian Git Survey 2013
@kannonboy
Pull requests
Code review
Post-commit review
Pre-commit review
master
for/master
Staging area @kannonboy
// flat fees and taxes final float customsFee = 5.5f; final float immigrationFee = 7f; final float federalTransportTax = .025f;
public float calculateAirfare(float baseFare) { float fare = baseFare; fare += immigrationFee + customsFee; fare *= return fare; }
AirfareCalculator.java
federalTransportTax;
@kannonboy
// flat fees and taxes final float customsFee = 5.5f; final float immigrationFee = 7f; final float federalTransportTax = .025f;
public float calculateAirfare(float baseFare) { float fare = baseFare; fare += immigrationFee + customsFee; fare *= return fare; }
(1 + )
AirfareCalculator.java
federalTransportTax ;
@kannonboy
Pre-commit reviewFrom: Tim Pettersen <[email protected]> Date: Mon, 12 Sep 2016 10:24:10 +1000 Subject: [PATCH] Calculate federal transport tax correctly
--- a/com/atlassian/airfare/AirfareCalculator.java +++ b/com/atlassian/airfare/AirfareCalculator.java @@ -10,7 +10,7 @@ public float calculateAirfare(float baseFare) { float fare = baseFare; fare += immigrationFee + customsFee; - fare *= federalTransportTax; + fare *= (1 + federalTransportTax); return fare; }
$ git format-patch HEAD~1 --stdout
@kannonboy
Pre-commit review
dev maintainer
public float calculateAirfare(float baseFare) { float fare = baseFare; fare += immigrationFee + customsFee;- fare *= federalTransportTax;+ fare *= (1 + federalTransportTax); return fare;}
Subject: [PATCH] Calculate federal transport tax correctly
@kannonboy
> > > > >
The braces are unnecessary.
Pre-commit review
dev maintainer
public float calculateAirfare(float baseFare) { float fare = baseFare; fare += immigrationFee + customsFee;- fare *= federalTransportTax;+ fare *= (1 + federalTransportTax);
return fare;}
Subject: Re: [PATCH] Calculate federal transport tax correctly
> >
@kannonboy
> >
> > > > > > > >
Technically true, but I think they add clarity.
> >
> > > > >
The braces are unnecessary.
Pre-commit review
dev maintainer
public float calculateAirfare(float baseFare) { float fare = baseFare; fare += immigrationFee + customsFee;- fare *= federalTransportTax;+ fare *= (1 + federalTransportTax);
return fare;}
Subject: Re: Re: [PATCH] Calculate federal transport tax correctly
@kannonboy
> > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
Pre-commit review
dev maintainer
Subject: Re: Re: Re: Re: Re: Re: Re: Re: Re: Re: Re: Re:
@kannonboy
Pre-commit reviewLinus Torvalds*
Module maintainers
File / driver maintainers
* Gross oversimplification, check out: bit.ly/linux-dev
@kannonboy
Pre-commit review
Junio C Hamano*
* Also an oversimplification
@kannonboy
Pull requests
Post-commit review
Pre-commit review
master
for/master
Staging area
Pre-commit reviewVersion control agnostic
Difficult to read
High barrier to submit patches
Simple and decentralized
Difficult to iterate
Easy to keep ‘000s of people in the loop
Have to manually merge work@kannonboy
Flexible structure
master
master
patches & untracked files
commits
branches
specific filesmultiple repos
@kannonboy
Not an SCM
SCM host (e.g. Bitbucket)
Review host (e.g. Crucible)
Dev team
push & pull
code review
slurp
slurp
slurp
slurp
@kannonboy
Pull requests
Post-commit review
Post-commit reviewSupports popular CVCS and DVCS
Have to manually merge work
Flexible review content
Need a separate VCS host
Easy to iterate
Hard to enforce process
@kannonboy
Gerrit is very “Git-y”
$ git config -f ~/gerrit/etc/gerrit.config gerrit.canonicalWebUrl
http://localhost:8080/
@kannonboy
Staging area review
for/master
master
reviewers approved?
verified (build passing)?
Dev team
fetchpush
@kannonboy
“squash commits first”
$ git push origin HEAD:refs/for/master
# typey type type
$ git commit -am “Review feedback”
Counting objects: 2108776, done.Delta compression using up to 8 threads.Compressing objects: 100% (59/59), done.Writing objects: 100% (86/86), 893.21 KiB, done.Total 86 (delta 0), reused 0 (delta 0)
! [remote rejected] HEAD -> refs/for/master (squash commits first)error: failed to push some refs to ‘ssh://...'
@kannonboy
Pull requests
Post-commit review
Pre-commit review
master
for/master
Staging area
Staging area reviewSophisticated policy enforcement
Steep learning curve to use & administer
Iteration is a little awkward
FOSS project built in Java, on JGit
Git only
Clean commit history
@kannonboy
Branches only
master
master
patches & untracked files
commits
branches
specific filesmultiple repos
@kannonboy
git merge --ff
@kannonboy
master
feature/JIRA-123
“Fast forward if you can, otherwise create a merge commit.”
git merge --ff
@kannonboym
aster
feature/JIRA-123
“Fast forward if you can, otherwise create a merge commit.”
git merge --squash
@kannonboy“Combine commits on branch into a new commit on master.”
master
feature/JIRA-123
git merge --squash \
@kannonboy“Combine commits on branch into a new commit on master. Fail if master has any commits that branch doesn’t.”
master
--ff-only
git merge --squash \
@kannonboy“Combine commits on branch into a new commit on master. Fail if master has any commits that branch doesn’t.”
master
--ff-only
git merge --squash \
@kannonboy“Combine commits on branch into a new commit on master. Fail if master has any commits that branch doesn’t.”
mast
er
--ff-only
Merge Commit Fast forward Squash
Concise history
Lose context of how features evolved
Which should I use?
“Ugly” history
Full traceability
Hard to screw up
mostly some
--no-ff --ff-only --squash
@kannonboy
No merge commits
Verbose history
Requires rebasing
Pull requests
bit.ly/git-lfs
Sophisticated policy enforcement
Less flexible review content
DVCS had some limitations around file size
Choice of merge strategies
Multiple team members can author code under review
Git & Mercurial
@kannonboy
1. One issue, one pull request
# what’s shipping? $ git branch --merged
# what’s left to ship? $ git branch --no-merged
@kannonboy
feature/JIRA-30
master
IN REVIEW DONEIN PROGRESSOPEN
Branch created!
@kannonboy
Automatic Transitions
feature/JIRA-30
master
IN REVIEW DONEIN PROGRESSOPEN
Pull Request Created!
@kannonboy
Automatic Transitions
feature/JIRA-30
master
IN REVIEW DONEIN PROGRESSOPEN
Pull Request Merged!
@kannonboy
Automatic Transitions
Fewer reviewers find more defects
0
7.5
15
22.5
30
1 2 3 4 5 6 7 8 9
Def
ects
/ k
LoC
Number of Reviewers @kannonboysource: bit.ly/review-stats
More reviewers spend less time
0
7
14
21
28
1 2 3 4 5 6
Min
utes
spe
nt /
revi
ewer
Number of Reviewers @kannonboysource: bit.ly/review-stats
4. Use blame to pick reviewers
$ npm install -g git-guilt
# find blame delta for current branch $ git guilt `git merge-base master HEAD` HEAD
Alice Foo ++++++++++++++++++++++++(239) Bob Bar ++++++++ Eve Baz ------- Mira Ted ---------------- Bec Opal ------------------------(-159)
@kannonboy
9. Add screenshots for UI/UX changes
(gifs / videos are even better) @kannonboy
Monosnap GIPHY ScreenFlow
@kannonboy
Ask a programmer to review 10 lines of code, they'll find 10 issues. Ask them to do 500 lines and they'll say it looks good.@ G I R AY O Z I L
“”
10. Keep it concise
10. Keep it concise Se
cond
s pe
r lin
e
Lines of Code
1 line per second
100 lines per second
@kannonboysource: bit.ly/review-stats
Code Review vs. Pull requests
Need to review multiple repos at once
Not on Git
Heavily iterative workflow Literally everyone else
Dendrophobia
+
@kannonboy
(Traditional)