Hacker News Board Hacker News, browsed like a classic message board
Live data from Hacker News

Stacked Diffs versus Pull Requests

jg.gg

Post reply on HN
Print view View original

61–70 of 77 posts

Re: Stacked Diffs versus Pull Requests

#61

Post by WorldMaker »

krupan wrote: post #48
jorams wrote: post #36

The idea of "stacked diffs" looks like a good one, but this article doesn't do a great job of presenting them. It's way too long for the actual claims it makes. It combines two things: Working on a single branch and reviewing commits instead of branches. Reviewing commits instead of branches makes a lot of sense, and would definitely result in a cleaner history than pull requests seem to do in practice. The article a…

You are missing the subtle and hard to explain point that he tried to illustrate with the Tale of the Tree Icon. It's that our tools and their defaults shape the way we work. I watched a team switch from branches to stacked diffs (mercurial queues, way back in the day) and it was almost exactly how this article describes.

It's the tools and their defaults that leads me to my biggest criticism with the article. On darcs something like "stacked diffs" makes a ton of sense because the UI was built for it from top down, and cherry-picking is _magic_ (when it works), including that it tracks your dependencies between commits/patches for you.

In git, we have a dependency tracker in the DAG, but it requires the work of branching to make use of its power. In git, cherry-picking is a footgun with all sorts of potential problems. I've seen far too much work lost to bad rebases to trust junior developers with cherry-picking and rebase-heavy workflows in git. (Yes, I realize that a lot of people disagree and a lot of people use very heavy rebase workflows, this is just my opinion that in general rebase workflows are more danger than I want in my source control, as the resident source control guru on many of the teams I've worked on.)

Pull Requests actually work really well with this dependency tracking. If I create a PR for the database changes, a PR for the REST API changes, and a PR for the UI work that depends on both of the previous two, and all of those PRs are well merged branches (no rebases) using the DAG to encode dependencies, my reviewers have the option to review those in any order. If they review the two smaller subset PRs first, as that content merges to master most good PR tools simplify the remaining UI PR to just the UI stuff not in master. If they review and merge everything all at once in the giant UI change PR, then most good PR tools will automatically mark the other two PRs as merged and complete, no work needed for individual review of the two subset PRs. The choice there is left to the reviewers, and the tools just do the right thing, whatever the preference the reviewers have.

Re: Stacked Diffs versus Pull Requests

#62

Post by WorldMaker »

dom96 wrote: post #46
yumaikas wrote: post #39

How does this workflow work when there are 3 commits that need to be applied in order (A, B, and C), when C gets approved from a code-review standpoint before A and B?

You are prevented from merging commits at the top of the stack until the commits below are accepted. The advantage is that each commit can be reviewed separately. You cannot achieve this with pulled requests as far as I know. I often see people creating a pull request and writing "this depends on PR #1233", the second PR is unreviewable because it contains changes from. The first PR. So the first PR has to merged fir…

If the branches are properly merged so that the dependency is represented in the git DAG, most good PR tools just handle this correctly: if you merge the dependent PR first the dependee updates to show the commits as already merged and "simplify" their diff views appropriately; if you merge the dependee PR first, the dependent PR simply gets marked as completed/merged for you.

I find it is still useful to include helpful hints like "this PR includes all of PR #1234, if you prefer to review a piece at a time", but that's for the reviewers' sake, not the tools'.

Re: Stacked Diffs versus Pull Requests

#63

Post by rfoo »

falsedan wrote: post #16
stevebmark wrote: post #10

This is very long and i'm tired, is there a more condensed resource?

I think the tl;dr is “stacked diffs require running different commands than git pull --rebase” The author complains about wasting time branching and merging, but other examples at the end still have the same number of operations and still hit the same merge conflicts. They make the point of how dependant work requires multiple branches but I feel it’s a workflow constraint, or a technical one. It can be easily avoide…

As the article pointed out, the problem here is you can't easily deal with multiple branches stacking on each other.

You may argue that it is possible to put all these changes into one branch, but "asking 6 people to review a 500+ lines change, in which only ~100 lines is meaningful for each reviewer" is not a nice thing to do and it obviously delays the code review process.

Re: Stacked Diffs versus Pull Requests

#64

Post by UK-Al05 »

yxhuvud wrote: post #57
lima wrote: post #28

Earlier quoted context omitted.

You can do stacked diffs on GitHub by always using squash merges and rebasing on top of master, and it's a great way to get started. It's just a lot nicer with the extra tooling on top that Phabricator and Gerrit provide (dependencies, command line tools, and so on).

If you squash merge you just lost all the small commits that do one thing well that you spent time creating, unless you review them one by one. Bleh.

My pull request branches are of the size that it only makes sense together.

Re: Stacked Diffs versus Pull Requests

#65

Post by pdpi »

donatj wrote: post #24
petters wrote: post #6

> How do you git bisect a codebase where every 6th commit doesn’t build because it was jammed into the middle of a Pull Request? This I think is the main problem with pull requests. A better default would be squashing the commits into a single one after review.

I have always been of the mind that every commit should be a working state. My coworkers who often commit with the message “WIP” disagree. I doubt I’ll ever sell them on it.

The stacked diff approach says: You get to have both, because it pushes you towards a rebase-heavy workflow, where you keep updating the WIP commits until they're ready to merge. This is a pain in the arse with git, but `hg absorb` makes it 100% painless

Re: Stacked Diffs versus Pull Requests

#66

Post by ajuc »

donatj wrote: post #24
petters wrote: post #6

> How do you git bisect a codebase where every 6th commit doesn’t build because it was jammed into the middle of a Pull Request? This I think is the main problem with pull requests. A better default would be squashing the commits into a single one after review.

I have always been of the mind that every commit should be a working state. My coworkers who often commit with the message “WIP” disagree. I doubt I’ll ever sell them on it.

WIP commits in gerrit are perfectly fine. You can even push them to gerrit with all the rest commits. Just filter "WIP.*" out in gerrit view that lists commits to review.

Re: Stacked Diffs versus Pull Requests

#67

Post by falsedan »

rfoo wrote: post #63
falsedan wrote: post #16

Earlier quoted context omitted.

I think the tl;dr is “stacked diffs require running different commands than git pull --rebase” The author complains about wasting time branching and merging, but other examples at the end still have the same number of operations and still hit the same merge conflicts. They make the point of how dependant work requires multiple branches but I feel it’s a workflow constraint, or a technical one. It can be easily avoide…

As the article pointed out, the problem here is you can't easily deal with multiple branches stacking on each other. You may argue that it is possible to put all these changes into one branch, but "asking 6 people to review a 500+ lines change, in which only ~100 lines is meaningful for each reviewer" is not a nice thing to do and it obviously delays the code review process.

I feel like my workflow address that exactly: by making a separate branch per reviewable change from the base of the problem branch.

Re: Stacked Diffs versus Pull Requests

#68

Post by zbentley »

zbentley wrote: post #51
Joky wrote: post #12

Earlier quoted context omitted.

"on top of the current master" is limiting: how do you handle concurrency? I.e. two developers kicking tests on their PR on top of current master, everything green, but they merge and the result of their merge is not building (or passing tests) anymore.

Most CI systems serialize builds on master, at least at the very end of the build pipeline. That can slow things down, but, if combined with a decently fast/thought-through build process with tears, I think the benefits of not risking untested states outweigh the costs.

*tests, not tears, but I guess it works either way...

Re: Stacked Diffs versus Pull Requests

#69

Post by dom96 »

WorldMaker wrote: post #62
dom96 wrote: post #46

Earlier quoted context omitted.

You are prevented from merging commits at the top of the stack until the commits below are accepted. The advantage is that each commit can be reviewed separately. You cannot achieve this with pulled requests as far as I know. I often see people creating a pull request and writing "this depends on PR #1233", the second PR is unreviewable because it contains changes from. The first PR. So the first PR has to merged fir…

If the branches are properly merged so that the dependency is represented in the git DAG, most good PR tools just handle this correctly: if you merge the dependent PR first the dependee updates to show the commits as already merged and "simplify" their diff views appropriately; if you merge the dependee PR first, the dependent PR simply gets marked as completed/merged for you. I find it is still useful to include hel…

Yes, that's nice. But the problem is that these are still shown as separate PRs, GitHub for example doesn't tie these together unlike Phabricator.

Re: Stacked Diffs versus Pull Requests

#70

Post by charrondev »

WorldMaker wrote: post #61
krupan wrote: post #48

Earlier quoted context omitted.

You are missing the subtle and hard to explain point that he tried to illustrate with the Tale of the Tree Icon. It's that our tools and their defaults shape the way we work. I watched a team switch from branches to stacked diffs (mercurial queues, way back in the day) and it was almost exactly how this article describes.

It's the tools and their defaults that leads me to my biggest criticism with the article. On darcs something like "stacked diffs" makes a ton of sense because the UI was built for it from top down, and cherry-picking is _magic_ (when it works), including that it tracks your dependencies between commits/patches for you. In git, we have a dependency tracker in the DAG, but it requires the work of branching to make use…

What PR tools are you referring to? We just use github pull requests, but stacking PRs seems impossible because merging the parent PR doesn’t seem to update the children PRs automatically.
Post reply on HN