Skip to content

Commit e050fdd

Browse files
committed
On Reviewing: add more info about ACKs
based on today's #bitcoin-core-dev IRC discussion and suggestions by Pieter Wuille (thanks!)
1 parent bf1cf3b commit e050fdd

File tree

2 files changed

+31
-6
lines changed

2 files changed

+31
-6
lines changed

articles.html

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@
5252

5353
<div id="entry_block">
5454
<div class="entry">
55-
<div class="date">Last updated: 10 May 2020</div>
55+
<div class="date">Last updated: 17 May 2020</div>
5656
<div class="body">
5757
<div class="title">Articles</div>
5858
<ul>

articles/how-to-review-pull-requests-in-bitcoin-core.html

Lines changed: 30 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -52,7 +52,7 @@
5252

5353
<div id="entry_block">
5454
<div class="entry">
55-
<div class="date">Last updated: 24 April 2020</div>
55+
<div class="date">Last updated: 17 May 2020</div>
5656
<div class="body">
5757
<article>
5858
<div class="title">How to Review Pull Requests in Bitcoin Core</div>
@@ -716,6 +716,13 @@ <h4 id="peer-review">Peer review</h4>
716716
<a href="https://github.com/bitcoin/bitcoin/pull/16149">updated</a>,
717717
so refer back to it frequently.
718718
</p>
719+
<p>
720+
An ACK is generally followed by a description of how the
721+
reviewer did the review and any manual testing. As a new
722+
contributor, it's advisable to be even more verbose in review
723+
comments to provide context on what you did and thought through
724+
during the review.
725+
</p>
719726
<p>
720727
Concept ACK means the reviewer acknowledges and agrees with the
721728
goal of the change, but is not (yet) confirming they've looked
@@ -730,6 +737,18 @@ <h4 id="peer-review">Peer review</h4>
730737
implementing the change. Approach NACK would therefore indicate
731738
agreement with the goal but not the approach.
732739
</p>
740+
<p>
741+
Sometimes reviewers comment "code review ack" to be specific
742+
about the fact that the code looks good but they haven't tested
743+
the changes further or don't have much of an opinion on the
744+
concept yet. When in doubt, adding more context doesn't hurt,
745+
e.g. "code review ACK <code>HEAD</code>, unsure about the
746+
concept, I'd like to verify x, y, z..."
747+
</p>
748+
<p>
749+
Other ACK variants that are sometimes used are "tACK" or "tested
750+
ACK", and "utACK" or "untested ACK".
751+
</p>
733752
<p>
734753
Manual testing of new features and reported issues is always
735754
welcome. A comment that is really helpful in review: "Here's
@@ -745,6 +764,12 @@ <h4 id="peer-review">Peer review</h4>
745764
about behaviour to give an ACK" is a perfectly helpful
746765
contribution.
747766
</p>
767+
<p>
768+
Examples of other useful comments could be "verified move-only"
769+
if the PR includes move-only commits, or "thought hard about how
770+
change X could break Y but didn't find any (or could this
771+
scenario happen?)", etc.
772+
</p>
748773
<p>
749774
When giving an ACK, specify the commits reviewed by appending
750775
the commit hash of the <code>HEAD</code> commit. The trustless
@@ -761,10 +786,10 @@ <h4 id="peer-review">Peer review</h4>
761786
</p>
762787
<p>
763788
The Bitcoin Core merge script currently copies into the merge
764-
commit all ACK comments pertaining to the <code>HEAD</code>
765-
commit at the time of merge. Keep in mind that anything you
766-
write in an ACK comment that is copied by the merge script will
767-
be in git history forever.
789+
commit the first line of each ACK review pertaining to
790+
the <code>HEAD</code> commit at the time of merge. Keep in mind
791+
that anything you write there that is copied by the merge script
792+
will be in git history forever.
768793
</p>
769794
<p>
770795
A complex PR usually requires at least 3-4 experienced ACKs

0 commit comments

Comments
 (0)