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
|
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 <andyparkins@gmail.com>) 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 <bitcoin-development@lists.sourceforge.net>;
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 <andyparkins@gmail.com>
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: <CANEZrP1Sd8cK2YUr4OSvnOxEJrbWpmfdpor-qbap1f98tGqPwg@mail.gmail.com>
<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: <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 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--
|
