It’s a vast subject, but one thing is certain: reviewing other people’s code is hard. Because good mentoring require technical and non-technical skills (such as patience).
I would like to dive directly into a specific detail of code reviews. It’s an iterative process: author submits code for review, reviewer make suggestions, author amends or pushes more code, reviewer make different or more suggestions, and so on.
In Git, « more code » takes the form or one or more commits appended to the Pull Request (or Merge Request if you use Gitlab, for simplicity I’ll just use « Pull Request » in this piece). And « amended code » means overwriting existing commits and force-pushing, which makes the old commits disappear.
As a reviewer, it can be very annoying because what I first look for in an update is whether my suggestions have been implemented or not, and how. That’s why authors are sometimes encouraged to push new commits in their Pull Requests and never overwrite existing ones. It makes the reviewer’s job way easier, because the UI can just show the new commit and they’ll know what’s changed.
But having this policy has drawbacks. When the Pull Request is merged (by fast-forward or not) in can leave awkward commits in the history, like « implement suggestions », « fix according to review », « review fixes again », etc… And merging the PR by squashing it isn’t always relevant, sometimes I do want to have several commits, because they address different parts of the problem.
How can we solve this? Well, it would be nice if I could see the difference between the current state and the last time I reviewed regardless of whether the author has amended their commit or not. And for that, I need to have a local copy of that commit. Fortunately that’s one of the things that git is very good at: you just make a local branch that tracks the PR’s branch, and when the code is changed you make another one. And then you can diff between those branches and see what changed.
OK, sounds simple enough. I have an itch, let me scratch it.
I present you git-pr-branch! A « small » utility that will create branches from Pull Requests, and do a few things with them. You’ll be able to automatically create the PR-based branches that I just explained about. You’ll also be able to display a nice listing of the branches, their associated PR, the PR status (open or closed), and the PR URL to clickety click. And since this can end up in quite a lot of branches, there’s also a sub-command to clean all that up, and delete branches whose PR is closed.
Ironically, it’s hosted on Gitlab but at the moment it only works on Github and Pagure. I’ll add Gitlab support if I end up working more with Gitlab (something tells me it’s likely to happen in the near future), but you can also send me a patch if you want it sooner.
While writing this side project, I discovered the fantastic python library attrs. It’s really awesome, I encourage you to try it out. (as always, my side projects are a good opportunity to try out new libraries or frameworks that I discover 😉 )
The python packages are on PyPI, and for the lazy Fedora and Mageia users out there I’ve made a COPR repository that you can enable on Fedora 32 and Mageia 7. Once installed, just run
git pr-branch) to discover the available commands.
Feel free to tell me what you think of the tool. Do you like the idea? Are you going to use it in your reviewer workflow? Did I bother writing code again when there is an obvious and better tool to do it? Let me know! 🙂
[EDIT] I’ve made a COPR repo, added the link.
[EDIT2] It now works with Pagure too, added the reference.
2 réflexions sur « Reviews are hard »
[Code review is really simple with the right tool]
I don’t know about pagure state but I think you have the same issue reviewing code under Pagure.
I resolved the exact same issue in 2014 when diving in Gerrit.
Gerrit is the only decent (git based) software enabling pure code review.
It is designed as a Git forge + code review. To explain its advantages:
– every « modification » is assigned « Change-ID »
– a commit might be cherry-picked/merged to an other branch. You know have the same « Change-ID » accross several commit, each having their own git revision
– The review is against a match between project name + branch + change-id
Therefore, you review code for a match between project name + branch + change-id. In the same way, you update your « change » for a match between project name + branch + change-id. Now, the tool is able to show you diff between reviewed code only (it might even hide pure rebase add code modification).
You don’t care anymore about ‘Merge’ or ‘Pull’ request. You only care about ‘code change’, in the commit way. Of course you might also merge branches but you’ll then review the merge commit and have diff between previous reviews.
It’s really amazing.
Of course that won’t solve the problem if your code is hosted somewhere else, unless you move to gerrithub.io
http://gerrithub.io/ (checkout the blog section)
Even Zuul from OpenStack is now integrated to Gerrit in order to have really advanced CI/CD workflow
Yeah I used to use Gerrit something like 8 or 9 years ago. And even at that time it was really good.