from Hacker News

Stacked Diffs versus Pull Requests (2018)

by codesuki on 4/24/21, 6:07 AM with 49 comments

by strken on 4/24/21, 1:26 PM
I've used both, and I wouldn't describe the time savings as significant.
Compare the case study of Yang, who runs either
```
    git checkout neighbour-fix
    git checkout -b yang-work
    <time passes>
    git checkout neighbour-fix
    git pull
    git checkout yang-work
    git rebase neighbour-fix
```
or
```
    arc patch --diff D12345 # arc's a CLI tool for Phabricator
    <time passes>
    git fetch
    git rebase -i HEAD~2
    <goes into phabricator and finds the new commit hash for D12345>
    <pastes it into vim in place of the original commit hash>
```
From this, it's not clear to me that Yang is getting the better deal if he uses Phabricator. Certainly, he has to enter fewer commands, but he also has to leave the terminal to go copy a commit hash from Phabricator, and then paste it into an interactive rebase session. If given the choice, I'd choose the PR workflow over the Phabricator workflow, based on not switching to the browser and not needing to copy-paste things into an interactive rebase. Phabricator and arc handle deep stacks of diffs more nicely, but that's rare enough that I'd rather optimise for the common case.
Apologies if I've misunderstood exactly which commands Yang would run in either case.
by nhaehnle on 4/24/21, 3:50 PM
One related aspect that the article doesn't go into too much is that there is a tension in the size of the unit of code review: there are reasons for reviewing big chunks at once, but also reasons for reviewing individual changes that are as small as possible. I've gone into more detail on this in the past.[0]
Stacked diffs make that possible because you can review either individual commits or an entire stack at once.
The irony is that this is largely the way that Linux kernel development works -- and the Linux kernel was the first user of Git! Most projects who later adopted Git seem to have never learned this lesson that was there from the beginning, and have since been reinventing wheels.
[0] http://nhaehnle.blogspot.com/2020/06/they-want-to-be-small-t...
by captainmuon on 4/24/21, 9:04 AM
Hmm, I don't get it:
> If you want to have your code reviewed, you first have to branch master, then commit to that branch, then push it remotely, then create a PR.
When I want to create a PR, I always create a feature/WIP branch locally and push that. Branches are cheap in Git.
I find when the unit of scrutiny is an individual commit, like when you contribute to the Linux kernel, I spend way to much time fabricating nice atomic commits. Like maybe 50%, with 30% actual development time and 20% normal cleanup.
The only thing I miss with PRs is the ability to rewrite my branch after showing it. I think it is not supported to do `rebase -i` and then `push --force` to a PR, is it? It would be really useful, especially for the cases where you have multiple commits that cancel each other out (e.g. add debugging output, remove debugging output).
by marcinzm on 4/24/21, 2:33 PM
The piece seems to make the most negative assumptions about one workflow and the most optimistic ones about the other. Or rather the assumption that you take the effort to learn the optimal way to use one workflow but take no effort to do the same for the other. Most everything looks great when you take that approach.
Things like making a branch off of another branch being somehow so difficult that no one would ever even consider it. Or that there isn't a Github CLI tool that allows you to avoid having to go to the UI.
by FrenchyJiby on 4/24/21, 4:00 PM
I agree with most of the point of the article (I'm a huge Phab fan), but I feel like the explanation of what stack diffs are and why it matters was a bit lacking.
I usually describe it by comparing it to floating patches instead of anchored branches:
(Phabricator) diffs are more like git patches than commits/branches for review. They can be "grafted" on master or other places by applying the patch anywhere master or local branch, the diff is "floating". PRs are specific commits / commit ranges to review and adopt by merging/rebase-squashing, but are "anchored" to the place they branched off of.
This matters most when fixing 1 bug means multiple separate change that would be best staying separate commits, having to make N pull requests that depend on each other leads to rebase/forcepush cascades, which sucks. Having "patches" ready to apply means that halfway through "landing" (closing/merging) the diffs, the remaining diffs can be grafted on actual master instead of the (now closed) diff below, instead of having to rebase forever to update Github UI.
One major downside of this workflow is that it's different from what git teaches you, so every newcomer needs to learn "the Phab way", which gets people grumpy, and is only realistically feasible in a workplace, not really in a FOSS project as it increases the bar to contribution.
by xyzzy_plugh on 4/24/21, 2:58 PM
And then there's tools like Gerrit, which Just Work. Phabricator and GitHub are both very hostile to "stacked diffs".
by codesuki on 4/24/21, 10:03 PM
I think https://news.ycombinator.com/item?id=26925235 put it nicely.
PRs are expensive. So when you think 'this is a self contained change, but I need it to continue development' you are unlikely to send a PR for it. You will just pile on more code.
Because if you make another PR based on that first PR and someone finally does code review and tells you to fix something then you suddenly need to propagate these changes to all follow up PRs which is quite some work.
And while I think I might just miss something obvious it seems many people miss it that's why we have blog posts about 'stacked PRs'. (On HN a day ago or so)
by thatnerdyguy on 4/24/21, 1:35 PM
They seem to be conflating a few different things here. We are right now doing branch-from-master-and-squash-merge-pr, just do we don't have the bisection issues they mention.
Having a far greater emphasis on even smaller units of work for a single feature (per commit) seems to unnecessarily increase cognitive load? Like now I have to do even more "planning ahead" of how my changes are going to inter-relate, and find some way to separate them out. I mean, that is ideal, but... software also has to be written.
by twic on 4/24/21, 11:09 AM
Or you could just do trunk based development.
by greatgib on 4/24/21, 11:06 AM
In cases like that, you miss the magic of svn where you could easily commit/push small parts of the code independently in the current branch.