github-merge: Add option to overwrite user email

[maflcko]

trafficstars

The merge script claims authorship of the merge commit, which is confusing:

• The maintainer didn't author the merged code, nor the merge commit. Everything is done by the script.
• In case of breach of a git hosting platform, such as GitHub, or the account of a maintainer, the hosting platform would allow a signed commit (signed by the hosting platform) to be pushed in the name of the maintainer. Thus, ...
• ... the important thing to check for is the commit signature or the actual code changes, not the name (or email) of the maintainer.
• Most modern software projects attribute their merges to a "merge" account, regardless of who triggered the merge. (For example, rust-lang)

Solve this issue by:

• Setting the git user name (committer and author) to "merge-script".
• (optional) Allowing the maintainer to provide an email to use for the merge commit.

Just as before, the merge commit is still required to be signed by the maintainer. Just as before, the git hosting platform will still denote which account was used to push the changes.

[maflcko]

Reopen from https://github.com/bitcoin-core/bitcoin-maintainer-tools/pull/112

[hebasto]

Recent merge commits, for example, https://github.com/bitcoin/bitcoin/commit/141115a0604001b8cce848804798dc174770a994, point at bitcoin-core-merge-script GitHub user. Is it required?

[maflcko]

No, it is not required. I think it is up to the project that is using this script what to set the optional setting to, or leave it unset.

At least on GitHub, it seems to be possible to claim commits of unclaimed email addresses. Unclaimed means that the email is not tied to any GitHub account. While there shouldn't be any risk, projects on GitHub may prefer to pick an email that is claimed.

The account you are referring to has an email tied to it on GitHub ([email protected]), which can not be re-tied to another account? Also, the account was registered with an email account that is now deleted, so logging in to it shouldn't be possible either. If someone still manages to log in to it, there doesn't seem to be any risk?

[maflcko]

See also the example output: https://github.com/bitcoin-core/bitcoin-maintainer-tools/pull/112#issuecomment-915969747

[maflcko]

Happy to have this closed, if there is no interest. (Again, setting the email is optional)

[fanquake]

cc @achow101 @glozow

[jonatack]

Concept ACK

[maflcko]

Looks like this has some Concept ACKs. Actual review of the 6 line python diff should be trivial, btw.

[maflcko]

Anything left to be done here or is this rfm?

[achow101]

Concept NACK, and I've reverted this in my own version of the merge script.

The maintainer didn't author the merged code, nor the merge commit. Everything is done by the script.

This argument does not make any sense to me. That's like saying git is the one authoring merge commits when you use git merge, rather than the user who called it. While the tool does the heavy lifting, the maintainer is still the author as they took the explicit action to perform the merge. If you really want to be pedantic, then the real "author" is the person who wrote the boilerplate text that the script outputs into the merge commit message, but it would be insane for every merge to be "authored" by that person.

Most modern software projects attribute their merges to a "merge" account, regardless of who triggered the merge. (For example, rust-lang)

I think that's kind of ridiculous for us since we do not do automated merging. Furthermore, I think that paradigm is a bit ridiculous in general since maintainers should be held accountable for their actions, and part of doing that requires knowing who did what action. Obscuring who made the decision to merge something reduces maintainer accountability.

I understand that the actual merger identity is still preserved in the PGP signature attached to the commit, but git hides that information by default. Obviously verification of the signature is required to be sure of the identity, but it is easy to run the verify commits script in a cronjob and therefore looking at the log wouldn't require signature verification at that time, so having the author be the merger is safe and useful.

Although even with the verify commits script malicious maintainers could misattribute the author field by setting the user and email to a different maintainer (as the script does now), but that is easy to mitigate by having the script check that the name and email exists as a UID on the PGP key.

[maflcko]

so having the author be the merger is safe and useful.

Ok, makes sense to revert, if it is too controversial, or causing more confusion than it fixes. I still think the first of the two commits is useful, because the merge commit is not meant to do more involved stuff (like solving (silent) merge conflicts, or even trivial typos), which would be possible with a "normal" git merge outside the script. But after all I think it shouldn't matter much one way or another, so if it is too controversial it seems better to revert it.