Received: from sog-mx-4.v43.ch3.sourceforge.com ([172.29.43.194] helo=mx.sourceforge.net) by sfs-ml-2.v29.ch3.sourceforge.com with esmtp (Exim 4.76) (envelope-from ) id 1VS4am-0002ol-Pp for bitcoin-development@lists.sourceforge.net; Fri, 04 Oct 2013 12:34:48 +0000 Received-SPF: pass (sog-mx-4.v43.ch3.sourceforge.com: domain of gmail.com designates 74.125.82.182 as permitted sender) client-ip=74.125.82.182; envelope-from=andyparkins@gmail.com; helo=mail-we0-f182.google.com; Received: from mail-we0-f182.google.com ([74.125.82.182]) by sog-mx-4.v43.ch3.sourceforge.com with esmtps (TLSv1:RC4-SHA:128) (Exim 4.76) id 1VS4am-0005rF-19 for bitcoin-development@lists.sourceforge.net; Fri, 04 Oct 2013 12:34:48 +0000 Received: by mail-we0-f182.google.com with SMTP id t61so3743796wes.13 for ; Fri, 04 Oct 2013 05:34:41 -0700 (PDT) X-Received: by 10.194.241.228 with SMTP id wl4mr12258186wjc.2.1380890081790; Fri, 04 Oct 2013 05:34:41 -0700 (PDT) Received: from momentum.localnet ([91.84.15.31]) by mx.google.com with ESMTPSA id dl10sm15405667wib.1.1969.12.31.16.00.00 (version=TLSv1.2 cipher=ECDHE-RSA-AES128-GCM-SHA256 bits=128/128); Fri, 04 Oct 2013 05:34:40 -0700 (PDT) From: Andy Parkins To: bitcoin-development@lists.sourceforge.net Date: Fri, 04 Oct 2013 13:34:39 +0100 Message-ID: <5088770.66l0OxckCD@momentum> User-Agent: KMail/4.10.5 (Linux/3.10-2-amd64; KDE/4.10.5; x86_64; ; ) In-Reply-To: <20131004113517.GA8373@savin> References: <3552695.aET6a1zFq8@momentum> <20131004113517.GA8373@savin> MIME-Version: 1.0 Content-Type: multipart/signed; boundary="nextPart1824160.1IYKqKxaEg"; micalg="pgp-sha1"; protocol="application/pgp-signature" X-Spam-Score: -1.6 (-) 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 FREEMAIL_FROM Sender email is commonly abused enduser mail provider (andyparkins[at]gmail.com) -0.0 SPF_PASS SPF: sender matches SPF record -0.1 DKIM_VALID_AU Message has a valid DKIM or DK signature from author's domain 0.1 DKIM_SIGNED Message has a DKIM or DK signature, not necessarily valid -0.1 DKIM_VALID Message has at least one valid DKIM or DK signature X-Headers-End: 1VS4am-0005rF-19 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 12:34:49 -0000 --nextPart1824160.1IYKqKxaEg Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" 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 --nextPart1824160.1IYKqKxaEg Content-Type: application/pgp-signature; name="signature.asc" Content-Description: This is a digitally signed message part. Content-Transfer-Encoding: 7Bit -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.20 (GNU/Linux) iEYEABECAAYFAlJOtd8ACgkQwQJ9gE9xL235YwCgvZ2WUYmt5T8DcQJI8pvZk4R3 r4sAoMkYSxTYyUQTX86+0gJNI5T79V8j =ExvI -----END PGP SIGNATURE----- --nextPart1824160.1IYKqKxaEg--