10mo ago

How common is it to code review like this?

For non-trivial reviews, when there are files with several changes, I tend to do the following in Git:

Create local branch for the pr to review
Squash if necessary to get everything in the one commit
Soft reset, so all the changes are modifications in the working tree
Go thru the modificiations "in situ" so to speak, so I get the entire context, with changes marked in the IDE, instead of just a few lines on either side.

Just curious if this is "a bit weird", or something others do as well?

(ed: as others mentioned, a squash-merge and reset, or reset back without squashing, is the same, so step 2 isn't necessary:))

40 comments

Make branch for the ticket
Make periodic commits to branch
Open PR from branch to main
(optional) if the changes are big, I have a meeting with devs to discuss it. If I can't easily explain to the junior devs what I did and why, it means I did something wrong.
As a senior dev, I've found "can the junior devs grok wtf I did/made" to be an excellent "did I overengineer?" Litmus test.
A good implementation should be not too hard to explain to the juniors, and they should be able to "get it" in a single short 20-30 minute meeting at most.
If they are curious/interested and ask questions, that's a good sign I made something useful and worthwhile.
If I get a lot of "I'm not sure I get it" and blank stares, I probably have overcomplicated the solution.
If that "ooooh, okay!" Comes quickly, then we are good!
- If you need a whole meeting to explain what's going on in your PR then it's already too complicated, IMO. That explanation should be done as a combination of PR description, commit messages, documentation, comments, and the code itself.
  Everyone will forget a meeting. The stuff I described will pay dividends for future maintainers.
  The meetings should happen at design and planning phase.
  
  Do you not do demo meetings after introducing entirely new features?
  Sometimes a PR can be quite large as it involves an entirely brand new feature that simply didn't exist before.
  And if it's an internal tool/service for fellow devs to use to make their lives easier, yes, it likely deserves a meeting so the devs can have a chance to QnA about it. Usually 5-10 minutes going over the who/what/why/where/how, then 20 mins or whatever of any needed QnA if devs are curious for more info about specifics, like performance or extensibility or etc.
  If you create a new tool like that abd then just hand it off with all the devs have to go on being "here's the manual, figure it out" you know what happens?
  Almost no one reads it, and pretty much no one uses it, because parsing a giant manual of info is difficult co.pared to seeing a live demo

If you were reviewing a "non-trivial" PR from me, I'd recommend not squashing because I would've broken it up into readable atomic commits.
- This should be much more wide-spread. The hardest part of programming is reading someone else's code.
  More people should learn to do git rebase -i, it's a simple way to re-organise your commits to make sure that they tell a story to someone going through the PR commit by commit. It only takes a minute and can save your colleagues so much time and increase the quality of the review process.
  
  Unfortunately it's uncommon now that GitHub's PR workflow dominates, so people think in terms of (often squashed) PRs and talk about "stacking PRs". At least GitHub supports viewing PRs commit by commit.
  If PRs are just how it's going to be, I wish GitHub could auto cut stacked PRs from a linear branch of commits.
  
  Or use a tool like StackedGit which makes the atomic commit workflow incredibly simple. Build atomic commits as you go instead of after you've written all of the code.

I made a branch, make commits, and then make a PR. I don’t care about the number of commits because sometimes a reviewer might be able to make more sense of a PR if they view each commit instead of all the changes at once.
For us we just make sure that the branch builds and passes tests before merging it in, and just do a general look over to make sure everything looks correct, follows best practices, etc. if the UI was changed I usually add screenshots of before/after or a screen recording of me using the feature. Sometimes these can really help a reviewer understand what all the changes mean.
- In my experience, I prefer to review or contribute commits which are logical changes that are compartmentalized enough that if needed, they could be reverted without impacting something completely differently. This doesn't mean 1 commit is always the right number of commits in a PR.
  For example, if you have a feature addition which requires you to update the version of a dependency in the project, and this dependency update breaks existing code, I would have two commits, being:
  Update dependency and fix issues because of the upgrade
  Add new feature using new dependency
  When stepping through the commits in the PR or looking at a git blame, it's clear which changes were needed because of the new dependency, and which were feature additions.
  Obviously this isn't a one size fits all, but if someone submitted a PR with 12 commits of trial and error, and the overall changes are like +2 lines -3 lines, I'd ask them to clean that up before it gets merged.
  
  You can squash merge so it goes into the main branch as one commit, that's usually how I do it.

It's possible to do a diff across all the commits in the PR (rather, a diff between the last commit before, and the final commit of), so no, I do not squash them all for review. Otherwise, yes, I will definitely clone and review the code locally for large PRs.

Squashing seems like you'd potentially lose out on info and have a harder time isolating the changes you're looking through. I guess it depends on how much has been changed and whether some of the commits along the branch were more important than others.
I also don't think the reset is necessary, you should be able to diff the branch head against whatever you want.
- Yeah I had the same thoughts, had no idea people even bothered to do that
- Sometimes the info lost is just a typo or a revert. I'd say heavily depends on the workflow of the people involved. Some like long history, some like rebasing, others, something in between. How you review those approaches changes a lot
  
  Sure, that's fine. I use interactive rebase for "cleaning" a lot. I'm just saying it doesn't make a difference for diffing (as you can diff any commit against any other) and doing it as a matter of routine sounds like it could skip potentially useful history.
  I mostly rebase but if a branch has things happen in a sequence that matters, I would merge it instead, for example.

I just comment "Looks good!" and approve.
- As a manager I wait until another engineer approves then comment “lgtm” and approve so the PR hits its two review minimum.
- Same here, unless it's a small PR then I like to nitpick 👍

Different things will work for different organizations. More important than this is whether everyone is on the same page with their workflow in checking PRs. One commit prs might be easy for some workflows and bad for others

I hadn't yet tried squashing + reset, that seems like a smart idea, but yeah, I don't particularly like the usual PR review UIs.
I do the merge via CLI anyways, so might as well check out the code for the review and then view it in my IDE + be able to run it and such.
- If you're using the CLI and cleaning up a branch for a PR, the interactive rebase is a godsend. Just run git rebase -i origin/main (or whatever your target branch is) and you can reorder/squash/reword commits.

It depends on the platform you are using. But, for platforms like github and gitlab there are extensions for popular IDEs and editors available that allow you to review all changes in the editor itself.
This at the very least allows you to simply do the diffing in your own editor without having to squash or anything like that.

This does strike me as odd, your commits should be cleaned up if they are a mess of "reverted X", "fix typo", "saved days work", etc. on the other hand, you don't usually have to explain your modifications if you didn't squash your commits.

There are IDE extensions that show the diff of the entire PR locally without having to squash anything. So yes, it's weird to reinvent a square wheel.

Not too weird. Personally I use a tool called reviewable.io which essentially does the same thing but bundled into a nice UI. It depends on the branch though, if there are a lot of changes and it's a senior developer then the changes are usually broken up into meaningful commits but usually the commits are just a ball of mud.

Squash merge into the main branch. It's the only way to fly. (just my 2c!)

I'll usually do steps 1 and 2 for a reasonably complex review. I'll reference the diff from the website (for work, this is Azure DevOps, for personal, GitHub or similar) while I inspect and run the modified code locally.

I usually find web gui good enough to get context of code. Doing all that and then needing to go back and forth from IDE to PR to leave comments sounds like it'd take a bunch more time.
Why squash all the commits if you're just going to reset? Unless I'm crazy you can reset across multiple commits.
- Yeah git reset --soft then the sha of the last commit you want included in reset.

40 comments