1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43
44
45
46
47
48
49
50
51
52
53
54
55
56
57
58
59
60
61
62
63
64
65
66
67
68
69
70
71
72
73
74
75
76
77
78
79
80
81
82
83
84
85
86
87
88
89
90
91
92
93
94
95
96
97
98
99
100
101
102
103
104
105
106
107
108
109
110
111
112
113
114
115
116
117
118
119
120
121
122
123
124
125
126
127
128
129
130
131
132
133
134
135
136
137
138
139
140
141
142
143
144
145
146
147
148
149
150
151
152
153
154
155
156
157
158
159
160
161
162
163
164
165
166
167
168
169
170
171
172
173
174
175
|
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 <pete@petertodd.org>) 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 <pete@petertodd.org>
To: Andy Parkins <andyparkins@gmail.com>
Message-ID: <20131004113517.GA8373@savin>
References: <CANEZrP1Sd8cK2YUr4OSvnOxEJrbWpmfdpor-qbap1f98tGqPwg@mail.gmail.com>
<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: <bitcoin-development.lists.sourceforge.net>
List-Unsubscribe: <https://lists.sourceforge.net/lists/listinfo/bitcoin-development>,
<mailto:bitcoin-development-request@lists.sourceforge.net?subject=unsubscribe>
List-Archive: <http://sourceforge.net/mailarchive/forum.php?forum_name=bitcoin-development>
List-Post: <mailto:bitcoin-development@lists.sourceforge.net>
List-Help: <mailto:bitcoin-development-request@lists.sourceforge.net?subject=help>
List-Subscribe: <https://lists.sourceforge.net/lists/listinfo/bitcoin-development>,
<mailto:bitcoin-development-request@lists.sourceforge.net?subject=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/--
|
