3 ms·
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 y
by 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.