doc: Explain how NACK commit_id works #21408

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:2103-docNack changing 1 files +2 −0

MarcoFalke commented at 5:52 PM on March 10, 2021: member

No description provided.
doc: Explain how NACK commit_id works faa7b6a223
in CONTRIBUTING.md:332 in faa7b6a223
```
 328 | @@ -329,6 +329,8 @@ A review can be a conceptual review, where the reviewer leaves a comment
 329 |     request",
 330 |   * `Approach (N)ACK`, meaning `Concept ACK`, but "I do (not) agree with the
 331 |     approach of this change".
 332 | + * `NACK commit_id`, meaning `Concept ACK`, but "I do not agree with the
```
JeremyRubin commented at 6:01 PM on March 10, 2021:

Maybe add:

a NACK commit_id where commit_id == HEAD~N serves to NACK all commits commit_id..HEAD. Following future commits or rebases which purport to fix the issue, NACK commit_id must be explicitly removed in light of future commits which fix the issue or if the commit_id is no longer present. A commit ID is not sufficient rationale to justify a NACK. NACKs should be placed on the current tip commit ID at the time of comment, and clarifications around specific issues with non-tip commits should be described in the rationale.

MarcoFalke commented at 6:10 PM on March 10, 2021:

I think common sense can be used to derive that a NACK can't be cleared by simply changing the commit_id. Using that many words to describe a simple concept seems overkill and makes the documentation bloated and harder to parse for newcomers (which are the primary audience for this).

laanwj commented at 8:51 PM on March 10, 2021:

Would agree a problem is that commit IDs change all the time when people rebase, amend, etc, I think it's better to NACK by commit title or subject. Or write out your Concept ACK but … it seems this particular situation is rare enough I'm not sure we need a specifically documented abbreviation case for it. But maybe that's just me.

MarcoFalke commented at 7:17 AM on March 11, 2021:

Ok, closing for now
DrahtBot added the label Docs on Mar 10, 2021
MarcoFalke closed this on Mar 11, 2021
MarcoFalke deleted the branch on Mar 11, 2021
DrahtBot locked this on Aug 16, 2022

Contributors

Labels