4 ms·
Yes! This is what I imagine in my head as a real code review style, not the stuff Github does. Glad to have a name for it. I'd add I'd also like my review syst
by deathanatos 2y ago
Yes! This is what I imagine in my head as a real code review style, not the stuff Github does. Glad to have a name for it.
I'd add I'd also like my review system to be able to kick patches "out" of the review once they're ready. E.g., the small bugfixes that you make while working on that bigger feature should hopefully be small, isolated patches, ones that are going to find consensus with a reviewer quite quickly. Once that happens … I want to just eject that patch from the entire series & cherry-pick it to main, and rebase the review on the new HEAD. (Or, differently, rebase the latest version of the patch series on main, reordered such that the agreed patch is fist, and then FF main onto that agreed upon patch.)
Essentially, narrow the scope of the review to "the part we're still talking about", but let bug fixes see merging as soon as they're ready.
The argument I'd have against this is "just make that a separate review/PR". But then you get into the hairiness of patchset A depends on patchset B, until B is merged, then it just depends on main.
- zeotroph 2y agoThat's exactly what Gerrit can do. When you push an x-b-c-d-e chain, these show up stacked in the UI, but you can easily cherry-pick b onto main (see that the CI passes, and the usual review), and rebase everything on top of that. If it is x, the bottom one, you can directly submit it and continue with the others.
- lolc 2y agoAll this rebasing sounds like constant pain to pull from Gerrit. Does it actually create new branches v2 v3 after a rebase? Or how do I switch my local checkout to the rebased branch from remote?
- zeotroph 2y agoThat's just a `git pull --rebase` away (or set pull to never merge but rebase), and all the non-merged commits are rebased onto the new upstream, and while doing the rebase the already-merged commit is dropped. The next git push origin HEAD:refs/for/main will then push the remaining commits.
- lolc 2y agoOk thanks, that sounds reasonable. Now I want to try Gerrit!
- aseipp 2y agoI keep saying this over and over but, Gerrit basically does that. :) You can see the relationships between any two patches on Gerrit, and more importantly, Gerrit shows you each patch individually. So you can see in a series A -> B -> C that yeah, B is small, let's go ahead and get that in. Part of this is that UX has some really smart ideas like the "Attention Set". The attention set is basically "Which people need to take the next step?" Like a turn-based game. So, if you just did a review, you're not in the attention set for that patch anymore -- the author is. That means Gerrit puts it down at the bottom of your queue in the UX. And what's at the top of the queue? Things where you are in the attention set! So it naturally groups things this way. I didn't get into all the other really annoying papercuts with GitHub's UX, but even the pull request listing is worse than the alternatives. How do you know what state anything is in? You don't, you have to go read the whole thing.
- zeotroph 2y agoAttention Set <3 - that alone is worth another post. Gerrit really is one of the best kept dev secrets, and if you never had the luck of seeing it in person at a company where you worked at, well... Makes me wonder what other git or dev-in-general blindspots I have.
- ankitdce 2y agoYeh, Attention Set is a game changer. We (Aviator) also took inspiration (ahem.. copied) attention set from Gerrit: https://docs.aviator.co/attentionset https://docs.aviator.co/attentionset
- deathanatos 2y agoI guess I missed it in your article, and I've never had the opportunity otherwise to use Gerrit. (Since Github is essentially so pervasive. I've only used that, Gitlab, and an internal review system that didn't do interdiff.)
- SlySherZ 2y agoDo you know how to create a local branch that tracks a gerrit commit? Usually I just do git commit --amend to update gerrit, but then I lose access to my patch's history (it's still on gerrit, but I want it locally).