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
|
Received: from sog-mx-3.v43.ch3.sourceforge.com ([172.29.43.193]
helo=mx.sourceforge.net)
by sfs-ml-1.v29.ch3.sourceforge.com with esmtp (Exim 4.76)
(envelope-from <andyparkins@gmail.com>) id 1VS4aV-0006BL-BC
for bitcoin-development@lists.sourceforge.net;
Fri, 04 Oct 2013 12:34:31 +0000
Received-SPF: pass (sog-mx-3.v43.ch3.sourceforge.com: domain of gmail.com
designates 74.125.82.45 as permitted sender)
client-ip=74.125.82.45; envelope-from=andyparkins@gmail.com;
helo=mail-wg0-f45.google.com;
Received: from mail-wg0-f45.google.com ([74.125.82.45])
by sog-mx-3.v43.ch3.sourceforge.com with esmtps (TLSv1:RC4-SHA:128)
(Exim 4.76) id 1VS4aS-0001B8-3c
for bitcoin-development@lists.sourceforge.net;
Fri, 04 Oct 2013 12:34:31 +0000
Received: by mail-wg0-f45.google.com with SMTP id y10so4073201wgg.12
for <bitcoin-development@lists.sourceforge.net>;
Fri, 04 Oct 2013 05:34:22 -0700 (PDT)
X-Received: by 10.180.81.71 with SMTP id y7mr7070759wix.63.1380890062000;
Fri, 04 Oct 2013 05:34:22 -0700 (PDT)
Received: from momentum.localnet ([91.84.15.31])
by mx.google.com with ESMTPSA id
dl10sm15407271wib.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:21 -0700 (PDT)
From: Andy Parkins <andyparkins@gmail.com>
To: bitcoin-development@lists.sourceforge.net
Date: Fri, 04 Oct 2013 13:34:19 +0100
Message-ID: <1625158.f2FY7VNCrz@momentum>
User-Agent: KMail/4.10.5 (Linux/3.10-2-amd64; KDE/4.10.5; x86_64; ; )
In-Reply-To: <CANEZrP1-4vP10w6_Yg3tXAKcMh406rw2bsAnvML2WoaU5SUjYw@mail.gmail.com>
References: <CANEZrP1Sd8cK2YUr4OSvnOxEJrbWpmfdpor-qbap1f98tGqPwg@mail.gmail.com>
<3552695.aET6a1zFq8@momentum>
<CANEZrP1-4vP10w6_Yg3tXAKcMh406rw2bsAnvML2WoaU5SUjYw@mail.gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
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: 1VS4aS-0001B8-3c
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:31 -0000
On Friday 04 October 2013 13:32:47 you wrote:
> > There is more to a git branch than just the overall difference. Every
> > single
> > log message and diff is individually valuable.
>
> When the log messages don't accurately describe the contents of the diff,
> it's just misinformation and noise. Everyone starts out by wanting a neat
> collection of easy to understand and review commits, but in practice it's
> extremely hard to always get it.
Then your request should be for better commits, not for just squashing the lot
into some incoherent blob.
The alternatives under discussion are:
- Coder produces long chain of commits on feature branch. Compresses them,
throwing away any individual and accurate messages into one large diff. It's
unlikely you'll get a log message that is as descriptive in the large one if
you made them throw away the little ones. Large diff is offered for review.
Review is of one large diff.
- Coder produces long chain on commits on feature branch. Offers them for
review. Reviewer only likes to review large diffs, so uses the tools
available to produce it.
Exactly the same diff is being reviewed, but in one case you're throwing away
information. There is no getting that information back ever.
You're also discarding the advantages of individual commits.
- Merges are considerably harder than rebases. You have to resolve all the
conflicts at once with a merge, with a rebase you can resolve them with the
log message and original isolated diff to help you.
- Bisect doesn't give as fine-grained an answer.
> I know how to make squashed commits, thanks. I've done LOTS of code review
Excellent. Don't take it personally -- I only offered it in case you didn't
know. Not everyone is familiar with git plumbing.
> in my life. I'm making a point here as one of the few people who goes
> through large pull requests and reviews them line by line. It's hard,
That doesn't make you the only person who does code reviews. I do plenty of
reviews here; they're just not bitcoin reviews. Obviously we're talking about
bitcoin, so you get to decide in the end.
> partly because github sucks, and partly because reviewing lots of small
> commits sucks.
I'm not suggesting you review lots of small commits anyway. I can't comment
on whether github sucks or not -- that's obviously personal preference.
However, nothing stops you doing reviews on your own local checkout.
> There's nothing that makes a single large commit harder to review. It's the
> same amount of code or strictly less, given the tendency for later commits
That's not true. There are often lots of small changes that are manifestly
correct -- let's use string changes as an example -- in the large commit, they
are just noise. You want to be able to focus on the hard commits. However --
I am not trying to persuade you to review small commits, I'm trying to
persuade you not to throw away the small commits, gone forever, merely because
your preference is to review large commits.
> to change earlier ones. You can easily search the entire change whilst
> reviewing. There are lots of things that make it easier.
Since the large commit is always available, no facilities have been lost.
Personally I work hard in my repositories to make coherent, small, well
described commits. If I had gone to that effort for a bitcoin branch only to
be told to collapse them all and throw away that effort, I'd think I'd been
wasting my time.
Andy
--
Dr Andy Parkins
andyparkins@gmail.com
|
