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

21–30 of 224 posts

Re: Some of us like "interdiff" code review

#21

Post by aseipp »

fosterfriends wrote: post #11

I agree with the argument laid out here. Series of small diffs with versions is a fantastic clean model. When creating Graphite on top of GitHub, we chose to only support rebase model (despite the chaos that creates in GitHub timeline events). We also added “versions” support, which wasn’t too hard because GitHub holds on to old commits even if you force push over them. A lot of what we try to build is the exact idea…

We strongly considered Graphite as an alternative to Gerrit at my job that I mentioned at the start of this post (which I am no longer at, actually) because it does look like an absolutely excellent product, I will admit. You should all be proud of a smart design and smart set of tools.

But there's a really really really really really really big problem. Me and the other main engineer on our team used a custom frontend to Git called Jujutsu[1] for all development. Jujutsu is about 1000x better than Git. So that's nice. (I'm also one of the developers, so I'm not going to abandon it anytime soon.)

But gt, the graphite client, is not open source. I have no idea how to make them work together. I have no idea how to extend Jujutsu to handle Graphite stacks, because I don't even think there's an API to handle any of this.

I even wrote a Gerrit integration for Jujutsu because JJ works so well at stacking, and Gerrit + Jujutsu is absolutely a force to be reckoned with IMO, even if the UX isn't as nice as Graphite's. (I'm happy to show the people at Graphite why that is, too, if anyone has time or is interested, but I suspect you could all grasp Jujutsu quite easily :)

Please! Make gt open source and make it possible for third parties to make and update stacks. This isn't just useful for jj but all kinds of automation that wants to contribute patches -- imagine tools like Google's internal "Code Review ML models" for Critique that might recommend you rename a variable based on context. They will suggest the fix for you or even apply it! You can get around some of those workflows with "Incorporate suggested edits" which is great on Gerrit (and Graphite?), but not all of them.

[1] https://github.com/martinvonz/jj

Re: Some of us like "interdiff" code review

#22

Post by aseipp »

enasterosophes wrote: post #7

Nice, this taught me about `git range-diff` which wasn't on my radar before. Is the conclusion likely to be that the author thinks Gerrit is good, or is there some nuance I didn't pick up? I've used Gerrit before and in hindsight I much prefer it to other ways of doing code review.

Yes, Gerrit is fucking great. If you actually want to do code review and not just rubber stamp shit on GitHub because your eyes are going to bleed after reading the same thing for the 15th time, just use Gerrit.

The thing is, I just never got around to finishing this article because what's there right now is "good enough" to get the ideas across.

Re: Some of us like "interdiff" code review

#24

Post by john_the_writer »

lozenge wrote: post #12
ams92 wrote: post #4

Most of the complaints here could be solved by having smaller pull requests and then squashing commits when it’s time to merge.

Not really. The idea is to split work into separate stages which are reviewed separately, but as a whole. In the example: "small refactor 25LOC -> new API 500LOC -> migrate API users 50LOC" Making a PR of the small refactor will probably garner comments about "why is this necessary". Opening two PRs at the same time is clutter as GitHub presents them as separate. As well, sometimes CI won't pass on one of the stages…

the comments about "why is this necessary" can be handled with a decent PR template, and a comment.

What I tend to do is make the changes locally with different commits and then cherry pick the refactor into a PR branch and wait for that to be accepted. Then I rebase the FULL branch with "master" after the merge and create the PR.

Re: Some of us like "interdiff" code review

#25

Post by PoignardAzur »

gloryjulio wrote: post #9
ams92 wrote: post #4

Most of the complaints here could be solved by having smaller pull requests and then squashing commits when it’s time to merge.

We use stacked commits + rebase only in our company. The commit history is linear and it's very easy to revert changes. I don't see any advantage of using merging instead of rebase I am not sure why we need to squash commits. We encourage the opposite where you should commit small and often. So if we need to revert any commit, it's less painful to do so.

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

Re: Some of us like "interdiff" code review

#26

Post by aseipp »

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 even have side-by-side diffs! It was a total non-starter for me for those reasons among others.

But that actually wasn't the first time I migrated a team to a code review tool. My first job in 2009 was a very small tight knit team of engineers in a single room in Houston, and I remember thinking it would be really good to get reviews of my code from other people, and to read the things they wrote so I could better understand the codebase. So, the first thing I did in the first few months was pester my manager to set up... ReviewBoard! And we all really liked it.

So I guess this is a way of saying thanks for RB! I still think of it fondly from time to time. And because of it, Code Review has always just been a huge part of my career, almost since day one (and I could still do more of it.)

Re: Some of us like "interdiff" code review

#27

Post by gloryjulio »

PoignardAzur wrote: post #25
gloryjulio wrote: post #9

Earlier quoted context omitted.

We use stacked commits + rebase only in our company. The commit history is linear and it's very easy to revert changes. I don't see any advantage of using merging instead of rebase I am not sure why we need to squash commits. We encourage the opposite where you should commit small and often. So if we need to revert any commit, it's less painful to do so.

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 I'm working on a long series of changes across multiple days, and halfway through it the code doesn't build yet?

That's why you break them down into small commits. The early you push it to CI, the earlier you will know whether each commit builds. For example, push commit 1 2 3 to the CI when they are ready. When the CI is running, you are working on commit 4 5 6

> The code won't pass CI because I'm not finished, but I want to commit my progress

If your commit 1,2,3 are ready, just ship them. It doesn't stop you have a few commits in reviews and a few WIP commits. There is no down time

Re: Some of us like "interdiff" code review

#28

Post by john_the_writer »

gloryjulio wrote: post #17
majormajor wrote: post #13

Earlier quoted context omitted.

Without squashing it's hard for me to commit as small and often as I would like. Some things I want out of the final series of commits: 1) everything builds. If I need to revert something or roll back a commit, the resulting point of the codebase is valid and functional and has all passing tests. 2) features are logically grouped and consistent - kinda similar to the first, but it's not just that I want the build to…

When we publish a stack of commits, our ci ensures that every commit is build and tested individually. There is no consistency issue Squash and merge actually makes the above goal harder. With rebase + small commits, all we need to make sure is that every commit pass all the build signals and tests during ci

This only works if your commit in a green state. Sometimes we have to change when things are still "Yellow"..

I tend to add all my tests in one go and commit the RED. "tests are written" Then as I pass each test, I commit that.

This pattern works really well for me because if I mess up, then rolling back to the last yellow is easy. I can also WIP commit if I have to fix an urgent bug, and then get back to the WIP later.

Re: Some of us like "interdiff" code review

#29

Post by zeotroph »

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?

Re: Some of us like "interdiff" code review

#30

Post by gloryjulio »

john_the_writer wrote: post #28
gloryjulio wrote: post #17

Earlier quoted context omitted.

When we publish a stack of commits, our ci ensures that every commit is build and tested individually. There is no consistency issue Squash and merge actually makes the above goal harder. With rebase + small commits, all we need to make sure is that every commit pass all the build signals and tests during ci

This only works if your commit in a green state. Sometimes we have to change when things are still "Yellow".. I tend to add all my tests in one go and commit the RED. "tests are written" Then as I pass each test, I commit that. This pattern works really well for me because if I mess up, then rolling back to the last yellow is easy. I can also WIP commit if I have to fix an urgent bug, and then get back to the WIP lat…

Not sure what you mean... When we ship a stack of commits, every commit has to pass everything in CI. You are not suppose to ship a commit that's not passing the ci bar. There is a escape hatch that you can bypass but it's rarely used.

You can make changes before you ship however you wanted as long as they pass ci. If you already shipped the code and want to make changes later, that means making new commit or reverting a bad commit. It's simple as that

Post reply on HN