From: Peter Todd <pete@petertodd•org>
To: Mike Hearn <mike@plan99•net>
Cc: Bitcoin Dev <bitcoin-development@lists•sourceforge.net>
Subject: Re: [Bitcoin-development] Code review
Date: Fri, 4 Oct 2013 07:53:00 -0400 [thread overview]
Message-ID: <20131004115300.GA22215@savin> (raw)
In-Reply-To: <CANEZrP1Sd8cK2YUr4OSvnOxEJrbWpmfdpor-qbap1f98tGqPwg@mail.gmail.com>
[-- 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 --]
next prev parent reply other threads:[~2013-10-04 11:53 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-10-04 10:30 Mike Hearn
2013-10-04 10:42 ` Andy Parkins
2013-10-04 11:32 ` Mike Hearn
2013-10-04 12:34 ` Andy Parkins
2013-10-04 11:35 ` Peter Todd
2013-10-04 11:58 ` Arto Bendiken
2013-10-04 12:14 ` Peter Todd
2013-10-04 12:34 ` Andy Parkins
2013-10-04 11:53 ` Peter Todd [this message]
2013-10-04 12:14 ` Mike Hearn
2013-10-04 12:22 ` Eugen Leitl
2013-10-05 2:31 ` Gavin Andresen
2013-10-05 4:02 ` Warren Togami Jr.
2013-10-05 11:36 ` Mike Hearn
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20131004115300.GA22215@savin \
--to=pete@petertodd$(echo .)org \
--cc=bitcoin-development@lists$(echo .)sourceforge.net \
--cc=mike@plan99$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox