Skip to content

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

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.

Sign up for GitHub

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

Jump to bottom
Merged
laanwj merged 1 commit into from
Mar 27, 2019
Merged

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

laanwj merged 1 commit into from
Mar 27, 2019

Conversation

@maflcko
Copy link
Member

@maflcko maflcko commented Mar 22, 2019 •
edited
Loading

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

@laanwj
Copy link
Member

laanwj commented Mar 22, 2019 •
edited
Loading

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.

@maflcko maflcko changed the title contrib: gh-merge: Include review comments in merge commit contrib: gh-merge: Include ACKs in merge commit Mar 22, 2019
@maflcko
Copy link
Member Author

maflcko commented Mar 22, 2019

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

@maflcko
Copy link
Member Author

maflcko commented Mar 22, 2019

Example:

commit 67e54064294326b8fba1c420aaa1b8afb084bf25 (HEAD -> pull/15463/local-merge)
Merge: abd914ed34 643eba0aa2
Author: MarcoFalke <[email protected]>
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
Copy link
Member

laanwj commented Mar 22, 2019

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

Oh that does sound good!

@maflcko
Copy link
Member Author

maflcko commented Mar 22, 2019 •
edited
Loading

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
Copy link
Contributor

jamesob commented Mar 22, 2019

Nice! utACK fa1c073

@jnewbery
Copy link
Contributor

jnewbery commented Mar 22, 2019

Concept ACK

@practicalswift
Copy link
Contributor

practicalswift commented Mar 22, 2019 •
edited
Loading

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?

@maflcko
Copy link
Member Author

maflcko commented Mar 25, 2019 •
edited
Loading

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

@sipa
Copy link
Member

sipa commented Mar 25, 2019

Concept ACK

@practicalswift
Copy link
Contributor

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
Copy link
Member

laanwj commented Mar 27, 2019

utACK fa1c073

@laanwj laanwj merged commit fa1c073 into bitcoin:master Mar 27, 2019
laanwj added a commit that referenced this pull request Mar 27, 2019
@laanwj
Merge #15643: contrib: gh-merge: Include ACKs in merge commit
848ec56 
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
@maflcko maflcko deleted the 1903-ghMergeAck branch March 27, 2019 12:40
@fanquake fanquake mentioned this pull request Apr 15, 2019
Closed
fanquake pushed a commit to fanquake/bitcoin that referenced this pull request Jul 7, 2019
@fanquake
contrib: gh-merge: Include review comments in merge commit
9339489 
Github-Pull: bitcoin#15643
Rebased-From: fa1c073
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 11, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#15643: contrib: gh-merge: Include ACKs in merge commit
0bb6e90 
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Scripts and tools

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@maflcko @laanwj @jamesob @jnewbery @practicalswift @sipa @fanquake