[Bitcoin-development] Code review

[Bitcoin-development] Code review

[Mike Hearn]

[-- Attachment #1: Type: text/plain, Size: 1690 bytes --]

Git makes it easy to fork peoples work off and create long series of
commits that achieve some useful goal. That's great for many things.
Unfortunately, code review is not one of those things.

I'd like to make a small request - when submitting large, complex pieces of
work for review, please either submit it as one giant squashed change, or
be an absolute fascist about keeping commits logically clean and separated.
It really sucks to review things in sequence and then discover that some
code you spent some time thinking about or puzzling out got
deleted/rewritten/changed in a later commit. It also can make it harder to
review things when later code uses new APIs or behaviour changes introduced
in earlier commits - you have to either keep it all in your head, do lots
of tab switching, or do a squash yourself (in which case every reviewer
would have to manually do that).

On a related note, github seems to have lost the plot with regards to code
review - they are spending their time adding 3D renderers to their diff
viewer but not making basic improvements other tools had for years.

So, I'd like to suggest the idea of using Review Board:

http://www.reviewboard.org/

It's an open source, dedicated code review tool used by lots of big name
companies for their internal work. It has git[hub] integration and a lot of
very neat features, like the ability to attach screenshots to reviews. Also
more basic ones, like side by side diffs. Branches can be and often are
submitted to the system as single reviews.

The company behind it (disclosure - written and run by a long time friend
of mine) offers hosting plans, but we could also host it on a Foundation
server instead.

[-- Attachment #2: Type: text/html, Size: 1956 bytes --]

Re: [Bitcoin-development] Code review

[Andy Parkins]

On Friday 04 October 2013 12:30:07 Mike Hearn wrote:
> Git makes it easy to fork peoples work off and create long series of
> commits that achieve some useful goal. That's great for many things.
> Unfortunately, code review is not one of those things.
> 
> I'd like to make a small request - when submitting large, complex pieces of
> work for review, please either submit it as one giant squashed change, or

Don't do this.  It throws away all of the good stuff that git lets you record.  
There is more to a git branch than just the overall difference.  Every single 
log message and diff is individually valuable.  It's easy to make a squashed 
diff from many little commits; it's impossible to go the other way.

Command line for you so you don't have to think about it:

  git diff $(git merge-base master feature-branch) feature-branch 

git-merge-base finds the common ancestor between master and feature-branch, 
and then compares feature-branch against that.


Andy

-- 
Dr Andy Parkins
andyparkins@gmail.com

Re: [Bitcoin-development] Code review

[Mike Hearn]

[-- Attachment #1: Type: text/plain, Size: 1041 bytes --]

> There is more to a git branch than just the overall difference.  Every
> single
> log message and diff is individually valuable.


When the log messages don't accurately describe the contents of the diff,
it's just misinformation and noise. Everyone starts out by wanting a neat
collection of easy to understand and review commits, but in practice it's
extremely hard to always get it.

I know how to make squashed commits, thanks. I've done LOTS of code review
in my life. I'm making a point here as one of the few people who goes
through large pull requests and reviews them line by line. It's hard,
partly because github sucks, and partly because reviewing lots of small
commits sucks.

There's nothing that makes a single large commit harder to review. It's the
same amount of code or strictly less, given the tendency for later commits
to change earlier ones. You can easily search the entire change whilst
reviewing. There are lots of things that make it easier.

FWIW inside Google the code review process is one-commit-one-review.

[-- Attachment #2: Type: text/html, Size: 1453 bytes --]

Re: [Bitcoin-development] Code review

[Peter Todd]

[-- Attachment #1: Type: text/plain, Size: 3789 bytes --]

On Fri, Oct 04, 2013 at 11:42:29AM +0100, Andy Parkins wrote:
> On Friday 04 October 2013 12:30:07 Mike Hearn wrote:
> > Git makes it easy to fork peoples work off and create long series of
> > commits that achieve some useful goal. That's great for many things.
> > Unfortunately, code review is not one of those things.
> > 
> > I'd like to make a small request - when submitting large, complex pieces of
> > work for review, please either submit it as one giant squashed change, or
> 
> Don't do this.  It throws away all of the good stuff that git lets you record.  
> There is more to a git branch than just the overall difference.  Every single 
> log message and diff is individually valuable.  It's easy to make a squashed 
> diff from many little commits; it's impossible to go the other way.
> 
> Command line for you so you don't have to think about it:
> 
>   git diff $(git merge-base master feature-branch) feature-branch 
> 
> git-merge-base finds the common ancestor between master and feature-branch, 
> and then compares feature-branch against that.

Git is a revision *communication* system that happens to also make for a
good revision *control* system.

Remember that every individual commit is two things: what source code
has changed, and a message explaining why you thought that change should
be made. Commits aren't valuable in of themselves, they're valuable
because they serve to explain to the other people you are working with
why you thought a change should be made. Sometimes it makes sense to
explain your changes in 10 commits, sometimes it makes sense to squash
them all up into one commit, but there's no hard and fast rule other
than "Put yourself in your fellow coders' shoes - what's the best way to
explain to them what you are trying to accomplish and why?" You may have
generated a lot of little commits in the process of creating your patch
that tell a story that no-one else cares about, or equally by squashing
everything into one big commit you wind up with a tonne of changes with
little explanation as to why they were made.

Two caveats apply however: git-bisect works best if every commit in the
tree you are trying to debug works well enough that you can run tests
without errors - that is you don't "break the build". Don't make commits
that don't compile at the very least, and preferably everything you do
should be refactored to the point where the commit as a whole "works".

The second caveat is more specific to Bitcoin: people tend to rebase
their pull-requests over and over again until they are accepted, but
that also means that code review done earlier doesn't apply to the later
code pushed. Bitcoin is a particularly high profile, and high profit,
target for people trying to get malicious code into the codebase. It may
be the case that we would be better off asking reviewers making small
changes to their pull-requests to add additional commits for those
changes rather than rebasing, to make it clear what changes were
actually made so that code reviewers don't have to review the whole
patch from scratch. After all, the place where the most eyes will
actually look at the commits is during the pull-req process; after the
code has been pulled the audience for those commits is in most cases
almost no-one.

Having said that, there's currently a lot of other holes in the review
and source code integrity process, so fixing this problem is probably
not the low-hanging fruit right now.


FWIW personally I tend to review patches by both looking at the
individual commits to try to understand why someone wanted to make a
change, as well as all commits merged into one diff for a "what actually
changed here?" review.

-- 
'peter'[:-1]@petertodd.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [Bitcoin-development] Code review

[Peter Todd]

[-- Attachment #1: Type: text/plain, Size: 2850 bytes --]

On Fri, Oct 04, 2013 at 12:30:07PM +0200, Mike Hearn wrote:
> Git makes it easy to fork peoples work off and create long series of
> commits that achieve some useful goal. That's great for many things.
> Unfortunately, code review is not one of those things.
> 
> I'd like to make a small request - when submitting large, complex pieces of
> work for review, please either submit it as one giant squashed change, or
> be an absolute fascist about keeping commits logically clean and separated.
> It really sucks to review things in sequence and then discover that some
> code you spent some time thinking about or puzzling out got
> deleted/rewritten/changed in a later commit. It also can make it harder to
> review things when later code uses new APIs or behaviour changes introduced
> in earlier commits - you have to either keep it all in your head, do lots
> of tab switching, or do a squash yourself (in which case every reviewer
> would have to manually do that).

When I'm reviewing multiple commit pull-requests and want to see every
change made, I always either click on the "Files Changed" tab on github,
which collapses every commit into a single diff, or do the equivalent
with git log.

Why doesn't that work for you?

> On a related note, github seems to have lost the plot with regards to code
> review - they are spending their time adding 3D renderers to their diff
> viewer but not making basic improvements other tools had for years.
> 
> So, I'd like to suggest the idea of using Review Board:
> 
> http://www.reviewboard.org/
> 
> It's an open source, dedicated code review tool used by lots of big name
> companies for their internal work. It has git[hub] integration and a lot of
> very neat features, like the ability to attach screenshots to reviews. Also
> more basic ones, like side by side diffs. Branches can be and often are
> submitted to the system as single reviews.
> 
> The company behind it (disclosure - written and run by a long time friend
> of mine) offers hosting plans, but we could also host it on a Foundation
> server instead.

One advantage of using github is that they're an independent third
party; we should think carefully about the risks of furthering the
impression that Bitcoin development is a closed process by moving the
code review it to a server that we control with explicit review groups.

Given that Review Board appears to remain cryptographically unverifiable
there may also be disadvantages in operating it ourselves in that if the
review server does get compromised we *don't* have a third-party to
blame. In addition GitHub is a third-party with a very valuable
reputation to uphold and full-time staff - they're doing a better job of
keeping their servers secure and running then we ever could.

-- 
'peter'[:-1]@petertodd.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [Bitcoin-development] Code review

[Arto Bendiken]

On Fri, Oct 4, 2013 at 1:35 PM, Peter Todd <pete@petertodd.org> wrote:
> The second caveat is more specific to Bitcoin: people tend to rebase
> their pull-requests over and over again until they are accepted, but
> that also means that code review done earlier doesn't apply to the later
> code pushed. Bitcoin is a particularly high profile, and high profit,
> target for people trying to get malicious code into the codebase.

On that note, this 2003 example of an attempt to backdoor the Linux
kernel is pertinent:

http://lwn.net/Articles/57135/

The backdoor in question came down to a single missing character,
easily overlooked by a reviewer if a spotlight hadn't been thrown on
it for other reasons. Compromising a Bitcoin implementation isn't
going to be as easy as that, one would hope, but certainly it seems
only a matter of time until there's an attempt at it.

Following these code review discussions with much interest.

-- 
Arto Bendiken | @bendiken | http://ar.to/

Re: [Bitcoin-development] Code review

[Peter Todd]

[-- Attachment #1: Type: text/plain, Size: 1773 bytes --]

On Fri, Oct 04, 2013 at 01:58:51PM +0200, Arto Bendiken wrote:
> On Fri, Oct 4, 2013 at 1:35 PM, Peter Todd <pete@petertodd.org> wrote:
> > The second caveat is more specific to Bitcoin: people tend to rebase
> > their pull-requests over and over again until they are accepted, but
> > that also means that code review done earlier doesn't apply to the later
> > code pushed. Bitcoin is a particularly high profile, and high profit,
> > target for people trying to get malicious code into the codebase.
> 
> On that note, this 2003 example of an attempt to backdoor the Linux
> kernel is pertinent:
> 
> http://lwn.net/Articles/57135/
> 
> The backdoor in question came down to a single missing character,
> easily overlooked by a reviewer if a spotlight hadn't been thrown on
> it for other reasons. Compromising a Bitcoin implementation isn't
> going to be as easy as that, one would hope, but certainly it seems
> only a matter of time until there's an attempt at it.

Exactly.

Ideally code review discussions would be PGP signed and have a mechanism
for someone to sign a commit saying they had in fact reviewed it.
Combined with git's per-commit signature mechanism it'd make it possible
to write a git-pull hook that checked that whatever was being pulled had
some sufficient number of signatures from people whose reviews you
trusted. With such a system you could host code review anywhere safely,
or for that matter, use a completely distributed system.

But that's going to be a long way off. In the meantime github is
probably more trustworthy and competent than anything we ran ourselves,
and we should focus on making sure reviewers eyeballs actually look at
the code that ends up in master.

-- 
'peter'[:-1]@petertodd.org

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [Bitcoin-development] Code review

[Mike Hearn]

[-- Attachment #1: Type: text/plain, Size: 1060 bytes --]

On Fri, Oct 4, 2013 at 1:53 PM, Peter Todd <pete@petertodd.org> wrote:

> When I'm reviewing multiple commit pull-requests and want to see every
> change made, I always either click on the "Files Changed" tab on github,
> which collapses every commit into a single diff, or do the equivalent
> with git log.
>
> Why doesn't that work for you?
>

The files changed tab definitely works better for reading. In the past
comments I put there have disappeared, but I think that can also be true of
comments put on the individual commit reviews (which is another issue with
github, but it's unrelated to how the commits are presented). So I have
lost trust in doing reviews that way. It does make things easier to read
though.

One advantage of using github is that they're an independent third
> party; we should think carefully about the risks of furthering the
> impression that Bitcoin development is a closed process by moving the
> code review it to a server that we control with explicit review groups.
>

I guess anyone would be able to sign up and comment.

[-- Attachment #2: Type: text/html, Size: 1724 bytes --]

Re: [Bitcoin-development] Code review

[Eugen Leitl]

On Fri, Oct 04, 2013 at 02:14:19PM +0200, Mike Hearn wrote:

> One advantage of using github is that they're an independent third
> > party; we should think carefully about the risks of furthering the
> > impression that Bitcoin development is a closed process by moving the
> > code review it to a server that we control with explicit review groups.
> >
> 
> I guess anyone would be able to sign up and comment.

It's a long shot, but have any of you looked into Fossil?

Re: [Bitcoin-development] Code review

[Andy Parkins]

On Friday 04 October 2013 13:32:47 you wrote:
> > There is more to a git branch than just the overall difference.  Every
> > single
> > log message and diff is individually valuable.
> 
> When the log messages don't accurately describe the contents of the diff,
> it's just misinformation and noise. Everyone starts out by wanting a neat
> collection of easy to understand and review commits, but in practice it's
> extremely hard to always get it.

Then your request should be for better commits, not for just squashing the lot 
into some incoherent blob.

The alternatives under discussion are:

 - Coder produces long chain of commits on feature branch.  Compresses them, 
throwing away any individual and accurate messages into one large diff.  It's 
unlikely you'll get a log message that is as descriptive in the large one if 
you made them throw away the little ones.  Large diff is offered for review.  
Review is of one large diff.

 - Coder produces long chain on commits on feature branch.  Offers them for 
review.  Reviewer only likes to review large diffs, so uses the tools 
available to produce it.

Exactly the same diff is being reviewed, but in one case you're throwing away 
information.  There is no getting that information back ever.

You're also discarding the advantages of individual commits.

 - Merges are considerably harder than rebases.  You have to resolve all the 
conflicts at once with a merge, with a rebase you can resolve them with the 
log message and original isolated diff to help you.

 - Bisect doesn't give as fine-grained an answer.

> I know how to make squashed commits, thanks. I've done LOTS of code review

Excellent.  Don't take it personally -- I only offered it in case you didn't 
know.  Not everyone is familiar with git plumbing.

> in my life. I'm making a point here as one of the few people who goes
> through large pull requests and reviews them line by line. It's hard,

That doesn't make you the only person who does code reviews.  I do plenty of 
reviews here; they're just not bitcoin reviews.  Obviously we're talking about 
bitcoin, so you get to decide in the end.

> partly because github sucks, and partly because reviewing lots of small
> commits sucks.

I'm not suggesting you review lots of small commits anyway.  I can't comment 
on whether github sucks or not -- that's obviously personal preference.  
However, nothing stops you doing reviews on your own local checkout.

> There's nothing that makes a single large commit harder to review. It's the
> same amount of code or strictly less, given the tendency for later commits

That's not true.  There are often lots of small changes that are manifestly 
correct -- let's use string changes as an example -- in the large commit, they 
are just noise.  You want to be able to focus on the hard commits.  However -- 
I am not trying to persuade you to review small commits, I'm trying to 
persuade you not to throw away the small commits, gone forever, merely because 
your preference is to review large commits.

> to change earlier ones. You can easily search the entire change whilst
> reviewing. There are lots of things that make it easier.

Since the large commit is always available, no facilities have been lost.

Personally I work hard in my repositories to make coherent, small, well 
described commits.  If I had gone to that effort for a bitcoin branch only to 
be told to collapse them all and throw away that effort, I'd think I'd been 
wasting my time.



Andy

-- 
Dr Andy Parkins
andyparkins@gmail.com

Re: [Bitcoin-development] Code review

[Andy Parkins]

[-- Attachment #1: Type: text/plain, Size: 2667 bytes --]

On Friday 04 October 2013 07:35:17 you wrote:

> Remember that every individual commit is two things: what source code
> has changed, and a message explaining why you thought that change should
> be made. Commits aren't valuable in of themselves, they're valuable
> because they serve to explain to the other people you are working with
> why you thought a change should be made. Sometimes it makes sense to

Yes -- I'm assuming that.  I'm not advocating creating commits with random 
data as a log, and random bits of the changes.

> explain your changes in 10 commits, sometimes it makes sense to squash
> them all up into one commit, but there's no hard and fast rule other
> than "Put yourself in your fellow coders' shoes - what's the best way to
> explain to them what you are trying to accomplish and why?" You may have
> generated a lot of little commits in the process of creating your patch
> that tell a story that no-one else cares about, or equally by squashing

They don't care _now_; but when it comes to finding bugs, I can't count the 
number of times having a detailed change history has helped.  Combined with 
git-blame, it makes it very easy to ask "why did this line go in?".

> everything into one big commit you wind up with a tonne of changes with
> little explanation as to why they were made.

True enough.  I'm happy to accept that what you want is "the most optimum" set 
of commits.  But that doesn't mean "squash it all together".

> Two caveats apply however: git-bisect works best if every commit in the
> tree you are trying to debug works well enough that you can run tests
> without errors - that is you don't "break the build". Don't make commits
> that don't compile at the very least, and preferably everything you do
> should be refactored to the point where the commit as a whole "works".

Absolutely true.  I'm in favour of having the CI system test every commit for 
exactly that reason.  Even if you don't do that though, simply making the 
effort to make commits coherent means that its rare to get commits that don't 
build.
 
> FWIW personally I tend to review patches by both looking at the
> individual commits to try to understand why someone wanted to make a
> change, as well as all commits merged into one diff for a "what actually
> changed here?" review.

I think that code review is fundamentally hard.  There is only so much you can 
do to make it easier; and I'm not sure encouraging contributors to squash 
their chains is it.  Encouraging better commit behaviour would be better.

However, I'm only a lurker, not a committer, weight my opinions accordingly.



Andy
-- 
Dr Andy Parkins
andyparkins@gmail.com

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [Bitcoin-development] Code review

[Gavin Andresen]

[-- Attachment #1: Type: text/plain, Size: 1070 bytes --]

On Fri, Oct 4, 2013 at 8:30 PM, Mike Hearn <mike@plan99.net> wrote:

> I'd like to make a small request - when submitting large, complex pieces
> of work for review, please either submit it as one giant squashed change,
> or be an absolute fascist about keeping commits logically clean and
> separated.
>

I'll try harder to be a fascist (it doesn't come naturally to me). HUGE
thanks for taking the time to review the fee changes in detail.

RE: using Review Board:

I'm all for using better tools, if they will actually get used. If a
potential reviewer has to sign up to create a Review Board account or learn
Yet Another Tool, then I think it would be counter-productive:  we'd just
make the pool of reviewers even smaller than it already is.

Are there good examples of other open source software projects successfully
incentivizing review that we can copy?

For example, I'm wondering if maybe for the 0.9 release and onwards the
"Thank you" section should thank only people who have significantly helped
test or review other people's code.

-- 
--
Gavin Andresen

[-- Attachment #2: Type: text/html, Size: 1603 bytes --]

Re: [Bitcoin-development] Code review

[Warren Togami Jr.]

[-- Attachment #1: Type: text/plain, Size: 2033 bytes --]

https://www.kernel.org/doc/Documentation/SubmittingPatches
Read the section under "14) Using Reported-by:, Tested-by:, Reviewed-by:
and Suggested-by:". That might be helpful in our process too?

Warren


On Fri, Oct 4, 2013 at 4:31 PM, Gavin Andresen <gavinandresen@gmail.com>wrote:

> On Fri, Oct 4, 2013 at 8:30 PM, Mike Hearn <mike@plan99.net> wrote:
>
>> I'd like to make a small request - when submitting large, complex pieces
>> of work for review, please either submit it as one giant squashed change,
>> or be an absolute fascist about keeping commits logically clean and
>> separated.
>>
>
> I'll try harder to be a fascist (it doesn't come naturally to me). HUGE
> thanks for taking the time to review the fee changes in detail.
>
> RE: using Review Board:
>
> I'm all for using better tools, if they will actually get used. If a
> potential reviewer has to sign up to create a Review Board account or learn
> Yet Another Tool, then I think it would be counter-productive:  we'd just
> make the pool of reviewers even smaller than it already is.
>
> Are there good examples of other open source software projects
> successfully incentivizing review that we can copy?
>
> For example, I'm wondering if maybe for the 0.9 release and onwards the
> "Thank you" section should thank only people who have significantly helped
> test or review other people's code.
>
> --
> --
> Gavin Andresen
>
>
>
> ------------------------------------------------------------------------------
> October Webinars: Code for Performance
> Free Intel webinars can help you accelerate application performance.
> Explore tips for MPI, OpenMP, advanced profiling, and more. Get the most
> from
> the latest Intel processors and coprocessors. See abstracts and register >
> http://pubads.g.doubleclick.net/gampad/clk?id=60134791&iu=/4140/ostg.clktrk
> _______________________________________________
> Bitcoin-development mailing list
> Bitcoin-development@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/bitcoin-development
>
>

[-- Attachment #2: Type: text/html, Size: 3598 bytes --]

Re: [Bitcoin-development] Code review

[Mike Hearn]

[-- Attachment #1: Type: text/plain, Size: 1478 bytes --]

On Sat, Oct 5, 2013 at 4:31 AM, Gavin Andresen <gavinandresen@gmail.com>wrote:

> I'll try harder to be a fascist (it doesn't come naturally to me). HUGE
> thanks for taking the time to review the fee changes in detail.
>

Thanks, although I wasn't thinking specifically of you. The fee pull is
pretty well laid out. It just reminded me that it seems to be a common
issue I've had over the past year or so, across projects and people.


> I'm all for using better tools, if they will actually get used. If a
> potential reviewer has to sign up to create a Review Board account or learn
> Yet Another Tool, then I think it would be counter-productive:  we'd just
> make the pool of reviewers even smaller than it already is.
>

Yes, I don't know if github supports any kind of SSO. I will investigate.
As for learning another tool, well, when the current tool kind of sucks I
don't see any way around that one :)


> Are there good examples of other open source software projects
> successfully incentivizing review that we can copy?
>
> For example, I'm wondering if maybe for the 0.9 release and onwards the
> "Thank you" section should thank only people who have significantly helped
> test or review other people's code.
>

Perhaps just have a separate section for people who helped review above the
current section? It seems a bit mean not to credit occasional contributors
who fixed bugs or maintained something important but didn't review
complicated changes to the core.

[-- Attachment #2: Type: text/html, Size: 2478 bytes --]