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

201–210 of 224 posts

Re: Some of us like "interdiff" code review

#201

Post by riwsky »

zoogeny wrote: post #193
steveklabnik wrote: post #185

Earlier quoted context omitted.

Do you only send PRs with one commit in them every time? I find in my work that if I were doing a three-commit series like this, I would end up sending in a PR on Thursday, containing all three commits. This is because I can’t be sure the refactor works well until I use the results of it with the new code, for example. In the scenario I’m talking about, to adapt it to yours, both developers send out their PR or stack…

Every PR is a single commit. The only extra commits on a PR would be "Review feedback", etc. But it would be squash merge and the final commit message updated to include any important details related to the changes from the feedback. Consider the case from the article. refactor: 25, api: 500, migration: 50. Lets say 25 = 1 day of work so we have refactor: 1 day, api: 20 days, migration: 2 days. I'll decrease api to 5…

You’re actually making the same argument that the article is—it’s just hard to tell because terms are being used differently.

Where you talk about merging PRs early, the article talks about merging commits early: “You don't have to wait on all 5 commits to be ready to go; maybe the first 3 are OK, and the last 2 need more work. You merge 3 out of 5.”

But what you call a disruption-reducing tradeoff, the article argues is just shitty UX on GitHub’s part. The stacking-native tools it mentions are what you’d get if you took the workflow you propose and let devs start task B (and then maybe even C) immediately after task A, instead of making them wait for reviewer sign-off on A.

Re: Some of us like "interdiff" code review

#202

Post by zeotroph »

lolc wrote: post #175
zeotroph wrote: post #38

Earlier quoted context omitted.

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.

All this rebasing sounds like constant pain to pull from Gerrit. Does it actually create new branches v2 v3 after a rebase? Or how do I switch my local checkout to the rebased branch from remote?

That's just a `git pull --rebase` away (or set pull to never merge but rebase), and all the non-merged commits are rebased onto the new upstream, and while doing the rebase the already-merged commit is dropped. The next git push origin HEAD:refs/for/main will then push the remaining commits.

Re: Some of us like "interdiff" code review

#203

Post by steveklabnik »

zoogeny wrote: post #193
steveklabnik wrote: post #185

Earlier quoted context omitted.

Do you only send PRs with one commit in them every time? I find in my work that if I were doing a three-commit series like this, I would end up sending in a PR on Thursday, containing all three commits. This is because I can’t be sure the refactor works well until I use the results of it with the new code, for example. In the scenario I’m talking about, to adapt it to yours, both developers send out their PR or stack…

Every PR is a single commit. The only extra commits on a PR would be "Review feedback", etc. But it would be squash merge and the final commit message updated to include any important details related to the changes from the feedback. Consider the case from the article. refactor: 25, api: 500, migration: 50. Lets say 25 = 1 day of work so we have refactor: 1 day, api: 20 days, migration: 2 days. I'll decrease api to 5…

Yeah, as my sibling says, you’re making the same argument the article is, it’s just that apparently for you, reviews and merging happen so quickly you never have outstanding PRs depend on each other, so you never need to stack. Treating the commit as the unit of review than a branch as the unit of review is the more fundamental difference, and you’ve got that.

Re: Some of us like "interdiff" code review

#204

Post by dietr1ch »

pinkorchid wrote: post #131
dietr1ch wrote: post #125

Earlier quoted context omitted.

Squashing is nice because it keeps the amount of commits minimal, but as mentioned in the article, it has the problem that now reviewers have to figure out what changed since their last review, which can get hard to do if the tree of changes originally proposed needed updates in dependencies of the commits they reviewed. I really like the idea that I'm working on a tree (often a stack/list) of changes, and as the rev…

> the problem that now reviewers have to figure out what changed That's something that the review tool can solve, and I agree that github doesn't handle it well. But other code review tools can show diffs between revisions independently of how the commits have been rebased or squashed over time.

That's part of the problem, if you are a reviewer you'll see the changes that other reviewers asked for on code that you did not want to review.

I guess this might be a problem mostly on large projects on monorepos, where different systems that interact end in the same codebase, but there's a cost to be paid too if each system has their own repo, build/test/deploy infra and independent ownership.

Re: Some of us like "interdiff" code review

#205

Post by keybored »

another-acct wrote: post #183
keybored wrote: post #126

As the author is aware of[1][2] the Git project uses this interdiff approach with email. - Patch series (PR equivalent) go through a round of reviews - Each version has a cover letter (like PR desription) unless it’s only a one-patch series - Each version has that optional cover letter with each patch as a reply email to it - The next version is a reply to the previous cover letter - And each version 2 and above cove…

This is the comment I've been looking for! :) High-five! Some remarks: > - The next version is a reply to the previous cover letter Not necessarily; sometimes the new version is not posted in-reply-to anything, but the cover letter includes a reference (usually message-id + web-archive URL) in the body. Depends on the project I guess. > And each version 2 and above cover letter has a git-range-diff in it (courtesy of…

> Never heard of "amlog"

Everything I discussed was for the Git project.

https://lore.kernel.org/git/20180724094501.GA3578@sigill.int...

Re: Some of us like "interdiff" code review

#206

Post by lolc »

zeotroph wrote: post #202
lolc wrote: post #175

Earlier quoted context omitted.

All this rebasing sounds like constant pain to pull from Gerrit. Does it actually create new branches v2 v3 after a rebase? Or how do I switch my local checkout to the rebased branch from remote?

That's just a `git pull --rebase` away (or set pull to never merge but rebase), and all the non-merged commits are rebased onto the new upstream, and while doing the rebase the already-merged commit is dropped. The next git push origin HEAD:refs/for/main will then push the remaining commits.

Ok thanks, that sounds reasonable. Now I want to try Gerrit!

Re: Some of us like "interdiff" code review

#207

Post by Vinnl »

riquito wrote: post #121
Vinnl wrote: post #83

I'm using mostly this workflow with GitHub, with the main disadvantages being that it's more work on my side, and not obvious to my collaborators. But it does carry the same advantages of allowing reviewers to view diffs with just their feedback incorporated, without breaking `git blame` and `git bisect`. When I incorporate a reviewer's feedback, I'll commit that with `git commit --fixup `. I'll then push that up and…

You can add to your ~/.gitconfig [rebase] autosquash = true from then on `git rebase -i origin/main` automatically reorder fixup and squash commits. It's a small thing but greatly improved my workflow

Thanks, but I do every now and then also want to rebase without applying the fixup commits yet. Just using shell autocomplete gives me the flexibility to do that with little effort. (And honestly since it already autocompletes if I type `git re`, this nor Git aliases wouldn't even really save me anything anyway.)

Re: Some of us like "interdiff" code review

#208

Post by Vinnl »

dmurray wrote: post #197
shambulatron wrote: post #152

Earlier quoted context omitted.

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.

There's also `git rebase --onto`, which effectively does the reverse - you tell git you've already rebased the part of this branch that overlapped with the "intermediate" branch, and it just needs to take care of the rest.

Oh that sounds exactly what I need to simplify the workflow I described for the multi-PR use case. I'm going to try that out today, I just happen to have a use case for it ready to go :)

Re: Some of us like "interdiff" code review

#209

Post by Vinnl »

aseipp wrote: post #100
Vinnl wrote: post #83

I'm using mostly this workflow with GitHub, with the main disadvantages being that it's more work on my side, and not obvious to my collaborators. But it does carry the same advantages of allowing reviewers to view diffs with just their feedback incorporated, without breaking `git blame` and `git bisect`. When I incorporate a reviewer's feedback, I'll commit that with `git commit --fixup `. I'll then push that up and…

Yes, fixup commits are a good way to approach this, though I don't personally like them, though. I think Sapling's "absorb" command which works on an underlying SCCS weave to automatically absorb changes into the relevant diff is much more elegant. It prompts you with an interactive UI. (For me, it sorta falls into the same space as rebasing a series that has multiple branches on it. You need --update-refs for that.…

I really want to give Sapling a try one day, yeah.

Note that the workflow I described is indeed mostly a workaround for people, like me, stuck with GitHub.

Re: Some of us like "interdiff" code review

#210

Post by keybored »

another-acct wrote: post #183
keybored wrote: post #126

As the author is aware of[1][2] the Git project uses this interdiff approach with email. - Patch series (PR equivalent) go through a round of reviews - Each version has a cover letter (like PR desription) unless it’s only a one-patch series - Each version has that optional cover letter with each patch as a reply email to it - The next version is a reply to the previous cover letter - And each version 2 and above cove…

This is the comment I've been looking for! :) High-five! Some remarks: > - The next version is a reply to the previous cover letter Not necessarily; sometimes the new version is not posted in-reply-to anything, but the cover letter includes a reference (usually message-id + web-archive URL) in the body. Depends on the project I guess. > And each version 2 and above cover letter has a git-range-diff in it (courtesy of…

And thanks for the feedback. It’s nice to find people who think this is interesting as well. :-)
Post reply on HN