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 --]
next prev 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 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$(echo .)com \
--cc=bitcoin-development@lists$(echo .)sourceforge.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