github-merge: fetch ACKS and show them before signing off #16200

1. 
   laanwj commented at 8:16 am on June 13, 2019: member

   Currently it happens that I add a ACK comment while doing local testing before merge (so in the shell that it invokes) and this gets missed in the merge commit message.

   This is because it fetches the comments from github immediately after invoking the command. I think it’d be better if it did so later.

   Ideally, I’d like github-merge.py to fetch the ACKs and display them before confirming sign-off on the merge.

2. laanwj added the label Scripts and tools on Jun 13, 2019
3. laanwj added the label Feature on Jun 13, 2019
4. 
   fanquake commented at 9:56 am on June 13, 2019: member

   It looks like the script is also still missing ACKs in some cases. i.e in the https://github.com/bitcoin/bitcoin/commit/afab1312c5e151b8688ef62da8c4e7c4f542a1f4 merge commit we’ve got:

   0ACKs for commit 75485e:
   1  jonasschnelli:
   2    utACK 75485ef
   3  ryanofsky:
   4    utACK 75485ef. It's a simple, sensible fix.
   

   However looking at #16118 there are two other ACKs. A tACK from myself, and a concept ACK from PastaPastaPasta.

5. 
   MarcoFalke commented at 10:35 am on June 13, 2019: member

   @fanquake No it does not. If you write ACK and some random commit (or no commit at all), it doesn’t count as review.
6. 
   fanquake commented at 6:03 am on June 14, 2019: member

   @MarcoFalke Right. A concept ACK with no commit hash isn’t worth including. I also see why my ACK wasn’t included. My top line included a commit hash from a different PR, wheres’s as I used the correct commit hash in the review text.
7. laanwj referenced this in commit cc713030e9 on Jun 17, 2019
8. laanwj referenced this in commit 0e01e4522e on Jun 17, 2019
9. 
   hebasto commented at 1:14 pm on June 19, 2019: member

   @fanquake

   A concept ACK with no commit hash isn’t worth including.

   CONTRIBUTING.md does not mention any BRANCH_COMMIT following after Concept ACK or Approach ACK.

   Should this be corrected?

10. 
    MarcoFalke commented at 1:45 pm on June 19, 2019: member

    Maybe it should mention that only (Concept|Approach)? ACKs followed by the commit hash are included in the merge commit, so the reviewer can decide whether it is included or not.
11. MarcoFalke closed this on Jun 24, 2019

    ---

12. MarcoFalke referenced this in commit e115a21f79 on Jun 24, 2019
13. sidhujag referenced this in commit 107fd6235d on Jun 30, 2019
14. fanquake referenced this in commit b58763ba01 on Jul 7, 2019
15. HashUnlimited referenced this in commit b285af20ad on Aug 30, 2019
16. PastaPastaPasta referenced this in commit d200c8e1bf on Sep 11, 2021
17. PastaPastaPasta referenced this in commit 7b046505a5 on Sep 11, 2021
18. PastaPastaPasta referenced this in commit ede3de195d on Sep 12, 2021
19. MarcoFalke locked this on Dec 16, 2021

Contributors
laanwj fanquake MarcoFalke hebasto

Labels
Feature Scripts and tools