4 ms·

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 cann

by 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.

by s4i 5y ago

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.