[Bitcoin-development] Code review

[Andy Parkins]

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 at gmail.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: This is a digitally signed message part.
URL: <http://lists.linuxfoundation.org/pipermail/bitcoin-dev/attachments/20131004/cde9187b/attachment.sig>