Tracking Changes in a Code Review
Let's walk through the life of a code review together, and while we do so I'd like to show you two ways to view that lifecycle.
A junior dev, Bob, pulls the main branch (formerly known as the master branch) and creates his own branch, at commit beef...
:
Bob$ git pull
Bob$ git checkout -b bob/featureBob
Bob$ git rev-parse HEAD
beef...
He then makes some changes using my favorite editor:
Bob$ vim path/to/project/filename.py
And now he's ready to commit his changes:
Bob$ git add path/to/project/filename.py
While Bob takes his time, Alice does the same, but then commits her changes and pushes her branch, and makes a PR all at once:
Alice$ git pull
Alice$ git checkout -b alice/featureAlice
Alice$ git rev-parse HEAD
beef...
Alice$ vim path/to/project/filename.py
Alice$ git commit !$ -m "New feature A"
git commit path/to/project/filename.py -m "New feature A"
Alice$ git push --set-upstream origin alice/featureAlice
Alice$ gh pr create
At this point, their code is on a collision course. Whoever gets their change in first, is potentially going to cause merge conflicts in the other's PR. Since Alice seems to be the quicker engineer, we're going to assume she gets her change in soon:
Alice$ gh pr merge --rebase
Bob, however, doesn't merge or rebase the main branch yet. He's going to send his code out for review as-is to Carol:
Bob$ git commit -m "New feature B"
Bob$ git push --set-upstream origin bob/featureBob
Bob$ gh pr create --reviewer carol-on-github
Carol sees a new, seemingly-unreviewed PR:
When Carol sees Bob's PR, it will include a single commit based on what HEAD was, beef...
and that's what she reviews as well. But then, when Carol leaves her review, she notices the warning that the PR can't merge cleanly, so she leaves a comment for Bob to update:
Bob$ git fetch origin main
Bob$ git rebase origin/main
...snip merge conflict resolution...
Bob$ git push --force
When Carol views this PR again, she could use either code review tool available to her. This is where we come to a fork in the road.
What Carol would see in GitHub: Commits
Let's take the well-traveled path now, and see what GitHub shows us: commits.
Carol sees a familiar PR, but it's a ghost town. She jumps to the "Files Changed" tab and sees code she's seen before, she could swear she's seen this exact code, and left comments on it, too. However, she sees 0 comments.
She clicks on the "Changes from all commits" dropdown, but no dice:
If she goes back to the main PR view, aka the "Conversation" tab, she sees her comments strewn across the now long page:
Strange, though, she sees comments, but if she flips back to the "FIles changed" tab, there's no comments still. What happened here?
Ghost town PR's
Carol now has to review this PR from scratch, again. And if she leaves any more comments for Bob to address, there's a chance she has to review it from scratch for a third time, or a fourth time, and so on.
Taking this path ends up here when a PR gets rebased, or when files change significantly, or even when they just get renamed between reviews. There's no way to review what Bob did, only what Bob wants to merge, so Carol can easily lose context on what she reviewed and what happened between reviews.
What Carol would see in Reviewable: Revisions
Now let's take the path where Carol clicks this little button instead:
This takes Carol to a view of the PR that immediately shows her the status of the PR, and what she needs to do, if anything:
And if she scrolls down, she not only sees the diff she's seen before, but she sees the comments she left, too!
They're now on the left, showing that they were left on a previous version of the PR, but they're not gone! In fact, she can see which line her comments were on, and if Bob made any relevant changes. Here, if we squint at the screenshot, we can see that the comment doesn't have any added, removed, or changed lines near it, so Bob didn't change anything here.
In fact, we know that Alice made changes to those lines and Bob's rebase picked them up. We can see that in the PR if we select the right revisions to be compared:
That then shows that the r1 to r2 changes affected the code in this way:
And since it's not marked as a "base-only change", that means that Bob did some work to handle a merge conflict, and it wasn't a simple merge or rebase operation. Therefore, Carol should still review this change, but she doesn't have to review the entire file from scratch!
Reviewing what the author did, not only what they want to push
This path, which involves using Reviewable, allowed Carol to review what Bob did between reviews, instead of having to review the entire PR again.
Imagine that this was a larger PR, with a more complicated merge conflict, would you want to review the whole PR again to check if Bob rebased correctly? Would you notice a tiny whitespace mistake, or if Bob flipped two if-statements?
As a fellow senior engineer, I can confidently tell you that I've missed those in the past. Good engineers, that I trusted and still trust, made honest mistakes, and my tools didn't help me catch them. If I was lucky, they were caught by the unit-tests, or at lesat by the QA team. Some were found in production.
Conclusion
Reviewing code from scratch is not only annoying and not cost-effective, but it's also error-prone. Reviewers aren't perfect, and neither are authors, and every time you have a reviewer doing a manual review of the same code again, they're going to miss things.
for serious code reviews