search

dtr-org / unit-e

A digital currency for a new era of decentralized trust
https://unit-e.io
MIT License
45 stars 15 forks source link

process: GitHub merge from command line #892

Open scravy opened 5 years ago

scravy commented 5 years ago

Out current review process does not allow reviewing the commit message. The accepted agreement is that we take the description as commit message for the squash commit.

This could be automated, and the bitcoin guys have done that. They perform merges using a script from contrib called github-merge. It has several more features: It retrieves all the comments and preserves ACK, utACK, etc.

See example commit message created from it: https://github.com/bitcoin/bitcoin/commit/8dbb2c5e67043a4ba3bff9030fd465c28bb21f96

We would need to change it a bit, make sure it contains the Signed-off-by and setup an api token, I guess. It could also take care of Co-authored-by (Example commit with co-authored-by messages: https://github.com/dtr-org/unit-e/commit/b63c788bb79a03927d39b302040a2bcae91b41cd).

cornelius commented 5 years ago

Having a script helping with merges and including things like the ACKs might be helpful, but I'm not sure it solves the problem of reviewing the commit message.

The description of the pull request is not part of the git history so it also is not part of the review mechanism. There is no way to refer to or ACK a specific version of the description. So if the correct version of the description gets into the commit and if it contains all the necessary information is still ultimately up to the person doing the merge.

The referenced bitcoin commit is actually a good example of this issue. The commit message of the merge commit contains the commit message of the first commit to the pull request, because that's what GitHub inserts into the description by default when creating a pull request. But it does not contain anything from the second commit which was added to the pull request later. So the resulting commit message is a bit inconsistent. Leaving out adding the description of the pull request would have been the clearer solution there, because the commit messages are present in the merged commits and referenced from the merge commit.

I tend to think that the better way to avoid issues with the commits created when merging pull requests is to make it simpler and avoid having to do work such as creating the commit message at the time of merging. Using a script to do that feels like adding more complexity and moving parts which potentially could cause trouble. So it would be good if using the script would be optional and not a required part of the workflow.

The clean way in terms of reviewing merges would be to use merge commits instead of commits squashed by GitHub. Then the exact commits as reviewed and approved in the pull requests would end up on master. Commits could be squashed by developers as needed on the pull request branch and then (re-)reviewed by others.

Doing this would also eliminate the potential for mistakes in the cases we currently treat as exceptions, where we do merge commits instead of squash commits.

Not requiring squashes would also solve the problem of assembling Co-authored-by trailers as the more natural way would be to keep the commits of different people intact when merging, so that it isn't required to create trailers at merge-time.

It seems like we should have a discussion about merge vs. squash commits first, and then decide if we want to adapt the github-merge script to whatever workflow we decide on.