contrib: gh-merge: Include ACKs in merge commit #15643

pull MarcoFalke wants to merge 1 commits into bitcoin:master from MarcoFalke:1903-ghMergeAck changing 1 files +29 −4

MarcoFalke commented at 4:07 PM on March 22, 2019: member

This includes all up-to-date ACKs in the merge commit for reference
contrib: gh-merge: Include review comments in merge commit fa1c073154
fanquake added the label Scripts and tools on Mar 22, 2019
laanwj commented at 4:19 PM on March 22, 2019: member

I think including all review/PR comments in the merge commit is overkill, it already tends to be hard to read the history with git log -p because the long, not-for-console formatted text that gets included from the opening post.
MarcoFalke renamed this:
~~contrib: gh-merge: Include review comments in merge commit~~
contrib: gh-merge: Include ACKs in merge commit
on Mar 22, 2019
MarcoFalke commented at 4:30 PM on March 22, 2019: member

With review comments I mean only the ACKs (adjusted title)

MarcoFalke commented at 4:34 PM on March 22, 2019: member

Example:

commit 67e54064294326b8fba1c420aaa1b8afb084bf25 (HEAD -> pull/15463/local-merge)
Merge: abd914ed34 643eba0aa2
Author: MarcoFalke <falke.marco@gmail.com>
Date:   Fri Mar 22 12:33:22 2019 -0400

    Merge [#15463](/bitcoin-bitcoin/15463/): rpc: Speedup getaddressesbylabel
    
    643eba0aa2 rpc: Speedup getaddressesbylabel (João Barbosa)
    
    Pull request description:
    
      Fixes [#15447](/bitcoin-bitcoin/15447/). Same approach of [#14984](/bitcoin-bitcoin/14984/), this change avoids duplicate key check when building the JSON response in memory.
    
    ACKs for commit 643eba:
      ryanofsky:
        utACK 643eba0aa262ead636d5de18ced31b6f166ec033
      MeshCollider:
        utACK https://github.com/bitcoin/bitcoin/pull/15463/commits/643eba0aa262ead636d5de18ced31b6f166ec033
    
    Tree-SHA512: ee0941130ada600171d36cf69219921ab2fa044402cedc57cdf73bdb23499555c53bc076d65ef117d0a8f5cbc5381a49e40d945766796241936a701678b72ab6

laanwj commented at 4:34 PM on March 22, 2019: member

With review comments I mean only the ACKs (adjusted title)

Oh that does sound good!
MarcoFalke commented at 5:13 PM on March 22, 2019: member
see merge commits for a live example:
- 68520597ccf8ff3f6e8a7ad6869b06bf2012ae8a (ACK includes the full line, with trailing comment)
- 8a8b03ecd2218dcdbcbf3127f2fa94f0f0da4698 (ACK pulled from issue comment and review comment)
jamesob commented at 7:13 PM on March 22, 2019: member

Nice! utACK https://github.com/bitcoin/bitcoin/pull/15643/commits/fa1c073154c6a39dca878f5c9a37abee8af0fd30
jnewbery commented at 7:21 PM on March 22, 2019: member

Concept ACK
practicalswift commented at 7:26 PM on March 22, 2019: contributor

Concept ACK

Nit: If I'm reading the code right also NACK:s will be included under the heading "ACKs for commit […]". If that is intentional then perhaps change heading to "ACK/NACKs for commit […]" to clarify?
MarcoFalke commented at 10:28 PM on March 25, 2019: member

@practicalswift No, it would only show up if you NACK+the commit id, which is rarely (never) done.
sipa commented at 10:52 PM on March 25, 2019: member

Concept ACK
practicalswift commented at 9:51 AM on March 27, 2019: contributor

@MarcoFalke Thanks for the clarification. I missed the and head_commit in l :-)

utACK fa1c073154c6a39dca878f5c9a37abee8af0fd30 (nit: a run with your yapf script would make it even more beautiful :-))
laanwj commented at 10:55 AM on March 27, 2019: member

utACK fa1c073154c6a39dca878f5c9a37abee8af0fd30
laanwj merged this on Mar 27, 2019
laanwj closed this on Mar 27, 2019
laanwj referenced this in commit 848ec5603f on Mar 27, 2019
MarcoFalke deleted the branch on Mar 27, 2019
fanquake referenced this in commit 93394898a4 on Jul 7, 2019
PastaPastaPasta referenced this in commit 0bb6e9000e on Sep 11, 2021
DrahtBot locked this on Dec 16, 2021

Contributors

Labels

Linked (view graph)

#15816 scripts: GitHub merge script is missing review comments