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:))
You must log in or register to comment.
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.
It’s surprising how Github is sorely lacking in git features and even encourages bad practices.
-
There is only a linear history view of commits without any child/parent relationships visualized. I have coworkers that tell me it’s annoying how I use merge commits because they just see a big list of small commits in the history, and they’d rather see one commit per PR. Well… I sympathize, but also, fuck that! I’m not squashing my carefully crafted commits because Github has a shitty UI. Use
git log --first-parentor something likelazygit. It’s sad that many open source projects have adopted a “squash and merge” mantra for PRs. All of the merge methods have a valid use case, but people are so beholden to the damned Github history view. -
PRs have no concept of what a patch is. If you rebase, the entire history disappears. Reviewers cannot see changes made to individual patches unless they are applied first in new commits and then manually squashed into old patches after the review is done. Phabricator does this better IMO; each patch has its own history. I’ve even heard devs at other companies tell me they never make PRs bigger than 100 LOC. That just seems like a huge waste of time to me, unless you have some special tools to support that bizarre workflow.
-
Merge queues only support a single merge method, one of merge commit, rebase, or squash. So we just don’t use this desirable feature because of this limitation.
Ooh yeah PR as patches, persistent despite rebases, would be nice.
Many git operations fundamentally have three SHAs as parameters (tree operations after all), and GitHub’s model simplifies it down to two.
-
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.
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.
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.
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.
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.
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.
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 --softthen the sha of the last commit you want included in reset.
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.