public inbox for bitcoindev@googlegroups.com
 help / color / mirror / Atom feed
From: Andy Parkins <andyparkins@gmail.com>
To: bitcoin-development@lists.sourceforge.net
Subject: Re: [Bitcoin-development] Code review
Date: Fri, 04 Oct 2013 13:34:39 +0100	[thread overview]
Message-ID: <5088770.66l0OxckCD@momentum> (raw)
In-Reply-To: <20131004113517.GA8373@savin>

[-- 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 --]

  parent reply	other threads:[~2013-10-04 12:34 UTC|newest]

Thread overview: 14+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-10-04 10:30 [Bitcoin-development] Code review 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 [this message]
2013-10-04 11:53 ` Peter Todd
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=5088770.66l0OxckCD@momentum \
    --to=andyparkins@gmail.com \
    --cc=bitcoin-development@lists.sourceforge.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
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