Git pull request review strategies from three dev teams

Most of us know the definition of a git pull request:

Software developers submit a pull request (often abbreviated to PR) in their git system like GitHub, GitLab or BitBucket to signal to their teammates or manager that a branch or fork they have been working on is ready for review.

The git pull request usually happens in the software development process after coding and before merge.

On the Cycle Time diagram below...

PR issued is the act of the software developer submitting the pull request in the git system.

PR pick up time is the time it takes for the developer's teammate to accept the request for review.

PR picked up is the act of the developer's teammate starting to review the pull request.

PR review time is the time it takes for the developer and their teammate to go back and forth and incorporate changes from the review process.

PR merged is the act of merging the pull request into the base branch or fork after the peer review process has been completed.

Cycle time is one of the most important engineering metrics. Click here to learn about the metrics that matter for dev leaders.

Git pull request origin story:

Linus Torvalds invented the pull request shortly after he invented git. Before the cloud, developers would write new code on their local machine and other people working on the same project would "pull" it to help determine quality and relevancy . This is how the git pull request was born.

That's the definition and the origin...

But what is the actual point of a git pull request? What is the real reason for this step in the software development process?

If you ask 10 software engineers you will get 10 different answers.

I decided to ask one. Not just any engineer though.

Naom Hofshi knows about git pull requests. He's a really talented developer on my team at LinearB.

The last three software engineering teams he's worked on handled them in three completely different ways so he has a unique perspective.

But I'm jumping ahead.

It is common for different dev teams to use different coding, review, version control, testing, and release processes.

But how big are the differences from team to team?

The following is a summary of Noam's three experiences with three different approaches to git pull requests including the pros and cons of each approach.

1. No Review 

It may come as a surprise to some of you, but some teams use no pull requests in their git system at all during the development process.

Noam experienced this practice working on a small team (around 20 developers) creating internal infrastructure tooling, where there were no external customers.

"Our customers were other internal developers so they gave us feedback very quickly and we were able to implement fixes quickly if there were any."

The team tested their code before pushing to their production branch (typically called 'master' on GitHub), so didn't feel the need to conduct reviews, and found that their tight feedback loop meant they were able to move quicker with features and fixes than constantly stopping for reviews. This gave him a better feeling of completion. We have all sat waiting on hold on one feature waiting for a review, started another task, and then needed to switch context back to the older task when someone reviews it.

The same process may make sense for other small and tight teams working on tools that end users don't directly interact with.

There are times when a PR doesn't make sense. If an established contributor with full access to a repository wants to correct a small typo, then maybe a PR is unnecessary.

The negatives of this approach are while you feel like you are progressing as a team, it's hard to know if you actually are. As there is little feedback from colleagues on code standards or best practices, this lack of an opportunity to learn was Noam's biggest negative with the process. He missed the chance to learn techniques others are using, but more importantly, to learn about the rest of the codebase. Noam recalls one time when after a year of working on the project, the team discovered a file that consisted of over 1500 lines of code that repeated functionality of multiple other areas of the codebase. And if a team member is away, then few others know areas of the codebase they haven't worked on themselves, and the pace of development slows.

Alternative - Pair Programming

An alternative to "no pull requests" that results in some similar effects to review is pair-programming. If programmers work together on features and fixes, then they have the chance of experiencing some of the benefits of reviewing a pull request on your git system, but directly and in near real-time. However, pair programming is time-consuming, absorbing two (or more) of your developer team for a potentially long time. It's hard to say if their time is better spent elsewhere, but we recommend you try pair programming alongside a PR review process.

Prerequisite - Automated Testing

Before setting up any form of PR-based development process (especially basic review), we strongly recommend you set up automated testing. Set up as much as you have time for, or is important to you including linting, unit tests, and integration tests. In the long run, having these tests setup helps you move towards continuous integration, delivery, and deployment should you want to in the future.

If you are using GitHub for pull requests, then there are basic options for automation in the form of GitHub Actions. You can enable style checkers, linters, and test runners for a wide variety of programming languages to run on your GitHub repositories with a couple of lines of configuration, and no extra infrastructure.

Noam's take on no review:

"If you have enough testing - unit tests, integration tests, etc. - you might be able to merge without a git pull request and everything will be fine. But even then you still need a way to make sure each developer is adhering to code standards."

So to net it out, Noam isn't buying this approach as a good option for most teams.

2. Basic Review

If you are using pull requests with your git project, then this is probably the most common approach that teams follow.

I asked Noam how his second company approached "basic" reviews.

"We review contributions briefly and mostly just the diff."

Diff meaning only the differences between the current code and the contribution of the change.

The GitHub, Gitlab and BitBucket user interfaces have a lot of features and tools available to review contributions, from drafting requests, to discussions, suggesting changes, referencing other conversations and contributions, and more once you add actions as mentioned above.

When using basic git pull request reviews, you generally rely on automated testing to check contributions. If the tests pass, then you know that the code submitted works in its particular discrete context, but you have less idea of its effect on the wider codebase, or for someone using the application. And you still haven't filled the knowledge gap. You know the code "works," but the coder(s) who created it still have the most knowledge about what the code does and how it was written.

You could also argue that you cannot test everything, or to be more precise, test everything comprehensively. There are certain changes that only human reviewers may notice, or express an opinion on the effectiveness of them as opposed to the correctness.

Noam's take on basic review:

"Faster time to merge matters because it's good for the team and it's good for me. I like to finish what I'm doing and then move on and try new things. But sometimes I got the sense I was not progressing. We didn't take the time to dig into certain things so I wasn't learning as much about why our code worked a certain way."

In summary, Noam sees pros and cons to basic review but overall doesn't find it to be the most rewarding longterm approach for developers.

3. Full Review

A full review of GitHub pull requests may take a variety of different forms, but characteristics include:

• Significant review "depth" meaning 5 or more comments and 10 or more comments for substantial PRs.
• Multiple reviewers for substantial PRs (PRs with 20 or more code changes)
• 1:1 or group conversations during the review process for substantial PRs
• Full documentation of all comments from both synchronous and synchronous communications about all PRs.

Noam thinks of it as "Would another developer be able to step in and take over the PR after the review process without explanation? If so, you're probably doing a thorough review.

When doing a full review, you should check out the branch, build or run the branch, and test the feature or fix yourself as much as possible outside of automated testing. On any pull request in GitHub, below the discussion, you can see the branch details you need to check it out, and push to if you make further changes yourself.

Branch details in Pull Request

The reviewer(s) and the coder(s) should ask questions to each other about the code and implementation, and try to understand the approach and the code. Through all this process, it's important to maintain clear and constructive communication, and take questions as just that, and not a criticism of your work.

This process, of course, takes longer, but should pay off in the long run with less downtime between staff availability and changes, more efficient code, better developer education on alternative techniques and practices, increased developer awareness of the product, reduced double handling of similar issues, and better developer communication.

Noam's take on full review:

"It's good because it causes us to share with each other and helps us to learn more. Plus I think you actually get faster in the long run by slowing down a bit up front and going through a thorough review process. And I've noticed that teams that do expansive reviews also seem to have cleaner code bases which makes it easy to update and fix things."

There you have it. Noam sees some value in the other approaches but overall prefers working on teams that invest in a full review process upfront.

How dev teams can become great at Git Pull Request Review

So, if you believe in the benefits of a full review process before merge, how can you successfully move in that direction?

Noam has 6 pieces of advice based on his three different git pull request experiences:

Most things do :) Noam goes on... "If leadership and everyone on the team care about putting the work into review, it will happen."

This has been said before so I asked Noam what that looks like In the context of git pull requests.

He said "We track it on a dashboard and we talk about it in most of our stand-ups and retros. I take pride in being at the top of the board for helping my teammates with the most reviews."

He wanted me to point out that in the dashboard below he was lower than normal because of the holidays in Israel during this particular iteration :)

Everyone on the LinearB dev team puts in the work to check and see which teammates need help with review. Noam says "I know it's probably going to come up in the stand-up so I try to check the dashboard before I come in so I'm ready".

Noam felt like most teams, even ones trying to do full peer review, just work on best efforts.

"On my last team a lot of times we wouldn't find out we missed a review on a substantial PR until after the fact when it was too late. A production issue should not be the way you find out."

I think we would all agree with that. So what's the alternative?

"This is one of the reasons I joined LinearB. Our product has automation to help make sure the review process happens and helps devs know when their teammates need help. I had never really seen that. I wish I had it before. "

"The Slack alerts are super cool. It makes it easy to jump in and help with a review without logging in to a separate screen."

1. "Work-from-home review is different" 

A lot of teams had to go through an adjustment period. According to Noam, the LinearB dev team was no different.

"Our Cycle Time really slowed down when we first moved to remote in March. Review time was taking loner and that was a big part of it. We made some adjustments to our process which included adding some placeholder meetings to the schedule to make sure we were carving out time for review. After a while, we were back on track. "

It is common for many git pull request reviews to start online in the git system then move offline to Slack or Zoom (or face to face when that was possible). Noam thinks this is a wasted opportunity...

"The best part of taking the time for thorough review is you build up this incredible knowledge system. When a new dev comes into the code base the place for them to learn what's going on is the review history."

So Noam advises teams to document everything in the system even after the review process moves to synchronous communication.

Call to action

Noam had two parting ideas to share with developers about git pull requests:

"If you're joining a new team, find out how they approach reviews. It will tell you a lot about their culture."

"Tell your team lead to check out LinearB. You'll like the automation around the PR process."