doc: Rework section on ACK in CONTRIBUTING.md #16149

maflcko · 2019-06-05T13:47:13Z

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.

laanwj · 2019-06-05T13:54:59Z

utACK

promag

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.

CONTRIBUTING.md

moneyball · 2019-06-05T14:20:54Z

CONTRIBUTING.md

+   request",
+ * or `NACK`, including a rationale why they think the change is or is not
+   worthwile. NACKs without accompanying reasoning may be disregarded.
+


@sipa mentioned there is a distinction between ACKing

the problem intended to be solved

the approach being used to solve the problem

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...

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

morcos · 2019-06-05T14:53:43Z

~~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.

maflcko · 2019-06-05T14:59:03Z

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.

CONTRIBUTING.md

fanquake

ACK fabddde

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.

fanquake · 2019-06-07T11:01:12Z

CONTRIBUTING.md

 maintainers take into account the peer review when determining if there is
 consensus to merge a pull request (remember that discussions may have been
-spread out over GitHub, mailing list and IRC discussions). The following
+spread out over GitHub, mailing list and IRC discussions).


Could mention here that linking to those discussions when doing review/concept ACKing is encouraged.

maflcko · 2019-06-11T09:36:17Z

Replaced "Design ACK" with "Approach ACK", since that seems to be more common.

CONTRIBUTING.md

ajtowns · 2019-06-12T10:04:01Z

Approach ACK, this seems to be an improvement to me.

moneyball · 2019-06-13T15:56:38Z

ACK fac5ddf

michaelfolkson · 2019-06-16T14:37:40Z

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?

fac5ddf doc: Rework section on ACK (MarcoFalke) Pull request description: `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`. ACKs for commit fac5dd: moneyball: ACK fac5ddf Tree-SHA512: 29177e8d96aeba055b5cad6d99be3ca1be0c61af0fdc90f70a3136872c9ad6201a02f63fbac78b90b8a56b4c06af304f2583d52a94fdd954fdcc7ad0552b9ef8

moneyball · 2019-06-16T17:08:37Z

@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.

jonatack

Post-merge review. Cheers.

jonatack · 2019-06-16T17:15:16Z

CONTRIBUTING.md

+#### Conceptual Review
+
+A review can be a conceptual review, where the reviewer leaves a comment
+ * `Concept (N)ACK`, meaning "I do (not) agree in the general goal of this pull


jonatack · 2019-06-16T17:17:10Z

CONTRIBUTING.md

+
+#### Code Review
+
+After conceptual agreement on the change, code review can be provided. It is


Suggestion: s/It is starting/A review begins/

jonatack · 2019-06-16T17:18:44Z

CONTRIBUTING.md

+
+After conceptual agreement on the change, code review can be provided. It is
+starting with `ACK BRANCH_COMMIT`, where `BRANCH_COMMIT` is the top of the
+topic branch. The review is followed by a description of how the reviewer did


Suggestion: s/branch. The review is /branch, /

jonatack · 2019-06-16T17:22:53Z

CONTRIBUTING.md

+the review. The following
 language is used within pull-request comments:

-  - (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.

fac5ddf doc: Rework section on ACK (MarcoFalke) Pull request description: `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`. ACKs for commit fac5dd: moneyball: ACK fac5ddf Tree-SHA512: 29177e8d96aeba055b5cad6d99be3ca1be0c61af0fdc90f70a3136872c9ad6201a02f63fbac78b90b8a56b4c06af304f2583d52a94fdd954fdcc7ad0552b9ef8

fanquake added the Docs label Jun 5, 2019

maflcko changed the title ~~doc: Rework section on ACK~~ doc: Rework section on ACK in CONTRIBUTING.md Jun 5, 2019

promag approved these changes Jun 5, 2019

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

moneyball reviewed Jun 5, 2019

View reviewed changes

maflcko force-pushed the 1906-docACK branch from fad8b05 to facb455 Compare June 5, 2019 14:41

practicalswift reviewed Jun 6, 2019

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

moneyball mentioned this pull request Jun 6, 2019

Package relay design questions #14895

Closed

maflcko force-pushed the 1906-docACK branch from facb455 to fabddde Compare June 6, 2019 13:57

fanquake approved these changes Jun 7, 2019

View reviewed changes

maflcko force-pushed the 1906-docACK branch from fabddde to fa570e5 Compare June 11, 2019 09:36

ajtowns reviewed Jun 12, 2019

View reviewed changes

CONTRIBUTING.md Outdated Show resolved Hide resolved

maflcko force-pushed the 1906-docACK branch from fa570e5 to fadcc2a Compare June 12, 2019 10:07

real-or-random mentioned this pull request Jun 13, 2019

Function to compute optimal ecmult_multi scratch size for a number of points bitcoin-core/secp256k1#638

Open

doc: Rework section on ACK

fac5ddf

maflcko force-pushed the 1906-docACK branch from fadcc2a to fac5ddf Compare June 13, 2019 14:08

maflcko added this to the 0.19.0 milestone Jun 14, 2019

maflcko merged commit fac5ddf into bitcoin:master Jun 16, 2019

jonatack reviewed Jun 16, 2019

View reviewed changes

maflcko deleted the 1906-docACK branch June 16, 2019 17:35

promag mentioned this pull request Jul 31, 2019

gui: Remove unused RPCConsole::tabFocus #16514

Merged

michaelfolkson mentioned this pull request May 18, 2020

Update legend to align with current Bitcoin Core review guidelines PierreRochard/bitcoin-acks#88

Open

michaelfolkson mentioned this pull request Sep 14, 2021

doc: Improve documentation around the ACK statement #22966

Closed

bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021


		#### Code Review

		After conceptual agreement on the change, code review can be provided. It is

doc: Rework section on ACK in CONTRIBUTING.md #16149

doc: Rework section on ACK in CONTRIBUTING.md #16149

Uh oh!

Conversation

maflcko commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Jun 5, 2019

Uh oh!

promag left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

morcos commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Jun 5, 2019

Uh oh!

Uh oh!

fanquake left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 11, 2019

Uh oh!

Uh oh!

ajtowns commented Jun 12, 2019

Uh oh!

moneyball commented Jun 13, 2019

Uh oh!

michaelfolkson commented Jun 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

moneyball commented Jun 16, 2019

Uh oh!

jonatack left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

maflcko commented Jun 5, 2019 •

edited

Loading

morcos commented Jun 5, 2019 •

edited

Loading

michaelfolkson commented Jun 16, 2019 •

edited

Loading

jonatack left a comment •

edited

Loading