3 ms·

Stacked reviews are the crucial thing missing from GitHub pull requests. Truly, once you get used to working wit hthose, going back is intolerable, to the point

Stacked reviews are the crucial thing missing from GitHub pull requests. Truly, once you get used to working wit hthose, going back is intolerable, to the point where I now have a hacked together workflow where I live in git rebase -i and have it automatically push specially named branches for each commit in a stack.

selestify 6y ago
What are stacked reviews?
- erincandescent 6y ago
  You have a task to build a Thing, which involves creating the basic "Thing framework" and then implementing subfeatures A, B and C of into/on top of it Maybe you'll build the framework & subfeature A together, because you need some bit of meat in there to properly figure things out and be able to test things On Gerrit you'd probably create two commits which get pushed as two CLs * CL1 Create Thing framework * CL2 Implement A for Thing They're now pushed for people to review. While they're looking at them, you continue working on, making CL3 implementing B * CL3 Implement B for Thing One of your coworkers points out an issue in CL1, so you fix it (by amending the commit) and repush the stack. Now your stack is * CL1 Create Thing framework [v2] * CL2 Implement A for Thing * CL3 Implement B for Thing CL1 & CL2 get approved so you merge them (though typically with Gerrit they're cherry picked - CLs are individual Git commits). You push up an implementation of CL4, so your stack now looks like * CL3 Implement B for Thing * CL3 Implement C for Thing The important point is that CLs are atomic (and are reviewed atomically) even if they depend upon each other (i.e. are a part of a stack). When you're working in Git you typically (unless for whatever reason you have multiple unrelated stacks on the go, which is relatively rare) just work off of the master branch, so all the commits between `origin/master` and `master` - i.e. the set that automatically show up in `git rebase -i` and similar tooling - are your stack. When you pull, you `git pull --rebase` (or rather set the config for your machine or repo to default to that). When you've revised something in your stack or want to add something to it, you just do "git push HEAD:origin/refs/for/master" update Gerrit with the latest version of your stack It takes a little time to get used to (and you have to learn how to hold `git rebase -i` properly), but when you're used to it it's immensely more productive than the Github/clones PR flow (and doesn't involve manual branch juggling, etc). I can't express just how badly they deal at handling reviews of stacked PRs - either you create your stacked PR targeting master (and it gets all of the changes from the underlying PRs merged into its changes list, which makes reviewing it harder) or you target it at the branch of the underlying PR (which is non-obvious and painful and you have to manually remember to shift it across when the PR beneath it gets merged) When I'm in this situation when dealing with something which is PR based, I tend to end up merging CL1 & CL2 (because its easier to review things when you have an example) and hang on to CL3 & CL4 on my local machine until the first one gets merged
  - u801e 6y ago
    > The important point is that CLs are atomic (and are reviewed atomically) even if they depend upon each other (i.e. are a part of a stack). What prevents a situation like CL2 getting approved and merged prior to the resolution of CL1? Or more generally, what ensures that the ordering of commits in a set of CLs in a given stack is preserved prior to merging? > I can't express just how badly they deal at handling reviews of stacked PRs - either you create your stacked PR targeting master (and it gets all of the changes from the underlying PRs merged into its changes list, which makes reviewing it harder) or you target it at the branch of the underlying PR (which is non-obvious and painful and you have to manually remember to shift it across when the PR beneath it gets merged) Rather than stacked PRs (which essentially doubles the number of commits since Github (and possibly Gitlab) introduces a merge commit for each PR even if it's a fast-forward merge), would it not be better to just combine CL1 through CL4 into a single feature branch where each commit corresponds to a CL? It would make for a large PR, but it can be reviewed on a per commit basis.
Quekid5 6y ago
Yes, stacked reviews absolutely are a game-changer for a sane commit history and iterating patches over a few rounds of review.
u801e 6y ago
At my place of work, we've sort of implemented that feature by using fixup or squash commits. That is, if a comment is made on a diff line in a PR, use git blame to determine which commit was responsible for that line of code, update the line and commit it with a commit title like: fixup! <original commit title>. If we want to update the commit message, we use "squash! <original commit title>" and put the updated commit message in the commit message body (using the --allow-empty switch when committing so that we don't have to change any code to make a commit). Then, we can run git rebase -i --autosquash --keep-empty to handle applying the changes requested to the correct commits at the end of the review process.
ericyu3 6y ago
So a few months ago I actually found a tool for this! The tool makes it easy to manage stacked reviews on GitHub. It's working super well for me. I also showed it to a bunch of people I work with, and a lot of them have taken it up. Link: https://github.com/ezyang/ghstack https://github.com/ezyang/ghstack If you're interested, I'm happy to talk you through it - just book some time here: https://calendly.com/ericyu3/15min https://calendly.com/ericyu3/15min

selestify 6y ago

What are stacked reviews?

erincandescent 6y ago
You have a task to build a Thing, which involves creating the basic "Thing framework" and then implementing subfeatures A, B and C of into/on top of it Maybe you'll build the framework & subfeature A together, because you need some bit of meat in there to properly figure things out and be able to test things On Gerrit you'd probably create two commits which get pushed as two CLs * CL1 Create Thing framework * CL2 Implement A for Thing They're now pushed for people to review. While they're looking at them, you continue working on, making CL3 implementing B * CL3 Implement B for Thing One of your coworkers points out an issue in CL1, so you fix it (by amending the commit) and repush the stack. Now your stack is * CL1 Create Thing framework [v2] * CL2 Implement A for Thing * CL3 Implement B for Thing CL1 & CL2 get approved so you merge them (though typically with Gerrit they're cherry picked - CLs are individual Git commits). You push up an implementation of CL4, so your stack now looks like * CL3 Implement B for Thing * CL3 Implement C for Thing The important point is that CLs are atomic (and are reviewed atomically) even if they depend upon each other (i.e. are a part of a stack). When you're working in Git you typically (unless for whatever reason you have multiple unrelated stacks on the go, which is relatively rare) just work off of the master branch, so all the commits between `origin/master` and `master` - i.e. the set that automatically show up in `git rebase -i` and similar tooling - are your stack. When you pull, you `git pull --rebase` (or rather set the config for your machine or repo to default to that). When you've revised something in your stack or want to add something to it, you just do "git push HEAD:origin/refs/for/master" update Gerrit with the latest version of your stack It takes a little time to get used to (and you have to learn how to hold `git rebase -i` properly), but when you're used to it it's immensely more productive than the Github/clones PR flow (and doesn't involve manual branch juggling, etc). I can't express just how badly they deal at handling reviews of stacked PRs - either you create your stacked PR targeting master (and it gets all of the changes from the underlying PRs merged into its changes list, which makes reviewing it harder) or you target it at the branch of the underlying PR (which is non-obvious and painful and you have to manually remember to shift it across when the PR beneath it gets merged) When I'm in this situation when dealing with something which is PR based, I tend to end up merging CL1 & CL2 (because its easier to review things when you have an example) and hang on to CL3 & CL4 on my local machine until the first one gets merged
- u801e 6y ago
  > The important point is that CLs are atomic (and are reviewed atomically) even if they depend upon each other (i.e. are a part of a stack). What prevents a situation like CL2 getting approved and merged prior to the resolution of CL1? Or more generally, what ensures that the ordering of commits in a set of CLs in a given stack is preserved prior to merging? > I can't express just how badly they deal at handling reviews of stacked PRs - either you create your stacked PR targeting master (and it gets all of the changes from the underlying PRs merged into its changes list, which makes reviewing it harder) or you target it at the branch of the underlying PR (which is non-obvious and painful and you have to manually remember to shift it across when the PR beneath it gets merged) Rather than stacked PRs (which essentially doubles the number of commits since Github (and possibly Gitlab) introduces a merge commit for each PR even if it's a fast-forward merge), would it not be better to just combine CL1 through CL4 into a single feature branch where each commit corresponds to a CL? It would make for a large PR, but it can be reviewed on a per commit basis.

Quekid5 6y ago

Yes, stacked reviews absolutely are a game-changer for a sane commit history and iterating patches over a few rounds of review.

u801e 6y ago

At my place of work, we've sort of implemented that feature by using fixup or squash commits. That is, if a comment is made on a diff line in a PR, use git blame to determine which commit was responsible for that line of code, update the line and commit it with a commit title like: fixup! <original commit title>. If we want to update the commit message, we use "squash! <original commit title>" and put the updated commit message in the commit message body (using the --allow-empty switch when committing so that we don't have to change any code to make a commit). Then, we can run git rebase -i --autosquash --keep-empty to handle applying the changes requested to the correct commits at the end of the review process.

ericyu3 6y ago

So a few months ago I actually found a tool for this! The tool makes it easy to manage stacked reviews on GitHub. It's working super well for me. I also showed it to a bunch of people I work with, and a lot of them have taken it up. Link: https://github.com/ezyang/ghstack https://github.com/ezyang/ghstack If you're interested, I'm happy to talk you through it - just book some time here: https://calendly.com/ericyu3/15min https://calendly.com/ericyu3/15min