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

51–60 of 224 posts

Re: Some of us like "interdiff" code review

#51

Post by jacobegold »

aseipp wrote: post #21
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 fronte…

We (Graphite) love Jujutsu – comes up in conversation all the time here.

A prior version of the CLI is open source, the core data model (using git refs to store some extra data about what a branch's parent is) is still the same. https://github.com/withgraphite/graphite-cli

We've talked about supporting other clients, but don't currently have the bandwidth to build something like that – definitely something I am personally passionate about making sure happens at some point.

Re: Some of us like "interdiff" code review

#53

Post by codeapprove »

Love the blog post, it's great to see people actually thinking about how code review should work!

I've used four different code review systems extensively, all with different strengths and weaknesses: Critique (Google internal), Gerrit (at Google, but same as external), GitHub (duh), and CodeApprove (the one I built).

Critique was far and away the best, but it only works because it's perfectly fit to Google's monorepo and the custom VCS they've built as well as all of their custom lint/test tooling. I designed CodeApprove to bring as much of that as I could to GitHub, but it will never really be close.

Gerrit was the second best in terms of the reviewer experience ... but as an author I always hated it. It just seemed to be so author-hostile. There were more wrong ways to do something than right ways. And the UI is not exactly beautiful.

GitHub is extremely author friendly, it works how we think. You write code, you get feedback, you write more code, etc. If you squash and merge at the end of a PR you don't have the history problems the author mentioned. It's not very reviewer or team friendly though. Incremental diffs are not highlighted. Diffs and conversation are in different tabs. Force pushes and rebases destroy history. Comments are lost as "outdated". You can't comment on files outside the diff window. Large files are hidden by default, etc etc. They clearly don't care about this too much and maybe they know something I don't.

In the end, the thing I find most frustrating is how many teams just accept whatever code review tool is built in to their VCS platform. That would be like using whatever IDE shipped with your laptop! There are so many better options out there today. My favorites (besides CodeApprove) are GitContext, Reviewable, and Graphite but I can name half a dozen other excellent choices. Don't accept the defaults!

Re: Some of us like "interdiff" code review

#54

Post by mstachowiak »

It's always exciting to see new approaches to code reviews - GitHub has its strengths, but it’s far from perfect.

For the scenario you’ve outlined, have you thought about splitting the 3 patches into separate, dependent pull requests? While GitHub doesn’t natively support this, the right code review tool (shameless plug - I’m part of a team building one called GitContext) should allow you to keep pull requests small while maintaining dependencies between them. For example, patch 3 can depend on patch 2, which in turn depends on patch 1. The dependency tracking between them - provided by the code review tool - can ensure everything is released in unison if that's required.

Each patch can then be reviewed on its own, making feedback more targeted and easier to respond to. You can even squash commits within a pull request, ensuring a clean commit history with messages that accurately reflect the individual changes. Better still, with the right tool, you can use AI to summarize your pull request and review, streamlining the creation of accurate commit messages without all the manual effort.

A good code review tool also won’t get bogged down by git operations like rebases, merges, or force pushes. Reviewers should always see only the changes since their last review, no matter how many crazy git operations happen behind the scenes. That way, you avoid having to re-review large diffs and can focus on what’s new. The review history stays clean, separate from the commit history.

I'd be curious if this approach to splitting up pull requests and tracking their inter-dependencies would address your needs?

Re: Some of us like "interdiff" code review

#55

Post by EPWN3D »

Is it just me or is the author just arguing for fixup commits and squashing them, something GitHub and Stash/Bitbucket handle just fine? I don't see how the idea of "publish a new version of the three commits" is new with or exclusive to Gerritt.

Re: Some of us like "interdiff" code review

#56

Post by fHr »

mstachowiak wrote: post #54

It's always exciting to see new approaches to code reviews - GitHub has its strengths, but it’s far from perfect. For the scenario you’ve outlined, have you thought about splitting the 3 patches into separate, dependent pull requests? While GitHub doesn’t natively support this, the right code review tool (shameless plug - I’m part of a team building one called GitContext) should allow you to keep pull requests small…

Why not just do good old mergetrains with pullrequest A points to branch B amd then B points to master, merge B into master and thereafter point A back to master or am I missing the point?

Re: Some of us like "interdiff" code review

#57

Post by fHr »

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

Never going to understand those finance teams who think like 20k/yr for any enterprise deal is a good deal to onboard more customers and increase reach.

Re: Some of us like "interdiff" code review

#59

Post by wodenokoto »

When doing code reviews, I think it is annoying that every time I comment on a line, PR author gets a notification.

This is not a simultaneous, real-time thing.

I'm in the middle of doing my review, and my comments are not ready to be read. Maybe I'll change my mind on my comment on line 8 when I reach line 80.

Re: Some of us like "interdiff" code review

#60

Post by globular-toast »

GitLab supports this. Every time someone pushes or force pushes it tags that as a version which you can diff. If your developers know how to generate new commits then you can do it right away with GitLab.

The problem is generating the new commits. Developers just aren't very good at doing this. They can modify a single commit just fine, but modify a commit that isn't the latest commit involves a rebase.

Magit has the "instant fixup" option which is basically like amending an arbitrary commit instead of just the latest. What is actually doing is doing a commit with `--fixup` then `rebase --autosquash`. This technique can be used manually. Fixup/squash commits should be part of all developers' toolkits.

Post reply on HN