Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Some of us like "interdiff" code review

gist.github.com

Post reply on HN
Print view View original

31–40 of 224 posts

Re: Some of us like "interdiff" code review

#31

Post by aseipp »

zeotroph wrote: post #29
aseipp wrote: post #15

I am the person who wrote this. AMA EDIT: Also, I'm not sure if this is against the rules, but I also need a new job as of recently. I like working on dev tools and other hard problems. If you liked reading this, want me to make your dev team more productive, or just want to experience and enjoy my excellent (and occasionally eclectic ) taste, the email is in my profile.

From my interaction with the free part of GitHub, "diff soup" describes it very well. Does the paid version do anything better? What about GitLab, can this get near Gerrit? And then there are the external services which try to make GitHub less painful (and quite pricey, especially compared to a selfhosted Gerrit), by providing stacked diff support, did you look at these?

No, paying for GH doesn't make the code review experience any better. It's identical across public/cloud/enterprise GH.

I do not know if GitLab does anything different; I've never used it in anger. I'd bet $10 the answer is "no, it's basically just the same as GitHub", though.

If you want a service that adds stacking on top of GitHub, my conclusion after some research is that https://graphite.dev/ is the best option. We did consider it, but it couldn't work in my last job for "reasons" (see my other replies in this thread) but if you've got a shop of Git users and just want to throw money at the problem, I think it's the best choice.

GerritHub is also a possibility, they employ many of the Gerrit devs and know what they're doing, but holy shit the corporate options are expensive out of the gate. It's like $20k/yr minimum regardless of size or number of users.

Honestly, Graphite is cheap as hell considering how much more productive your engineers can be with a good review tool. Gerrit was basically night and day for us. It's not "oh, it pays for itself really quickly in a few days!" You'd probably pay off the monthly cost in less than an hour of actual code review. And you don't even have to opt most of your engineers in; you can trial it where 90% of them use GH and only a subset use graphite and pay.

Re: Some of us like "interdiff" code review

#32

Post by taspeotis »

aseipp wrote: post #16
ajkjk wrote: post #8

100% agree that this is ideal, the way Github does it is completely godawful and it's a tragedy that so many people have it normalized for them. We did this with Phabricator, although it was a somewhat-manual process, helped along by having some command line macros for updating all the reviews at once. But better still would be an explicit UI for it.

I am the author and used the phrase "Code review is a pretty good idea, in general" in the opening very specifically, because it used be one of the selling points listed on the Phabricator homepage. :) I miss it.

> Grab ahold of tasks, literally. Place them in confusing, new orders. Make a column just for interns! Ignore the backlog forever.

I swear the Herald selling points also had keeping tabs on the pesky interns, or something to that effect.

Re: Some of us like "interdiff" code review

#33

Post by aseipp »

rosmax_1337 wrote: post #20
aseipp wrote: post #15

I am the person who wrote this. AMA EDIT: Also, I'm not sure if this is against the rules, but I also need a new job as of recently. I like working on dev tools and other hard problems. If you liked reading this, want me to make your dev team more productive, or just want to experience and enjoy my excellent (and occasionally eclectic ) taste, the email is in my profile.

What is the meaning of life?

I know this is in jest, but I'll just take the opportunity to respond by posting my favorite poem. The relationship between it and your question -- well, that's for you to decide.

    The birds have vanished down the sky.
    Now the last cloud drains away.
    We sit together, the mountain and me,
    until only the mountain remains.
-- Zazen on Ching-t’ing Mountain

Re: Some of us like "interdiff" code review

#34

Post by chipx86 »

aseipp wrote: post #26
chipx86 wrote: post #19

We first built interdiffs in Review Board [ https://www.reviewboard.org ] way back in 2006 (in fact I think I may have coined the term, or arrived at it independently). It's still my favorite part of the product and my process when doing code reviews. And it's one of the things we hear the most nostalgia for when people move to something like GitHub. I've never felt that fix-it commits are really a proper alternative…

That's sick as hell, friend. Actually, I have a second part to this article discussing some of the history and politics of what brought me to these tools. In about 2013, I migrated the Glasgow Haskell Compiler from "read .patch files on bug reporter" that I joked about, to using Phabricator. For a couple reasons, but at the time one of them was not stacked diffs. It was because GitHub was so bad for review it didn't…

I love that! Thanks for sharing that with me :) It made my day. Looking forward to the second part! If you're on really any of the current social platforms, I'd love to connect. I'm chipx86 everywhere.

I still develop Review Board full-time with a small team :) The development world has changed a lot, and much of the world has converged on GitHub (it's a hard market to be in right now), so we still look for opportunities to build review capabilities that target problems people have that aren't being addressed elsewhere.

Just as an example, we launched PDF Review and diffing a while back, which we see companies use for things like industrial designs and schematics. It's neat, we actually diff two rendered PDFs without just converting them into text files first, like you usually see. Following that up with full Office Document Review soon.

Dark mode finally shipped earlier this year (I should write about that endeavor sometime). And there's a couple of super-neat code review capabilities we've come up with that nobody's doing, which I'm keeping under wraps for next year. I think they're going to be pretty awesome.

Re: Some of us like "interdiff" code review

#35

Post by deathanatos »

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.

Re: Some of us like "interdiff" code review

#36

Post by zeotroph »

aseipp wrote: post #31
zeotroph wrote: post #29

Earlier quoted context omitted.

From my interaction with the free part of GitHub, "diff soup" describes it very well. Does the paid version do anything better? What about GitLab, can this get near Gerrit? And then there are the external services which try to make GitHub less painful (and quite pricey, especially compared to a selfhosted Gerrit), by providing stacked diff support, did you look at these?

No, paying for GH doesn't make the code review experience any better. It's identical across public/cloud/enterprise GH. I do not know if GitLab does anything different; I've never used it in anger. I'd bet $10 the answer is "no, it's basically just the same as GitHub", though. If you want a service that adds stacking on top of GitHub, my conclusion after some research is that https://graphite.dev/ is the best option.…

Right, for Graphite $20/dev/month is nothing (I wonder if Enterprise is less or more more than that...), considering an ounce of review (prevention) is worth a pound of bugfixes (cure).

And when you can not get corporate to switch away from GH, then that is it. In hindsight an obvious way to (almost) print money, congratulations, but also a sad state of affairs.

But I imagine the $20k/yr is something you can easily spend on a 1/5 of a dev doing Gerrit maintenance.

Re: Some of us like "interdiff" code review

#37

Post by aseipp »

taspeotis wrote: post #32
aseipp wrote: post #16

Earlier quoted context omitted.

I am the author and used the phrase "Code review is a pretty good idea, in general" in the opening very specifically, because it used be one of the selling points listed on the Phabricator homepage. :) I miss it.

> Grab ahold of tasks, literally. Place them in confusing, new orders. Make a column just for interns! Ignore the backlog forever. I swear the Herald selling points also had keeping tabs on the pesky interns, or something to that effect.

One of my favorites was something like:

> Phabricator is well received, and has raging reviews from users inside Facebook, such as "Mandatory" and "OK"

Also, Evan Priestley still uses the https://secure.phabricator.com instance to occasionally post life updates, so I check in on it.

Last I checked he was learning about PCB design and printing. I think he wanted to learn how to actually physically manufacture his own boards (e.g. etching your own conductive copper layers) and then he posted some update like "I'm not sure I'll go down this route, because it turns out you can send your design to PCBWay, and it will come back within 5 days, because there is a magical PCB Faucet somewhere in Shenzen apparently."

What a guy.

Re: Some of us like "interdiff" code review

#38

Post by zeotroph »

deathanatos wrote: post #35

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. O…

That'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.

Re: Some of us like "interdiff" code review

#39

Post by danparsonson »

gloryjulio wrote: post #27
PoignardAzur wrote: post #25

Earlier quoted context omitted.

My experience is that systemically squashing PRs enables a "fire and forget" style where you can add a bunch of small commits to your PR to address reviews and CI failures without worrying about making them fit a narrative of "these are the commits my PR is made of". On a more concrete level, squashing PRs means every single commit is guaranteed to pass CI (assuming you also use merge queues) which is helpful when bi…

With stacked commits, every commit is already passing CI though. To us the mental model is minimum. All you need to do is to make sure each commit pass CI. You can ship any number of stacked commits together ---------------------------------------------------------------------------------------------------- Not sure why I can't reply in a technical discussion. I have to edit to answer your question @danparsonson > if…

Perhaps I misunderstand you but what if I'm working on a long series of changes across multiple days, and halfway through it the code doesn't build yet? The code won't pass CI because I'm not finished, but I want to commit my progress so I don't lose it if something goes wrong, and I can roll back if make mistakes.

Re: Some of us like "interdiff" code review

#40

Post by aseipp »

deathanatos wrote: post #35

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. O…

I 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.

Post reply on HN