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:))
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.
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.
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.
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.
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
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.
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’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 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.
Right, for junior devs or trivial changes, that’s fine. My take is if I’m going to make someone take the time to review my work, I take the time to make sure that it’s cleaned up and would be something I would merge if I were reviewing it. Most of this comes from working on some larger Open Source projects which still require patches be submitted via email which I know is a real “back in my day” moment, but it did instil good practices which I try to carry on.
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.
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.
(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!
You are not logged in. However you can subscribe from another Fediverse account, for example Lemmy or Mastodon. To do this, paste the following into the search field of your instance: !programming@programming.dev
Welcome to the main community in programming.dev! Feel free to post anything relating to programming here!
Cross posting is strongly encouraged in the instance. If you feel your post or another person’s post makes sense in another community cross post into it.
Hope you enjoy the instance!
Rules
Rules
Follow the programming.dev instance rules
Keep content related to programming in some way
If you’re posting long videos try to add in some form of tldr for those who don’t want to watch videos
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.
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.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.
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.
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
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.
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 just comment “Looks good!” and approve.
Same here, unless it’s a small PR then I like to nitpick 👍
As a manager I wait until another engineer approves then comment “lgtm” and approve so the PR hits its two review minimum.
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 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:
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.
Right, for junior devs or trivial changes, that’s fine. My take is if I’m going to make someone take the time to review my work, I take the time to make sure that it’s cleaned up and would be something I would merge if I were reviewing it. Most of this comes from working on some larger Open Source projects which still require patches be submitted via email which I know is a real “back in my day” moment, but it did instil good practices which I try to carry on.
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.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.
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!
Squash merge into the main branch. It’s the only way to fly. (just my 2c!)