Maintainers Policy

Maintainers are individuals who have the ability to merge changes into the main OpenAFS codebase. That power is granted based on trust that maintainers will act responsibly; the approval guidelines below lay out the general expectations for how maintainers will seek and provide review and approvals in the course of maintaining the codebase and merging changes to the repository, but leave flexibility for when a situation does not quite fit the mold or the right thing to do is a little different.

Approval Guidelines

The guidelines below provide information on what is needed before a commit can be merged into the main OpenAFS git repository. This is primarily of interest to the project maintainers (that is, the people doing the merging), but it can also be useful for individual contributors to know.

These are general guidelines with many exceptions, not a strict policy. Ultimately it is up to the maintainers to use their own judgement to decide whether to merge a commit.

Normal Commits

In general, we want a commit to have at least two reviews: a maintainer that provides a gerrit Code Review score of +2, and at least one other reviewer providing at least a +1 after reviewing the entire commit. Both of these reviews should come from someone that is not the primary author of the commit. The other reviewer can either be a maintainer or just another developer or community contributor that the maintainer trusts to provide meaningful review. Sometimes people review only part of a commit; that's fine, as long as the rest of the commit also gets reviewed to the same requirements.

And of course, commits require a +1 Verified score before they can be merged. This is usually provided by buildbot to verifying the commit on its spread of workers, though in some cases it will be provided by a human (such as when a code change is intrinsically limited to a single platform, a change is in a file not exercised by the build process, or when only the commit message has changed since a version of a commit that was verified by buildbot).

Larger Commits

If a commit is very large, or involves changes that seem risky or contentious, we want the commit to have a +2 from at least two maintainers, instead of just one. The decision to seek an additional +2 will typically be made by the maintainer that provides the first +2, but does not have to be (e.g., internal communication among the maintainers may cause a consensus to emerge naturally).

Commit Stacks

While commits are often examined and reviewed in the context of a branch or "stack" of commits, we merge individual commits one at a time, not as a whole branch. A commit earlier in a stack of commits can be merged while later commits still need changes; usually there's no need to wait for the whole branch to be done.

The one big exception to this is if a commit adds dead code that is used in a later commit. Normally we do not allow a commit to be merged if that commit adds dead code: a function, RPC, etc that is not called by anything in the tree. But if a branch contains commit A that adds a new function, and then another commit B that uses that function, that is okay. But commit A cannot be merged until commit B is also ready to be merged, and then they can be merged at the same time.

This approach aims to prevent adding dead code to the tree, but still allows for reasonable ways of splitting code up into commits that are easier to review.

Exception: Small Changes

If a commit is very unlikely to cause problems, or has been altered in very small ways since the last review, a maintainer can bypass the usual approval requirements according to their judgement.

Trivial changes tend to be whitespace-only changes, or changes to comments or preprocessor indentation, etc. Even actual code changes can qualify as trivial, if they are very small and you're very very sure about them. If you can mechanically verify that the change has not changed anything (e.g. git show -w), all the better.

There is no explicit "fast track" for small/trivial commits, but instead this serves as a broad exception to the normal rules for little things, just to help things move along.

But remember: if a change really is that trivial, it should be easy to get another +1 review for it! The exception allows for moving things along quickly when there's a reason to move quickly; just because a change qualifies for an exception doesn't mean you have to use one.

Exception: Responsiveness

If a maintainer can't get the needed reviews for a commit in a timely fashion because people are just unavailable, it's okay to bypass the usual approval requirements. Don't hold up all progress because one maintainer is on vacation and another one suddenly stopped responding. But record that you have exercised this exception, and make sure to discuss the change when people are available again.

What constitutes a "timely fashion" highly depends on the change in question and how urgent it is. Use your judgement.