RFC: "Require" pre-commit review for all PRs that change the compiler

What do people think about making it a policy that all non-trivial pull requests require pre-commit review? This is still very loose, but just means that there’s someone who’s taken a look at the code you just wrote.

Non-exhaustive list of examples of “trivial pull requests”:

  • Adding tests
  • Fixing typos
  • Reverts of your own commits
  • Reverts of things that are breaking the build
  • Disabling tests that are breaking the build
  • Cherry-picks that have already been reviewed elsewhere
  • but not refactoring
  • probably not even adding substantive comments or docs

(To be clear, this policy would only apply to the compiler itself for now. The SourceKit and stdlib folks can decide if they want to opt in as well, and the other repositories will probably continue to have their own policy.)

11 Likes

Note that anyone can count as the reviewer, not just “Apple people” or “people in the CODE_OWNERS file” or even “people with commit access” (maybe you have commit access and you’re improving something an external contributor did originally). It’s just “someone other than you”.

Sounds good to me. It would help a bit with the knowledge siloing issues we have.

3 Likes

Hi Jordan,

What’s motivating this change? Is the existing “trust but verify” system of post-commit review breaking down somehow, and if so, then how?

I’m asking because as an external contributor, it can be hard to know who to add as a reviewer. On the one hand, the code owners file can be helpful, but the owners tend to be busy folks. On the other hand, using a “git blame” to guess at potential reviewers isn’t as helpful as it first appears due to “drive by” changes, gardening, etc. Finally, even if one does know who to add, mapping git email addresses to GitHub user handles can be frustrating.

Dave

@jrose I agree with DaveZ. This is a big change, what is the motivation here? Can you provide specific situations that you want to eliminate? I just want to understand your concerns in an explicit way, rather than implicitly what I think they may be.

GitHub has a nice feature where it recommends reviewers for you based on people who also recently touched the code you modified. As far as “why”, @slava_pestov has a pretty nice manifesto (written without swift in mind, but generally applicable): https://gist.github.com/slavapestov/9ddc0a3b79b7be8a69e38f50e72ee46f

1 Like

I’ve seen a number of changes go in lately from both Apple and non-Apple people where they really could have used a second pair of eyes—not because the code is wrong, but because there might have been a better way to do something. Even when this isn’t the case, Joe’s point about information siloing is really important. When people change something, there’s often a bit of follow-up afterwards (did it break a bot? why was it done this way? how do I update my patch?), and having there be more than one person to ask is a really good thing.

But honestly, the idea that someone could commit something without explaining it to anyone else just seems like bad practice—like, the same kind of smell as “not running PR tests before merging”.

3 Likes

This also isn’t a sudden or new idea. I’ve been wanting to push it for a while, but just haven’t brought it up in anything more than casual discussions with other Apple folks. In practice, nobody is doing post-commit review.

Hi Jordan,

I don’t see what you’re proposing as fundamentally different than the model we have today. If people feel that a commit should have been reviewed before merging, they should not be afraid to just say so to the committer. Soft “requiring” pre-commit review will not change the fundamental social/communication problem. On the contrary, it’ll just shift the problem to the definition of “nontrivial”.

Dave

Right now I think people have a notion that “commits don’t need review unless they’re unusual, or complicated, or in a component I don’t usually touch”. I want to change the default.

I guess I’m struggling to see what changes. Isn’t “unusual, or complicated, or in a component I don’t usually touch” the definition of nontrivial? What does your definition cover that isn’t covered by the existing conventions?

People have a tendency to say to themselves “how can I justify asking for a review”. The real question could be, “how can I justify not asking for a review”

3 Likes

The last several projects I’ve worked on had an “everything gets review” policy, and while it takes a little initial adjustment I find it’s strongly advantageous. I’m in favour of this.

In addition to what Slava mentions in his post, it also increases situational awareness: more people know about each change, so have a chance of preemptively pointing out or fixing related issues, eyeballing regressions, or simply knowing what areas are undergoing what sorts of changes.

8 Likes

I agree. I believe that the everything needs a review policy is a good move. I’ve worked on several projects where merging is just blocked outright without a review.

Like @Graydon_Hoare says, it takes some getting used to, and can be annoying if the person who knows that bit of code is away (this is addressed in the gist posted above). But it serves to make sure that every bit of code in theory has had someone else’s eyes on it.

I agree that all of those things are good. I just want to be clear that requiring pre-commit review is not cost free in the best of scenarios. @Slava_Pestov mentions this in his post, but he doesn’t address how external contributors should handle the “unresponsive reviewer” problem given that they can’t “walk over” / “call the manager”, or easily “take ownership” / “find someone else to mentor”. I’d like to know what the plan is here for Swift as an open source project.

Also, if this policy change is enacted, I’d like whatever documentation gets written to be clear about when a reviewer should decline to review. In other words, if people don’t feel like declining a review is an option, then the review process starts trending towards “check the box” culture.

1 Like

Good points. I think that’s what the existing “code owners” file is for—if your reviewer is unresponsive, you ask the code owner who to ping instead. If the code owner themself is unresponsive, you ask Ted, as leader of the project.

When to decline a review is also a great point. I think that would look something like this:

  • If you don’t think you’ll understand the problem even with the PR author explaining it to you
  • If you know someone else might have concerns (tag them in)
  • If you know you are too busy to give a timely review (in this case you should suggest someone else)

…and that might be it, because reviewing isn’t supposed to be a high bar. It does mean that we people who are paid to work on Swift need to be able to consider reviewing as part of their everyday work, something that @Michael_Gottesman mentioned as a concern offline.

+1 from me, albeit echo-ing @DaveZ’s concerns. Frequent contributors suffer from the curse oftribal knowledge, i.e. they just know multiple potential reviewers for any change. Without this knowledge, I imagine it to be very difficult as GitHub’s recommendations are frequently wrong.

Having a clear and prominent escalation plan with time frame guidance is essential. Example, using a 1-week max:

(hopefully step #5 never happens)

+1. We should encourage prompt declining (ideally with recommendations) as helpful to the PR author in finding a more suitable reviewer.

1 Like

I think pre-commit review is a fine policy. I like the idea of a clear escalation path that @Michael_Ilseman outlined. I also sympathize with the concerns the @DaveZ raised about infrequent contributors having a harder time getting reviews. On the other hand, all contributors without commit access (all brand new contributors?) are already in this situation, right? Is this a significant problem for our new contributors?

SourceKit ought to have the same review policy as the compiler. I would oppose any change that didn’t apply to both.

I’d like to make the case that this RFC is getting ahead of itself. We need to focus on identifying the problems first before deciding that mandatory pre-commit review is the best solution rather than just the first solution people thought of or the quickest to implement.

As a preface, I want to be clear that I’m not against code review. In fact, I think more people should do code review more often. And I’ll freely admit that I’m human and I’ve sometimes been overly eager or overly optimistic about my changes before they went in. But requiring pre-commit review is not a change that should be made lightly. There are hidden costs, hidden risks, the gains are not guaranteed, and there are often better solutions.

As I see it, there are three core motivating concerns identified so far:

  1. The concern that not enough pre-commit or post-commit review is happening.
  2. The concern that “information silos” are developing.
  3. The goal that more people should be able to explain/justify/help after disruptive changes land.

I’d like to acknowledge these topics, suggest alternatives, and challenge the suggestion that mandatory pre-commit review is the “best” solution in each case – and I’d like to discuss these topics in reverse order:

First, if the community wants to decrease the short-term pain of disruptive changes, then committers need to learn that the community expects them to work harder at their communication and planning skills. Reviews – mandatory or not – are just one small step along the way of good communication/planning. If the community cannot be clear with each other about what kinds of disruptions are tolerable and which should have been planned out better, then mandating pre-commit reviews won’t magically fix the social dysfunction. In contrast, if the community is good at planning and communication, then mandatory pre-commit reviews are at best an unnecessary drag on productivity, and at worst an institutional bias against change itself.

Second, if the community is concerned about “information silos”, then more long-term communication needs to be done. Maybe that’s in the form of documentation, or a blog post, or a paper, or a talk. It really doesn’t matter. The important goal is that expertise needs to be distilled and recorded. In contrast, commit reviews – mandatory or not – are an incredibly noisy, raw, jargony, and ephemeral process. At best, one or two peers can passingly keep up with the firehose, but the community as a whole gains nothing. Making reviews mandatory doesn’t change anything in this regard, and if anything, it dilutes the value of reviewing and requesting reviews.

Third, I find the concern that not enough pre-commit or post-commit review is happening to be paradoxical. If people feel that way, then they’re actually paying attention, but they must feel that other people are not. If you feel that way, then it is your obligation to say something. You need to tell the people that are paying less attention than you do that something is going on that they should care about. It doesn’t take much. Maybe just a quick “FYI, you might want to look at this” email or a “mention” in GitHub. The important thing is that the social norm is established that we’re all expected to look out for each other. In contrast, if a engineer habitually ignores the prodding of the community, then mandating pre-commit review won’t magically convince them to care. It’ll just add friction to well behaved committers.

Dave

1 Like

In the same spirit as a code review, I’d ask “What could go wrong?” with this idea. The first answer that springs to mind is gaming the system. How could one game the system? It might bias people toward trivial changes, or it might lead to people picking reviewers who would be sympathetic—I don’t know. Could lead to “I’ll scratch your back if you scratch mine.”

I’m not for or against here—just want to ask about unintended consequences. Would also ask how the status quo came to be. I bet there was wisdom in its devising. Has something changed? Scale? Complexity? Malleability?