Git Pull Request Auto-complete Is Usually Bad

Thoughts on Git PR Auto-Complete usage.

Posted on 2022 January 9

I don’t like participating in pull requests that automatically close when the first approval is received. I really prefer that the author of the pull request closes the pull request after approvals are received.

At its core, auto-complete on pull requests does something that irks me: Auto-complete passes the responsibility of the PR to whoever comes next, and that’s bad.

Auto-complete assumes that reviewers are fully aware of the codebase’s functions, the context of why these changes are being submitted, and the context of what the new changes to the code will be doing. In practice this is rarely true. If a reviewer isn’t familiar with the codebase, then they can check for syntax issues and some local structural issues, but they won’t be able to completely review what’s going into the codebase. I also do not think approval should be reserved exclusively for veteran programmers.

When someone approves a PR marked for auto-complete, if someone else happens to be reviewing the code at the same time, an entirely new PR will need to be opened to address any feedback after the first approval.

Compare this to a situation without auto-complete. Unless the author is watching their PR like a hawk, then it’s unlikely that they would have closed the PR immediately after the first reviewer came through.

Auto-complete is an innocent but dangerous practice.

Problems

Reviewers Don’t Have Context

Reviewers may not have full context of the changes, and can approve a PR based solely on being valid code, but not being the right code to check in.

Example: Invalidating an API contract by making assumptions on format (should everything be a guid, or do the inputs just happen to be guids?).

If I have no knowledge of the codebase, and I approve, then I remove the opportunity for someone with more familiarity to review the code. Most reviewers won’t read through closed PRs unless there’s a bug. I do not want to take away the chance to review this PR from a more familiar code maintainer, so I simply will not vote, and instead rely exclusively on commentary to review.

However…

All Comments Are Blocking Comments

If a reviewer asks a question but chooses not to vote on the PR, and a second reviewer comes along and approves it, then the first comment is lost. This means if there is any dangerous ambiguity that needs to be settled, then any comments must become blocking comments. The commenter should block the PR until the questions are answered, and then must be summoned to change their vote later (unless votes get reset).

Verification Becomes the Reviewer’s Responsibility

When a PR is marked for auto-complete, then verification of the pull request is now the responsibility of the first person to approve.

Auto-complete implies that the author of the code is finished looking at it, and they are only interested again should a blocking response come in. This is lazy shepherding to me. The author of the PR should do a final-pass and be the one to decide when it should be completed.

Automatic Deployments are Automatic

If the team has CICD wired up, then a deployment will begin. Hopefully this is not to production.

Also keep in mind that a reviewer may approve the code outside of the author’s working hours, meaning if it goes wrong, then the author may get summoned to address it.

Never Auto-Complete a Deployment to Production

Don’t do it. Don’t deploy production unless you’re there to watch it, at least for the first couple servers/nodes.

When Auto-Complete is Okay

Auto-complete is completely acceptable and encouraged when a PR has approvals, but there are background processes waiting to finish. For example: The build pipelines are still running to produce a candidate build. In that case, tag it for auto-complete! ✅

Be responsible with auto-complete. Don’t pass on the responsibility to someone else.