Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Mar 27, 2019

Conversation

Projects
None yet
7 participants
@MarcoFalke
Copy link
Member

MarcoFalke commented Mar 22, 2019

This includes all up-to-date ACKs in the merge commit for reference

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 22, 2019

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 MarcoFalke changed the title contrib: gh-merge: Include review comments in merge commit contrib: gh-merge: Include ACKs in merge commit Mar 22, 2019

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 22, 2019

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

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 22, 2019

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: rpc: Speedup getaddressesbylabel
    
    643eba0aa2 rpc: Speedup getaddressesbylabel (João Barbosa)
    
    Pull request description:
    
      Fixes #15447. Same approach of #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

This comment has been minimized.

Copy link
Member

laanwj commented Mar 22, 2019

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

Oh that does sound good!

@MarcoFalke

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 22, 2019

see merge commits for a live example:

  • 6852059 (ACK includes the full line, with trailing comment)
  • 8a8b03e (ACK pulled from issue comment and review comment)
@jamesob

This comment has been minimized.

Copy link
Member

jamesob commented Mar 22, 2019

Nice! utACK fa1c073

@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 22, 2019

Concept ACK

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Mar 22, 2019

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

This comment has been minimized.

Copy link
Member Author

MarcoFalke commented Mar 25, 2019

@practicalswift No, it would only show up if you NACK+the commit id, which is rarely (never) done.

@sipa

This comment has been minimized.

Copy link
Member

sipa commented Mar 25, 2019

Concept ACK

@practicalswift

This comment has been minimized.

Copy link
Member

practicalswift commented Mar 27, 2019

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

utACK fa1c073 (nit: a run with your yapf script would make it even more beautiful :-))

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 27, 2019

utACK fa1c073

@laanwj laanwj merged commit fa1c073 into bitcoin:master Mar 27, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Mar 27, 2019

Merge #15643: contrib: gh-merge: Include ACKs in merge commit
fa1c073 contrib: gh-merge: Include review comments in merge commit (MarcoFalke)

Pull request description:

  This includes all up-to-date ACKs in the merge commit for reference

Tree-SHA512: 32c9352d884f9ecf94940f50f2921fc9fc026083c120f54d0651a41814872e852aee8d0c4ad5bcd03292329f05d76fcb7bac11741e1dd3bf417211a186005afb

@MarcoFalke MarcoFalke deleted the MarcoFalke:1903-ghMergeAck branch Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.