doc: Add CODEOWNERS file #17094

Code owners (maintainers) will be added automatically to the "Reviewers" list for a PR and notified when a PR is opened.

See https://help.github.com/en/articles/about-code-owners for syntax.

Let me know if you want to be added for a specific path or file wildcard.

This could potentially add a lot of noise, fanquake does a good job labelling PRs and often requesting reviews anyway. @fanquake would this make your life easier? Or are you happy to keep requesting reviews from people manually where necessary?

Well someone requested this as it'd be useful to list maintainers somewhere.

What kind of noise are you afraid of? (FWIW, I'll comment out your entry if you don't want to be in this)

I like it because it adds more transparency to this project. However, is the term 'CODEOWNERS' a good term? This sounds like they own the copyright. I would prefer a file called MAINTAINERS.md in the markdown format.

ACK c3f117fd966367a5bb61dbdbd492ba6b67c97296 -- this information is useful

It may be useful to also list people who aren't specifically maintainers with merge access but have the domain specific knowledge and are usually the ones who review that part of code.

w.r.t noise, if the way Github adds people is similar to how Drahtbot adds labels, there could be a lot of false positives where something in another module is changed but isn't actually the focus of the PR and thus requesting a review from that person is unnecessary.

However, is the term 'CODEOWNERS' a good term? This sounds like they own the copyright. I would prefer a file called MAINTAINERS.md in the markdown format.

It has to be called CODEOWNERS since it's a Github specific feature that will trigger review requests.

@emilengler The CODEOWNERS file is magic in GitHub, and drives what reviewers are automatically added for PRs that affect certain directories/files in the source tree; it doesn't work with any other name. We could additionally have some .md file that documents the maintainers, but that would feel like duplicating efforts.

It may be useful to also list people who aren't specifically maintainers with merge access but have the domain specific knowledge and are usually the ones who review that part of code.

Right, this is not about committers; of these, @sipsorcery nor @theuni has merge access. People can be maintainers of a certain part, which means they review and ideally sign off on changes, it doesn't necessarily mean they merge them.

ACK c3f117fd966367a5bb61dbdbd492ba6b67c97296

ACK a887a61ca517883324a59c284776f491008d60f8

What kind of noise are you afraid of? (FWIW, I'll comment out your entry if you don't want to be in this)

I'm happy to be in it, I'm just worried it'll request a lot of reviews from people for PRs that even slightly touch files in their directory. Should be fine though I guess. I'd also be happy to just list maintainers in the contribution guideline doc or something so people know who to talk to.

ACK https://github.com/bitcoin/bitcoin/commit/a887a61ca517883324a59c284776f491008d60f8

You could also add sipa to specific files like Bech32 which he wrote

Concept ACK on more visibility into maintainers. Although I'm concerned that GitHub is going to assign reviews poorly (if they are anything like the review "suggestions" I see). For example, if someone adds a new file to the wallet dir, touching a makefile, does that mean Cory would get a review request as well? I don't think that's what we want.

Anyone feel free to correct me, but I fee like I've been having some good effect with targeted review requests lately, either requesting via GitHub or through a different channel like IRC. I wouldn't want everyone to start ignoring the GH review requests because they become spammy.

You could also add sipa to specific files like Bech32 which he wrote

You could also add achow101 across the PSBT files, practicalswift across all the scripts/linter type files etc. Just don't want to end up having to maintain the maintainers file.

You could also add achow101 across the PSBT files, practicalswift across all the scripts/linter type files etc.

Sure thing -- that would be *.sh, test/lint/ and ci/lint/ I guess.

I'd be happy to review everything touching src/test/fuzz/ too :)

Concept ACK

I'd also be happy to just list maintainers in the contribution guideline doc or something so people know who to talk to.

Sure, we could do that instead. But I do prefer a machine-parseable solution as that it's less likely to become out of date, it's clear soon enough when github assigns a wrong reviewer. And so that contributors don't have to go looking in the (fairly large) documentation for it.

Listing it per path also encourages dividing up the source code along different parts with different maintainers. The idea is also organizational.

I'm happy to be in it, I'm just worried it'll request a lot of reviews from people for PRs that even slightly touch files in their directory.

IMO that's a feature. Essentially, the idea is that you're always aware when someone proposes changes to files you maintain.

(thinking of it, this would be important for consensus-critical files, maybe we should add many reviewers there)

Then again if this is too controversial with the actual maintainers I'm happy to close this and never bring it up again.

ACK a887a61ca517883324a59c284776f491008d60f8

I kind of lost interest here, sorry. I think

• although things have been moving in the right direction, the boundaries between different areas of maintenance are not well-defined enough yet in this project (it just intersects in too many ways now)
• maintainers being afraid to be assigned unnecessarily

kind of closes the door on this, maybe after a few more years of (purposeful) code moves, this can be resurrected

I think it is fine to allow Bitcoin Core contributors to add themselves to this file, so that the get notifications about code they care about. It is unfortunate that the file is called codeowners, but I think adding a comment that "owners" are "reviewers" should make things clear.