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
|
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 <cory@atlastechnologiesinc.com>) id 1Y0dXK-0006XC-Lm
for bitcoin-development@lists.sourceforge.net;
Mon, 15 Dec 2014 21:50:38 +0000
X-ACL-Warn:
Received: from mail-wg0-f48.google.com ([74.125.82.48])
by sog-mx-3.v43.ch3.sourceforge.com with esmtps (TLSv1:RC4-SHA:128)
(Exim 4.76) id 1Y0dXJ-00076n-9G
for bitcoin-development@lists.sourceforge.net;
Mon, 15 Dec 2014 21:50:38 +0000
Received: by mail-wg0-f48.google.com with SMTP id y19so15780647wgg.7
for <bitcoin-development@lists.sourceforge.net>;
Mon, 15 Dec 2014 13:50:31 -0800 (PST)
X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed;
d=1e100.net; s=20130820;
h=x-gm-message-state:mime-version:in-reply-to:references:date
:message-id:subject:from:to:cc:content-type;
bh=Z8ywXnif8P2OaZJeyV0V6UcF3jDkSXMy26PD+mrDqko=;
b=OI5J0XSWBb340VSaR0FoaTfGsWfwi+ucZzJEli/cWFgtqrjFQdqCnCI+lIeFbsAPyr
VpEVJWCIpswsDJQNGVGg5wpLGGXWudIeacv1w6s1XamlfLLndSst1kfQl6NsdkyCWdy5
iDpypFy7Yt63OGIiHZbrMSpTSeFRiSw0WtFyh5tUo7q0bMorRzoWyFpwer/5065/LC0E
32Nhr+BYEZ8GHEXOqiXbppJSjNBSJ6eEmVS1EGrBIRhVeG74lUOlqXv554wayEBTbUfz
o/UWpKS0iwwOhv8lUCw6JNXl6EwMxMKVqYgJZ0+wzeSUamxIg3CMRfgQZbQRIAVvT/Fy
qkhA==
X-Gm-Message-State: ALoCoQmEQRPmS6FvxENVDtXKebNjZynY6cMgnr2jasJIWEISJU2AYrXTAOobvUGtPmAPBnNZWRsU
MIME-Version: 1.0
X-Received: by 10.180.90.176 with SMTP id bx16mr35210055wib.0.1418678356438;
Mon, 15 Dec 2014 13:19:16 -0800 (PST)
Received: by 10.194.242.3 with HTTP; Mon, 15 Dec 2014 13:19:16 -0800 (PST)
In-Reply-To: <CAJHLa0MDftgnxB5itQS2jKqZsPj2vg-=SM+jVqUDxKGRZjuukA@mail.gmail.com>
References: <20141215124730.GA8321@savin.petertodd.org>
<CADJgMztRdNWyogPCjv64qpUHcvGDiOZDVAkv5Ra8hn25Z9xa8w@mail.gmail.com>
<CAJHLa0OvDPazCQVonhokm0KopN-WYj0_PP_LO8gPewc1LG5Qow@mail.gmail.com>
<CAApLimi+Jskwn=70+cfqr8=d55CKJFpBDBmW48zJk24PSBZczg@mail.gmail.com>
<CAJHLa0MDftgnxB5itQS2jKqZsPj2vg-=SM+jVqUDxKGRZjuukA@mail.gmail.com>
Date: Mon, 15 Dec 2014 16:19:16 -0500
Message-ID: <CAApLimjMkPvi+JfXMWsgS+tFCxXLivZc=eptuLn7z72O6CE6SA@mail.gmail.com>
From: Cory Fields <lists@coryfields.com>
To: Jeff Garzik <jgarzik@bitpay.com>
Content-Type: text/plain; charset=UTF-8
X-Spam-Score: 0.0 (/)
X-Spam-Report: Spam Filtering performed by mx.sourceforge.net.
See http://spamassassin.org/tag/ for more details.
X-Headers-End: 1Y0dXJ-00076n-9G
Cc: Bitcoin Dev <bitcoin-development@lists.sourceforge.net>
Subject: Re: [Bitcoin-development] Recent EvalScript() changes mean
CHECKLOCKTIMEVERIFY can't be merged
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: Mon, 15 Dec 2014 21:50:39 -0000
On Mon, Dec 15, 2014 at 2:35 PM, Jeff Garzik <jgarzik@bitpay.com> wrote:
> On Mon, Dec 15, 2014 at 1:42 PM, Cory Fields <lists@coryfields.com> wrote:
>>
>> That's exactly what happened during the modularization process, with
>> the exception that the code movement and refactors happened in
>> parallel rather than in series. But they _were_ done in separate
>> logical chunks for the sake of easier review.
>
>
> "That's exactly what was done except it wasn't"
>
> Yes, in micro, at the pull request level, this happened
> * Code movement
> * Refactor
>
> At a macro level, that cycle was repeated many times, leading to the
> opposite end result: a lot of tiny movement/refactor/movement/refactor
> producing the review and patch annoyances described.
>
> It produces a blizzard of new files and new data structures, breaking a
> bunch of out-of-tree patches, complicating review quite a bit. If the vast
> majority of code movement is up front, followed by algebraic
> simplifications, followed by data structure work, further patches are easy
> to review/apply with less impact on unrelated code.
>
I won't argue that at all because it's perfectly logical, but in
practice that doesn't translate from the macro level to the micro
level very well. At the micro level, minor code changes are almost
always needed to accommodate useful code movement. Even if they're not
required, it's often hard to justify code movement for the sake of
code movement with the promise that it will be useful later.
Rather than arguing hypotheticals, let's use a real example:
https://github.com/bitcoin/bitcoin/pull/5118 . That one's pretty
simple. The point of the PR was to unchain our openssl wrapper so that
key operations could be performed by the consensus lib without
dragging in bitcoind's structures. The first commit severs the
dependencies. The second commit does the code movement from the
now-freed wrapper.
I'm having a hard time coming up with a workflow that would handle
these two changes as _separate_ events, while making review easier.
Note that I'm not attempting to argue with you here, rather I'm
genuinely curious as to how you'd rather see this specific example
(which is representative of most of my other code movement for the
libbitcoinconsensus work, i believe) handled.
Using your model above, I suppose we'd do the code movement first with
the dependencies still intact as a pull request. At some later date,
we'd sever the dependencies in the new files. I suppose you'd also
prefer that I group a bunch of code-movement changes together into a
single PR which needs little scrutiny, only verification that it's
move-only. Once the code-movement PRs are merged, I can begin the
cleanups which actually fix something.
In practice, though, that'd be a massive headache for different
reasons. Lots in flux with seemingly no benefits until some later
date. My PR's can't depend on eachother because they don't actually
fix issues in a linear fashion. That means that other devs can't
depend on my PRs either for the same reason. And what have we gained?
Do you find that assessment unreasonable?
Cory
|
