search

bitcoindevkit / bdk

A modern, lightweight, descriptor-based wallet library written in Rust!
Other
862 stars 310 forks source link

Merge commit like a pro (like bitcoin repo) #476

Closed RCasatta closed 2 years ago

RCasatta commented 2 years ago

We should start using github-merge.py (see https://github.com/bitcoin-core/bitcoin-maintainer-tools) to merge PRs, it includes a description in the merge commit containing the PR description, the list of commits included, the ACKs in the discussion, the sha512

example https://github.com/rust-bitcoin/rust-bitcoin/commit/615a900c88f1128b124fda76d821372bca61f796

notmandatory commented 2 years ago

Easy to setup! First bdk PR merge with this tool here: https://github.com/bitcoindevkit/bdk/commit/b2ac4a0dfd33e1286059d0bd090fd9137e2545ba

One possible downside is by creating a merge commit the git commit history of the original author get lost (except in the commit message). This sometimes happens anyway when the PR branch isn't rebased, but this scripts seems to make a new commit every time.

rajarshimaitra commented 2 years ago

I used this tool to merge https://github.com/bitcoindevkit/bdk-cli/commit/58386a05d3c3f7288ef30adb483da17e65e2f640.

Is there any way we can edit the commit message? Its simply copy pasting the PR description, which can be a little ill formed.

thunderbiscuit commented 2 years ago

Does the original author get credit for the commit? From what I see @LLFourn made the PR but @notmandatory gets the commit on master for #461. It's a small thing but I feel like proper attribution of contribution is an important part of open source software, particularly when people aren't paid to contribute.

Other drawback if we can't properly attribute: the right number of contributors is not reflected on the main GitHub page (any new contributors will not see their name/avatar there). This feels off-brand for BDK, since one of the core idea is to have one library that extends to many languages in order to bring more eyes on the one, single core codebase. I think it's nice if the GitHub repo can reflect the full breath of the people that have participated in the making of the library.

If we find a way to give proper credit to the commits' authors (or if I misunderstand what the tool does), then I'm all for it!

RCasatta commented 2 years ago

Hey @thunderbiscuit, yes I think you misunderstood the tool. Merge request author retains authorship on all his commits (maybe looking at the commit history you didn't find @LLFourn 's commits because they are older). Merge commits normally exists even without this tool, what this tool does is simply enriching the merge commit with additional metadata from the GitHub site and other improvements (signing+ots+sha512)

thunderbiscuit commented 2 years ago

Oh gosh I see them now ok. Sorry! Somehow I thought it was also squashing them into one and attributing them to the person doing the merge. Then it's a great idea!

notmandatory commented 2 years ago

@thunderbiscuit I agree this was a little hard to see at first, but I also check the github "insights" for contributors and with or without using this tool it looks like everyone's contributions are being recorded accurately.

afilini commented 2 years ago

We've been using this tool for a while, I think we can finally close this issue