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

41–50 of 224 posts

Re: Some of us like "interdiff" code review

#41

Post by zeotroph »

zdw wrote: post #2

I've generally found that code review first, and rebase-centric systems like Gerrit tend to be much easier to review code in. One of the best parts of this is native support for stacking multiple patches, so people make smaller patches that are easier to review. Code review in Github feels like a bad afterthought - the space-wasting interface that looks more like a forum thread, the inability to track over rebases, e…

There is one thing I miss on Gerrit when you push a stack of commits: A central place to talk about the whole of the stack, not just individual commits. This "big picture", but still technical stuff, too often happens in the issue tracker. But where to place it, I have no idea. This stack is just too ephemeral and and can be completely different on the next push.

Re: Some of us like "interdiff" code review

#42

Post by danparsonson »

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…

I'd be quite happy with seeing the three jobs in the article as three separate PRs. Fixing a bug and adding a feature are two jobs that, as I think we all agree, need to be tracked individually - so work on them individually.

> As well, sometimes CI won't pass on one of the stages meaning it can't be a separate PR

Could you give an example of this? Not sure what you mean.

Re: Some of us like "interdiff" code review

#43

Post by zeotroph »

jeffbee wrote: post #14
ajkjk wrote: post #10

Earlier quoted context omitted.

Better it be a deep dive into developer sanity, a more important but comparatively under-valued metric.

For me, GitHub PR review drives me crazy. It's good for exactly one round of exchange. After that nobody can tell what the heck is going on. So my self-reported mental health would be worse. But on non-subjective metrics it seems like LLVM PRs on GitHub are gathering noticeably less discussion than they used to enjoy as Phabricator diffs.

And just visually, GitHub wastes so much vertical space, so even trying to place what belong to which patchset becomes hard.

Re: Some of us like "interdiff" code review

#44

Post by pronoiac »

I'm midway through, but a nitpick:

> You're on your own figuring out the Commit IDs and punching them into the URL bar if you want something more granular.

There's a "commits" tab at the top, like: Conversation | [Commits] | Checks | Files changed

Example: https://github.com/raspberrypi/linux/pull/6330/commits

Re: Some of us like "interdiff" code review

#45

Post by strken »

This is interesting. At work we use PRs like the author uses commits, and in fact we squash-and-merge them at the end, but our approach requires rebasing the later PRs whenever we make a change to the earlier PRs. This can be quite laborious, falls afoul of the "don't force-push" rule, takes a long time for engineers to learn, and tends to break existing code review comments in the GitHub interface, but works out okay for two or three PRs. In our workflow a commit is less a unit of work and more a savepoint.

I also use heavily use stashes, as well as undo-tree-mode in Emacs. This means we have four different ways of tracking the history of source code, which sounds redundant but works out okay in practice.

The ergonomics of doing this in Git are pretty bad. I found Phabicator better but still unnecessarily difficult. Perhaps a new source control management tool could have first class support for higher level concepts than just commits and branches, or perhaps that would be even worse to use.

Re: Some of us like "interdiff" code review

#46

Post by aseipp »

pronoiac wrote: post #44

I'm midway through, but a nitpick: > You're on your own figuring out the Commit IDs and punching them into the URL bar if you want something more granular. There's a "commits" tab at the top, like: Conversation | [Commits] | Checks | Files changed Example: https://github.com/raspberrypi/linux/pull/6330/commits

I more meant that it's hard to diff between arbitrary commits without using the URL bar. The results are also... Weird. For example, let's say you have the base B, and commit X:

    B  ---> X
Now someone pushed to main, so you rebase on B'

    B' ---> X
Now, you modify X to address something (maybe just a spelling error)

    B' ---> X'
Now you push the new rebased branch. Question: how do you view the difference between X' and X?

Well, you have to use that little "Compare" button, but there's a really big problem with it: it shows you the diff from X to X' and the diff from B to B' at the EXACT same time. Which is really bad! Imagine if the difference between B and B' is 500 lines; it will completely dwarf the 1 line typofix from X to X', making it impossible to read.

Now, this is kind of a problem in Gerrit too. But they use a UX technique to make it manageable, which is very smart: they color-code the lines of the diff, depending on if the diff comes from B..B' or from X..X' -- so you can see at a glance if the hunk is relevant.

More broadly, interdiffing between commit X and commit Y can be tricky, because what you really want to do is something like "Rebase Y onto the parent of X, then diff X and Y", because otherwise you get the included differences between their baselines. (We do "Rebase Y on X's parent" for Jujutsu's "interdiff" command IIRC?) But sometimes you DO want to include the base diff, because the changes from B..B' can be VERY relevant to your patch.

So, you need all these options, really. But once you go off this beaten path where you want to compare X to Y... yeah, you have to start typing into the URL bar, I think.

But I will say, the commits tab and the little n/p keyboard shortcuts to "flip through the commits" like book pages is at least a HUGE improvement over the basic UX though. I use that all the time on GH projects these days, even if I have tons of other problems.

Re: Some of us like "interdiff" code review

#47

Post by zeotroph »

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

Attention Set Makes me wonder what other git or dev-in-general blindspots I have.

Re: Some of us like "interdiff" code review

#48

Post by gleenjello »

danparsonson wrote: post #39
gloryjulio wrote: post #27

Earlier quoted context omitted.

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.

[dead]

Re: Some of us like "interdiff" code review

#49

Post by Forge36 »

danparsonson wrote: post #42
lozenge wrote: post #12

Earlier quoted context omitted.

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…

I'd be quite happy with seeing the three jobs in the article as three separate PRs. Fixing a bug and adding a feature are two jobs that, as I think we all agree, need to be tracked individually - so work on them individually. > As well, sometimes CI won't pass on one of the stages meaning it can't be a separate PR Could you give an example of this? Not sure what you mean.

Commits aren't always perfect.

Sometimes I'll make the unit test first, which fails CI and the next set of commits implements the behavior.

Re: Some of us like "interdiff" code review

#50

Post by deathanatos »

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

I 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.)
Post reply on HN