utACK and t(ested) ACK are deprecated and will be removed in the next major release. Please use the more generic ACK and include an explanation of what was reviewed.
There was a related discussion in http://diyhpl.us/wiki/transcripts/bitcoin-core-dev-tech/2019-06-05-code-review/ section The author could offer a guide for review.
utACK
236 | @@ -237,24 +237,32 @@ request. Typically reviewers will review the code for obvious errors, as well as 237 | test out the patch set and opine on the technical merits of the patch. Project 238 | maintainers take into account the peer review when determining if there is 239 | consensus to merge a pull request (remember that discussions may have been 240 | -spread out over GitHub, mailing list and IRC discussions). The following 241 | +spread out over GitHub, mailing list and IRC discussions). 242 | + 243 | +#### Conceptual review
nit, upper case Review, same below.
Done
With this change it's impossible to quickly filter for t(ested) reviews. Let's see if this doesn't make people ACK less (when a simple utACK is enough like #16149 (comment)).
ACK fad8b05, new guideline looks good as well as markdown.
245 | +A review can be a conceptual review, where the reviewer leaves a comment 246 | + * `Concept ACK`, meaning "I agree in the general principle of this pull 247 | + request", 248 | + * or `NACK`, including a rationale why they think the change is or is not 249 | + worthwile. NACKs without accompanying reasoning may be disregarded. 250 | +
@sipa mentioned there is a distinction between ACKing
As a concept reviewer, it would be nice to communicate whether you're ACKing just #1, or also #2. This can help the author understand whether other contributors feel like this is an important problem to solve, so even those the proposed approach might not gain acceptance, it could be worthwhile for the author to go back to the drawing board to solve the problem (vs. give up entirely).
How should a concept reviewer express this? Should they state "Problem ACK, Concept NACK"?
Can't remember the wording, but I picked "Concept ACK" and "Design ACK".
This seems to say "Approach NACK" means "Concept ACK" which I don't think is true...
Fixed
I actually assumed "Approach NACK" implies "Concept ACK" somehow. If you don't like the concept, you probably don't care too much about the approach.
So I think "Approach ACK" should imply "Concept ACK". Not sure what "Approach NACK" should imply, but I think a NACK should be explained anyway, so that's not an issue in practice.
Ok, reverted back
utNACK . how about discouraged? i think its definitely useful to have more information, but its also nice to have shorthand for "i read through this code and it appears correct to me (as well as an implicit concept ACK)"
EDIT: Changing my opinion to somewhere between "Alex was here" and "utACK". I do think it can be useful to help other contributors think about what kind of concerns you analyzed or existing tests you thought might have covered the changes.
I don't think it is hard to explicitly write ACK, I only looked at the diff in GitHub for 5 seconds and it doesn't introduce an obvious DoS. I think utACK has a meaning that is too broad to have any meaning. It is highly discouraged. Of course you can still use it, but at your own expense.
246 | + * `Concept (N)ACK`, meaning "I do (not) agree in the general goal of this pull 247 | + request", 248 | + * `Design (N)ACK`, meaning `Concept ACK` and "I also do (not) agree with the design of 249 | + this change". 250 | + 251 | +A `NACK` needs to include a rationale why the change is or is not worthwile.
Is this really worthwhile? :-)
236 | @@ -237,24 +237,35 @@ request. Typically reviewers will review the code for obvious errors, as well as 237 | test out the patch set and opine on the technical merits of the patch. Project 238 | maintainers take into account the peer review when determining if there is 239 | consensus to merge a pull request (remember that discussions may have been 240 | -spread out over GitHub, mailing list and IRC discussions). The following 241 | +spread out over GitHub, mailing list and IRC discussions).
Could mention here that linking to those discussions when doing review/concept ACKing is encouraged.
ACK https://github.com/bitcoin/bitcoin/pull/16149/commits/fabddde9038c0cec16379d17de9d338cc9efe402
The only other suggestion I'd make, while we are modifying the contributing sections, is to mention that reviewers should take advantage of the review functionality on GitHub (especially when doing nit type review), if only to save repository subscribers from getting 10 emails vs 1.
Replaced "Design ACK" with "Approach ACK", since that seems to be more common.
246 | + * `Concept (N)ACK`, meaning "I do (not) agree in the general goal of this pull 247 | + request", 248 | + * `Approach (N)ACK`, meaning `Concept ACK` and "I also do (not) agree with the 249 | + approach of this change". 250 | + 251 | +A `NACK` needs to include a rationale why the change is or is not worthwhile.
I think you could drop the "is or" from that sentence
Done
Approach ACK, this seems to be an improvement to me.
ACK fac5ddfc5796022601af2c17fd0b158b2c465cba
Both Concept ACK and Approach ACK do not give any indication of whether the reviewer has tested the code. Testing is only referenced after the PR has received both a Concept ACK and an Approach ACK. Correct?
@michaelfolkson Both Concept and Approach ACK can occur before any code is written or at least mergeable code. Concept ACK is "I agree this is a problem that is a priority to address." Approach ACK is "I agree the design/approach is the right one to address this problem." Neither of these require final, testable, or mergeable code, although having code can help the author make the case that this is the best approach as it brings more clarity.
241 | +spread out over GitHub, mailing list and IRC discussions). 242 | + 243 | +#### Conceptual Review 244 | + 245 | +A review can be a conceptual review, where the reviewer leaves a comment 246 | + * `Concept (N)ACK`, meaning "I do (not) agree in the general goal of this pull
s/in/with/
251 | +A `NACK` needs to include a rationale why the change is not worthwhile. 252 | +NACKs without accompanying reasoning may be disregarded. 253 | + 254 | +#### Code Review 255 | + 256 | +After conceptual agreement on the change, code review can be provided. It is
Suggestion: s/It is starting/A review begins/
253 | + 254 | +#### Code Review 255 | + 256 | +After conceptual agreement on the change, code review can be provided. It is 257 | +starting with `ACK BRANCH_COMMIT`, where `BRANCH_COMMIT` is the top of the 258 | +topic branch. The review is followed by a description of how the reviewer did
Suggestion: s/branch. The review is /branch, /
257 | +starting with `ACK BRANCH_COMMIT`, where `BRANCH_COMMIT` is the top of the 258 | +topic branch. The review is followed by a description of how the reviewer did 259 | +the review. The following 260 | language is used within pull-request comments: 261 | 262 | - - (t)ACK means "I have tested the code and I agree it should be merged", involving
Removing the utACK designation might complicate, or render less reliable or useful, review parsing for apps like bitcoinacks and the review bot (https://twitter.com/BitcoinCorePRs) that I would like to finish after the Chaincode seminars.
Post-merge review. Cheers.
Milestone
0.19.0