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

151–160 of 224 posts

Re: Some of us like "interdiff" code review

#151

Post by rfoo »

snatchpiesinger wrote: post #81
zeotroph wrote: post #76

Earlier quoted context omitted.

> [GitLab] you can mark the PR to depend on another How much user interaction does that require, and how is this visualized in the review UI? Gerrit creates this dependency with a single `git push`.

It's a bit cumbersome, and I think only recently you can make longer dependency chains. It's certainly not automated away with just git commands, but maybe there there is a Gitlab API way. The only way I know is to "edit" the PR (or MR in Gitlab speak) and paste the URL into some "depends on" field, then save. There are certainly other problems as well, like you might have an MR 1 from feature1 to master, and MR 2 fr…

I usually just point the target branch of MR 2 to MR 1. After merging MR 1 GitLab automatically change it to the default branch so it's more or less okay.

However this makes updating these MRs very rebase heavy and as said in OP it is hostile to reviewers.

Re: Some of us like "interdiff" code review

#152

Post by shambulatron »

jrochkind1 wrote: post #118
mizzao wrote: post #97

Earlier quoted context omitted.

I was going to say... interactive rebase addresses a lot of the "diff soup" comments that the writer complains about. It's really only done by disciplined engineering teams though (who bother to learn some more advanced features of git)

I realized reading the first part of this article that I often want to set up a sequence of PR's, for very similar reasons as in the begininng of OP. Say, a prefatory refactor, then the main work, then some data cleanup. When I do this, the problem with rebase is that it kind of breaks the additional "next in sequence" PRs "on top", or at least requires (confusing to me) cleanup in all of them when I rebase the base.

Have you come across `git rebase --update-refs`? This automatically moves your "intermediate" branches during a rebase and sounds like it could be useful in your situation.

Re: Some of us like "interdiff" code review

#153

Post by steveklabnik »

zoogeny wrote: post #140

I've heard people argue for this strategy a few times but I am not convinced. Most projects I work on have feature branches that get squashed into a single commit (erasing the history of the branch). If we even have the case described by the author we would just do the three steps (refactor, new api, update) as 3 commits. One thing that has been a solid practice for me is to avoid long-lived branches. That seems to b…

The people I know who prefer stacked diffs argue that it makes it easier to integrate changes faster, no matter the size. Part of this is because unlike a PR on GitHub, you can land parts of a stack: to use the example from the article, if the "small refactor" diff is good to go, it can be landed without landing the "new api" and "migrate API users" diffs. Centering commits rather than branches has the effect of making for smaller overall commits rather than long-lived branches.

Re: Some of us like "interdiff" code review

#154

Post by keybored »

steveklabnik wrote: post #148
keybored wrote: post #142

Earlier quoted context omitted.

From the peanut gallery of HN I’ve never understood Stacked Diffs. It looks like they reinvented commits as dependent PRs. Which are stacked on top of each other like commits are.

Fun fact: part of the reason that this article is on HN, I believe, is because I linked it to someone on another site as a means of explaining stacked diffs. > It looks like they reinvented commits as dependent PRs. Sort of kind of. It really depends on what you mean by PR: if we're talking about "review this branch please," which is what I would argue most mean by PRs, then yes, in the context of "stacked PRs for Gi…

Oh hey, thanks for the explanation! I’ve been wondering about this for a while. The linked articles on HN tended to be heavy on arguing how unlike the workflow is to what “you are used to” that the description of what it was about got obfuscated.

I’ve used Git with email a little bit which also lets you review commits in isolation. It’s too bad that so many review tools bury the commits (ask me how many times someone at work has asked “what this is about” on the PR diff when the relevant commit message explains exactly that).

But what I like about email is that the whole series/PR also gets reviewed as a unit. Both worlds.

Re: Some of us like "interdiff" code review

#155

Post by steveklabnik »

keybored wrote: post #154
steveklabnik wrote: post #148

Earlier quoted context omitted.

Fun fact: part of the reason that this article is on HN, I believe, is because I linked it to someone on another site as a means of explaining stacked diffs. > It looks like they reinvented commits as dependent PRs. Sort of kind of. It really depends on what you mean by PR: if we're talking about "review this branch please," which is what I would argue most mean by PRs, then yes, in the context of "stacked PRs for Gi…

Oh hey, thanks for the explanation! I’ve been wondering about this for a while. The linked articles on HN tended to be heavy on arguing how unlike the workflow is to what “you are used to” that the description of what it was about got obfuscated. I’ve used Git with email a little bit which also lets you review commits in isolation. It’s too bad that so many review tools bury the commits (ask me how many times someone…

You're welcome. Yeah, it as a flow is much closer to the email workflow than the PR workflow, when you commit to it.

Re: Some of us like "interdiff" code review

#157

Post by quodlibetor »

I have been chasing the gerrit code review high since I left a company that used it almost 5 years ago.

Stacked pull requests are usually what people point to to get this back, but this article points out that _just_ stacked pull requests don't handle it correctly. Specifically with github, you can't really see the differences in response to code review comments, you just get a new commit. Additionally, often github loses conversations on lines that have disappeared due to force pushes.

That said, I have a couple scripts that make it easier to to work with stacks of PRs (the git-*stack scripts in[1]) and a program git-instafix[2] that makes amending old commits less painful. I recently found ejoffe/spr[3] which seems like a tool that is similar to my scripts but much more pleasant for working with stacked PRs.

There's also spacedentist/spr[4] which gets _much_ closer to gerrit-style "treat each commit like a change and make it easier for people to review responses" with careful branch and commit management. Changes don't create new commits locally, they only create new commits in the PR that you're working on. It's, unfortunately, got many more rough edges than ejoffe/spr and is less maintained.

[1]: https://github.com/quodlibetor/dotfiles/tree/main/dot_local/... [2]: https://github.com/quodlibetor/git-instafix/ [3]: https://github.com/ejoffe/spr [4]: https://github.com/spacedentist/spr

Re: Some of us like "interdiff" code review

#158

Post by ankitdce »

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…

If you are looking for an open source stacked PRs CLI, you can look into av CLI (https://github.com/aviator-co/av). Unfortunately this also only works with GitHub, but it should be possible to add support for any Git-based platform.

Disclaimer: I'm the founder of Aviator who supports the av CLI. It's a free tool to manage stacked PRs.

Re: Some of us like "interdiff" code review

#159

Post by ankitdce »

zeotroph wrote: post #47
aseipp wrote: post #40

Earlier quoted context omitted.

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.

Yeh, Attention Set is a game changer. We (Aviator) also took inspiration (ahem.. copied) attention set from Gerrit: https://docs.aviator.co/attentionset

Re: Some of us like "interdiff" code review

#160

Post by jrochkind1 »

shambulatron wrote: post #152
jrochkind1 wrote: post #118

Earlier quoted context omitted.

I realized reading the first part of this article that I often want to set up a sequence of PR's, for very similar reasons as in the begininng of OP. Say, a prefatory refactor, then the main work, then some data cleanup. When I do this, the problem with rebase is that it kind of breaks the additional "next in sequence" PRs "on top", or at least requires (confusing to me) cleanup in all of them when I rebase the base.

Have you come across `git rebase --update-refs`? This automatically moves your "intermediate" branches during a rebase and sounds like it could be useful in your situation.

I have not! I'll try to check it out, thanks!

The git cli still scares me when I get off the familiar path. :(

Post reply on HN