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

141–150 of 224 posts

Re: Some of us like "interdiff" code review

#141

Post by keybored »

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…

The author already has a branch and its commits. What then would be the purpose of splitting the branch into three pull requests with one commit each?

- Each commit can be reviewed on its own

- Dependency tracking between commits? Yes...

- They don’t have AI built-in (a good thing)

- You can have an interdiff between commits unlike PRs

Commits are the bread and butter of Git. Just use commits.

Re: Some of us like "interdiff" code review

#142

Post by keybored »

steveklabnik wrote: post #93
fHr wrote: post #56

Earlier quoted context omitted.

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?

This is called "stacked diffs" and it's a good workflow; the issue is that it's annoying to use on GitHub without tooling. The "point A back to master" bit isn't easy/obvious with pull requests.

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.

Re: Some of us like "interdiff" code review

#143

Post by travisb »

Izkata wrote: post #95

As someone who has been on a maintenance team for years and regularly has to dig through the history to figure things out, I strongly prefer the original "bad" version with 7 individual commits. Yes "git blame" takes a little bit of extra work to get through all the commits, but knowing what initial mistakes were made and refactors done makes it much easier to tell what the original intent was. For example, if "fix b…

I concur.

I don't know why people are so happy to throw out the history. I find it's not uncommon that bugs are introduced during rebasing, and merging in from master. Sometimes this is due to incorrectly resolved conflicts, sometimes due to concurrent changes elsewhere in the codebase.

With --first-parent it isn't hard to have your cake (full history) and eat it too (linear mainline history).

Re: Some of us like "interdiff" code review

#144

Post by keybored »

strken wrote: post #45

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

You use commits as PRs. This complicates things because of PR dependencies.

Why not a better tool than Git? Sure, I would like a more user-friendly tool as well. Less warts. But what about the stopgap solution of simply treating commits like commits instead of making things harder for yourself?

(I wonder how much GitHub is to blame for giving people these PR-colored glasses)

Re: Some of us like "interdiff" code review

#145

Post by [deleted] »

aseipp wrote: post #16
ajkjk wrote: post #8

100% agree that this is ideal, the way Github does it is completely godawful and it's a tragedy that so many people have it normalized for them. We did this with Phabricator, although it was a somewhat-manual process, helped along by having some command line macros for updating all the reviews at once. But better still would be an explicit UI for it.

I am the author and used the phrase "Code review is a pretty good idea, in general" in the opening very specifically, because it used be one of the selling points listed on the Phabricator homepage. :) I miss it.

[deleted]

Re: Some of us like "interdiff" code review

#146

Post by keybored »

rfoo wrote: post #137
steveklabnik wrote: post #94

Earlier quoted context omitted.

> 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. You should check out jj, sapling, or mercurial.

If you are stuck with git, try git-branchless. At least it makes rebasing your patch stack easier (git sync && git submit).

Git-branchless gave me thousands of refs that a simple deinit wouldn’t delete. It’s so heavyweight for just experimenting with a few new workflows here and there.

I can’t recommend Git-branchless until it either tells me what it is about overall or figures it out for itself.

Re: Some of us like "interdiff" code review

#147

Post by keybored »

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.

Can someone explain to me why everything has to be done with PRs? Like you just have three commits for a PR. But the correct way is to split that up into three single-commit PRs? Why?

Not to mention that it doesn’t give you an interdiff. Because now you need to diff across three pull request.

It looks more like you are punting on the problem. Not solving anything.

Re: Some of us like "interdiff" code review

#148

Post by steveklabnik »

keybored wrote: post #142
steveklabnik wrote: post #93

Earlier quoted context omitted.

This is called "stacked diffs" and it's a good workflow; the issue is that it's annoying to use on GitHub without tooling. The "point A back to master" bit isn't easy/obvious with pull requests.

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 GitHub" it's largely about tooling that makes dependent PRs easier.

But there are other, non-GitHub tools. With those tools, you don't say "here's a branch, review it please," you say "here is a stack of commits, review them please." There's no branch going on inside. It's just a sequence of commits. This matters because it centers the commit as the unit of code review, not a branch. This also means that you can merge parts of the stack in at different times: to use the example from the article, once "small refactor" is good to go, it can be landed while "new API" is awaiting review. etc.

I think it takes actually using some of these tools to really "get it." I never understood them either, until I actually messed around with them. I am currently on my project solo, so I don't really stack at the moment, I think it really helps more the larger of a team you're working with is.

Re: Some of us like "interdiff" code review

#149

Post by steveklabnik »

rfoo wrote: post #137
steveklabnik wrote: post #94

Earlier quoted context omitted.

> 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. You should check out jj, sapling, or mercurial.

If you are stuck with git, try git-branchless. At least it makes rebasing your patch stack easier (git sync && git submit).

jj (and sapling, in my understanding) uses git as a backend, so you can use it even if you're stuck with git. Not that I think git-branchless is bad, mind you, but just to be clear about it.

Re: Some of us like "interdiff" code review

#150

Post by steveklabnik »

keybored wrote: post #144
strken wrote: post #45

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

You use commits as PRs. This complicates things because of PR dependencies. Why not a better tool than Git? Sure, I would like a more user-friendly tool as well. Less warts. But what about the stopgap solution of simply treating commits like commits instead of making things harder for yourself? (I wonder how much GitHub is to blame for giving people these PR-colored glasses)

Given that you can use git with Gerrit, I am confident in placing the blame on GitHub here.
Post reply on HN