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

71–80 of 224 posts

Re: Some of us like "interdiff" code review

#71

Post by tome »

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.

Then fix up the commit history at the end, for example like this: https://news.ycombinator.com/item?id=41509051

Re: Some of us like "interdiff" code review

#72

Post by trashburger »

Forge36 wrote: post #49
danparsonson wrote: post #42

Earlier quoted context omitted.

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.

By doing this, you break commit atomicity and make bisects hell. Please don’t do this. Commits aren’t perfect at first for sure, but they should be by the time you make them reviewable.

Re: Some of us like "interdiff" code review

#73

Post by snatchpiesinger »

Another way to look at this that this is 3 linearly dependent PRs masquerading as one. Make each a distinct PR and the problem goes away, especially if you can mark the PR to depend on another one (on Gitlab you can, not sure about Github). If you want to see each change as a single logical unit, then they will each be a distinct merge commit on your master branch, use `git log --first-parent-only master` to only see these kind of changes.

Re: Some of us like "interdiff" code review

#74

Post by dangsux »

snatchpiesinger wrote: post #73

Another way to look at this that this is 3 linearly dependent PRs masquerading as one. Make each a distinct PR and the problem goes away, especially if you can mark the PR to depend on another one (on Gitlab you can, not sure about Github). If you want to see each change as a single logical unit, then they will each be a distinct merge commit on your master branch, use `git log --first-parent-only master` to only see…

[dead]

Re: Some of us like "interdiff" code review

#75

Post by snatchpiesinger »

trashburger wrote: post #72
Forge36 wrote: post #49

Earlier quoted context omitted.

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.

By doing this, you break commit atomicity and make bisects hell. Please don’t do this. Commits aren’t perfect at first for sure, but they should be by the time you make them reviewable.

It's fine to break commit atomicity on feature branches. You can use git bisect --first-parent on you development/master branch.

Re: Some of us like "interdiff" code review

#76

Post by zeotroph »

snatchpiesinger wrote: post #73

Another way to look at this that this is 3 linearly dependent PRs masquerading as one. Make each a distinct PR and the problem goes away, especially if you can mark the PR to depend on another one (on Gitlab you can, not sure about Github). If you want to see each change as a single logical unit, then they will each be a distinct merge commit on your master branch, use `git log --first-parent-only master` to only see…

> [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`.

Re: Some of us like "interdiff" code review

#77

Post by zeotroph »

globular-toast wrote: post #60

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…

> Developers just aren't very good at doing this.

GitHub provided a way to contribute, but also to avoid learning to rebase, thus making it more welcoming to devs who only know about commit and pull - that is what made it so popular. The squash then rebase or merge step is done on server side. Plus it has a very "harmless" UI, but that hides a lot of details (patchsets) and the layout wastes so much space imo.

This also means devs could avoid learning more about git, and this lowest common denominator git workflow makes it so frustrating for those of us who learned git all the way. I can't even mark a PR as "do not squash" to prevent it being merged in the default way which throws out all history.

Re: Some of us like "interdiff" code review

#78

Post by xelxebar »

aseipp wrote: post #33
rosmax_1337 wrote: post #20

Earlier quoted context omitted.

What is the meaning of life?

I know this is in jest, but I'll just take the opportunity to respond by posting my favorite poem. The relationship between it and your question -- well, that's for you to decide. The birds have vanished down the sky. Now the last cloud drains away. We sit together, the mountain and me, until only the mountain remains. -- Zazen on Ching-t’ing Mountain

Beautiful. I wasn't aware of Li Bai (李白). Took a bit of searching, but the original looks to be called 独坐敬亭山:

    獨坐敬亭山
    衆鳥高飛盡
    孤雲獨去閒
    相看兩不厭
    只有敬亭山
This is gorgeous. Thank you for sharing!

Re: Some of us like "interdiff" code review

#79

Post by torarnv »

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…

As far as I know, splitting the series into individual PRs only works if you have commit rights to the repository, so you can base one PR on a different branch (in the main repository) than main.

As an outside contributor, with a fork of the repository, your three PRs will incrementally contain change A, A+B, and A+B+C, making the review of the last two PRs harder, because you need to review diffs for code you're already reviewed in another PR.

Re: Some of us like "interdiff" code review

#80

Post by torarnv »

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.

Amazing summary of the mental model of the interdiff review style, and the problems with GitHub's approach to code review. Thank you!

One thing you don't touch upon (yet) is that the diff soup may lead people to prefer the squash merge strategy, to get rid of the "noise" of the fixup commits, which throws away the "good" initial 3 atomic commits as well.

With interdiff review style you're left with the initial 3 commits, and the choice of whether to land them individually or squash them is based entirely on the commits themselves and how atomic they really are.

Post reply on HN