6 ms·

I've worked at both FB and at smaller companies. The "stacked diff" workflow is quite good, although it can be replicated via Github's PRs via "logical commits.

I've worked at both FB and at smaller companies. The "stacked diff" workflow is quite good, although it can be replicated via Github's PRs via "logical commits." The basic workflow is: 1. Each commit is a Phabricator-style diff. Put the whole idea into a single commit via `git add -p` and rebasing. 2. Multiple ideas/diffs for the same feature? Split them into multiple commits on the same PR. 3. Review the PR commit-by-commit: you can click on the links in Github to the individual commits and see+comment on the changes there. Comments on commits automatically propagate back to the PR FWIW, so this works pretty well. The mental model becomes: the PR is the overview of the full feature, but not how reviewers are expected to review. Reviewers review commits. It works pretty well! Assuming you have buy-in from your team to operate this way. In fact, I actually somewhat prefer it: with "stacked diffs," it can be harder to get the birds-eye view of the entire feature's set of changes. Git and Github are very flexible, so in practice I find you do need some established workflows that everyone agrees to follow, or else it's chaos.

s4i 5y ago
But, when using GitHub, isn't the missing puzzle piece here (which Graphite and other "stacked review tools" mentioned here attempt to solve) that you then cannot just merge those individual PRs one by one – you either have to merge the whole PR or nothing? Which leads us to the second option: to use multiple dependent PRs, which causes its own issues explained in other comments.
- tumetab1 5y ago
  I think his/her perspective, and mine, is that we don't want to merge individual commits. My personal feeling is that I need to review each commit and then I need to review the whole PR (list of commits). If both are good then the PR can be merged but each individual commit is not OK to merge unless it's part of a PR.
- Liskni_si 5y ago
  > you either have to merge the whole PR or nothing That's not true, though. You absolutely can merge a single commit, via git push, there's just no button in the GitHub UI for it.
  - s4i 5y ago
    > You absolutely can merge a single commit, via git push, there's just no button in the GitHub UI for it. Well, I'm asuming we are talking about PRs against some protected branch where manually pushing changes directly is forbidden.
- reissbaker 5y ago
  Stacked diffs in practice were often deployed as a unit at FB, in my experience, with the "Ship stacked diffs" button. There's maybe a little value in merging individual commits in a PR, but I think not a lot: if all the commits aren't in master, the feature doesn't work anyway. Technically you get a little bit of an edge from merging individual commits if earlier ones are acceptable and later ones aren't, in that those commits land in master faster and are less likely to have to deal with merge conflicts later — but in practice I didn't find this to be a dealbreaker. Most PRs weren't enormous, and most files weren't subject to many engineers modifying the exact same lines concurrently, so overall review time didn't usually cause too many conflicts — and conflicts usually weren't hard to solve anyway.
  - exclipy 5y ago
    The "stacked diff" workflow means each diff lands as a separate squashed commit on master, even you click Ship Stack to land all of them in one CI run. This has advantages for code archaeology reasons.
xapata 5y ago
Second. Squashing and amending commits is equivalent to the workflow described in the article.
kristjansson 5y ago
This should describe commits for a normal PR workflow too though - each commit is the smallest deployable unit with a meaningful title and description. I guess squashing merges by default allows / encourages people to put less thought into individual commits since they aren’t expected to become part of the permanent history?
deleted 5y ago
[deleted]
exclipy 5y ago
But you can't amend a commit without destroying the old version of it. Stacked diffs run in two dimensions. In one dimension is the stack of diffs -- logical incremental changes to the code, which make sense to code historians from the future. But each diff in the stack also has a history as it progressed through code review.

s4i 5y ago

But, when using GitHub, isn't the missing puzzle piece here (which Graphite and other "stacked review tools" mentioned here attempt to solve) that you then cannot just merge those individual PRs one by one – you either have to merge the whole PR or nothing? Which leads us to the second option: to use multiple dependent PRs, which causes its own issues explained in other comments.

tumetab1 5y ago
I think his/her perspective, and mine, is that we don't want to merge individual commits. My personal feeling is that I need to review each commit and then I need to review the whole PR (list of commits). If both are good then the PR can be merged but each individual commit is not OK to merge unless it's part of a PR.
Liskni_si 5y ago
> you either have to merge the whole PR or nothing That's not true, though. You absolutely can merge a single commit, via git push, there's just no button in the GitHub UI for it.
- s4i 5y ago
  > You absolutely can merge a single commit, via git push, there's just no button in the GitHub UI for it. Well, I'm asuming we are talking about PRs against some protected branch where manually pushing changes directly is forbidden.
reissbaker 5y ago
Stacked diffs in practice were often deployed as a unit at FB, in my experience, with the "Ship stacked diffs" button. There's maybe a little value in merging individual commits in a PR, but I think not a lot: if all the commits aren't in master, the feature doesn't work anyway. Technically you get a little bit of an edge from merging individual commits if earlier ones are acceptable and later ones aren't, in that those commits land in master faster and are less likely to have to deal with merge conflicts later — but in practice I didn't find this to be a dealbreaker. Most PRs weren't enormous, and most files weren't subject to many engineers modifying the exact same lines concurrently, so overall review time didn't usually cause too many conflicts — and conflicts usually weren't hard to solve anyway.
- exclipy 5y ago
  The "stacked diff" workflow means each diff lands as a separate squashed commit on master, even you click Ship Stack to land all of them in one CI run. This has advantages for code archaeology reasons.

xapata 5y ago

Second. Squashing and amending commits is equivalent to the workflow described in the article.

kristjansson 5y ago

This should describe commits for a normal PR workflow too though - each commit is the smallest deployable unit with a meaningful title and description. I guess squashing merges by default allows / encourages people to put less thought into individual commits since they aren’t expected to become part of the permanent history?

deleted 5y ago

[deleted]

exclipy 5y ago

But you can't amend a commit without destroying the old version of it. Stacked diffs run in two dimensions. In one dimension is the stack of diffs -- logical incremental changes to the code, which make sense to code historians from the future. But each diff in the stack also has a history as it progressed through code review.