From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from sog-mx-2.v43.ch3.sourceforge.com ([172.29.43.192] helo=mx.sourceforge.net) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1VS3fQ-0008Il-Su for bitcoin-development@lists.sourceforge.net; Fri, 04 Oct 2013 11:35:32 +0000 Received-SPF: pass (sog-mx-2.v43.ch3.sourceforge.com: domain of petertodd.org designates 62.13.148.113 as permitted sender) client-ip=62.13.148.113; envelope-from=pete@petertodd.org; helo=outmail148113.authsmtp.com; Received: from outmail148113.authsmtp.com ([62.13.148.113]) by sog-mx-2.v43.ch3.sourceforge.com with esmtp (Exim 4.76) id 1VS3fP-00082A-1o for bitcoin-development@lists.sourceforge.net; Fri, 04 Oct 2013 11:35:32 +0000 Received: from mail-c235.authsmtp.com (mail-c235.authsmtp.com [62.13.128.235]) by punt10.authsmtp.com (8.14.2/8.14.2) with ESMTP id r94BZM9u083064; Fri, 4 Oct 2013 12:35:22 +0100 (BST) Received: from savin (76-10-178-109.dsl.teksavvy.com [76.10.178.109]) (authenticated bits=128) by mail.authsmtp.com (8.14.2/8.14.2/) with ESMTP id r94BZHu3002705 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES128-SHA bits=128 verify=NO); Fri, 4 Oct 2013 12:35:20 +0100 (BST) Date: Fri, 4 Oct 2013 07:35:17 -0400 From: Peter Todd To: Andy Parkins Message-ID: <20131004113517.GA8373@savin> References: <3552695.aET6a1zFq8@momentum> MIME-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha256; protocol="application/pgp-signature"; boundary="TB36FDmn/VVEgNH/" Content-Disposition: inline In-Reply-To: <3552695.aET6a1zFq8@momentum> User-Agent: Mutt/1.5.21 (2010-09-15) X-Server-Quench: 0cf2a63a-2ce9-11e3-b802-002590a15da7 X-AuthReport-Spam: If SPAM / abuse - report it at: http://www.authsmtp.com/abuse X-AuthRoute: OCd2Yg0TA1ZNQRgX IjsJECJaVQIpKltL GxAVKBZePFsRUQkR aAdMdgYUF1YAAgsB AmUbWlZeU1h7W2M7 bAxPbAVDY01GQQRq WVdMSlVNFUsqCBhz bGZbURlydQJGfTBx YEVrXT5bDxJ4JEB+ E1NWRjgPeGZhPWMC AkhYdR5UcAFPdx8U a1UrBXRDAzANdhES HhM4ODE3eDlSNilR RRkIIFQOdA4gGTIx DwoPAzQiAiUA X-Authentic-SMTP: 61633532353630.1023:706 X-AuthFastPath: 0 (Was 255) X-AuthSMTP-Origin: 76.10.178.109/587 X-AuthVirus-Status: No virus detected - but ensure you scan with your own anti-virus system. X-Spam-Score: -1.5 (-) X-Spam-Report: Spam Filtering performed by mx.sourceforge.net. See http://spamassassin.org/tag/ for more details. -1.5 SPF_CHECK_PASS SPF reports sender host as permitted sender for sender-domain -0.0 SPF_PASS SPF: sender matches SPF record 0.0 URIBL_BLOCKED ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [URIs: petertodd.org] X-Headers-End: 1VS3fP-00082A-1o Cc: bitcoin-development@lists.sourceforge.net Subject: Re: [Bitcoin-development] Code review X-BeenThere: bitcoin-development@lists.sourceforge.net X-Mailman-Version: 2.1.9 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 04 Oct 2013 11:35:33 -0000 --TB36FDmn/VVEgNH/ Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable 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. > >=20 > > I'd like to make a small request - when submitting large, complex piece= s of > > work for review, please either submit it as one giant squashed change, = or >=20 > Don't do this. It throws away all of the good stuff that git lets you re= cord. =20 > There is more to a git branch than just the overall difference. Every si= ngle=20 > log message and diff is individually valuable. It's easy to make a squas= hed=20 > diff from many little commits; it's impossible to go the other way. >=20 > Command line for you so you don't have to think about it: >=20 > git diff $(git merge-base master feature-branch) feature-branch=20 >=20 > git-merge-base finds the common ancestor between master and feature-branc= h,=20 > 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. --=20 'peter'[:-1]@petertodd.org --TB36FDmn/VVEgNH/ Content-Type: application/pgp-signature; name="signature.asc" Content-Description: Digital signature -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.12 (GNU/Linux) iQEcBAEBCAAGBQJSTqf0AAoJECSBQD2l8JH7IHYH/2fZ3Y0h1xlDu1pU7QxPNAr6 ESiNbj6q8ee3sbo3+BjANsCgLm33F8tcYEVr97FOZA0yFAKw5tLT5jDCvz2S8r3F Ud2RD+WtCQox/uwW8vLB9SkdLdX5O5xNqQQrjSGRI1A4PhKptGFUllgZQRvywjT0 th3UoBWLihDmzZN5NkcUwtQGS2IYiy3Y/ri1WeBWJGA0p+t54AGBQT4KkqGJkl+O Xt0KjKYTIsoKQS6L5IWtpBrHTvIgzX1wZq8K2orlRIj3BmMYiumxX8V59Dsp62E/ OBHt197wpBx2mYbhcr8BQMRaKjCEzEYIYTWvmBX4ISxkgfudp3N3Gua5cPqshPw= =UQ2Y -----END PGP SIGNATURE----- --TB36FDmn/VVEgNH/--