3 ms·

How do you guys handle managing multiple merge/pull requests that depend on the previous one? For example: Request 1: install dependencies Request 2: add sca

How do you guys handle managing multiple merge/pull requests that depend on the previous one? For example: Request 1: install dependencies Request 2: add scaffolding to application like adding a menu item, page layout, etc Request 3: add feature A Request 4: add feature B My questions: 1. Do you add all 4 Request reviews at once, or do you wait until the previous one has been approved before adding the next one? 2. How do you communicate Request 2 is dependent on Request 1? 3. How do you enable the reviewer to review Request 4 without reviewing duplicate code (since Request 4 has merged Request 1, 2, and 3)?

cbhl 6y ago
Ideally you want tooling that allows you to toggle between the diff between 2 and 3 and HEAD and 3, and to specify that 2 is the parent of 3. Some folks use "stacked diffs" or "stacked commits" to do this. Others use feature branches, or a patch series.
onorton 6y ago
To add to the sibling comments, you should avoid this scenario in the first place by hiding things behind feature flags that only apply in dev. Request 1 can go in fine on its own and reviewed separately Request 2 can be added feature flagged. Requests 3 and 4 would remove the respective flags for the scaffolding.
- nullsense 6y ago
  What about structural stuff that just can't be hidden behind a feature flag? What do people do in those scenarios?
eat_veggies 6y ago
This is a hard problem and I usually try to avoid it, but when I have to do it I usually have them branch off each other so that I can merge the PRs in reverse order once they're approved (4 -> 3 -> 2 -> 1 -> main). The diffs are all based off the previous branch in the "stack" so they aren't duplicated. It's really easy to mess up and merge the wrong PR first, and it's a pain in the ass to rebase all the way back up if you make a change in Request 1. I'd love to know how other people are handling this.
- jan_g 6y ago
  I'm using 'git rebase --onto one HEAD~<number of new commits in branch two>', repeat for 3 and 4.
leipert 6y ago
Create request-1 targeting the default branch, request-2 targeting request-1, etc. Once request-1 is merged, I'd change the target of request-2 to the default branch and merge the branch back. This is under the assumption that e.g. you can feature flag the feature away, so that e.g. the scaffolded app doesn't show in production yet. An alternative would be to have a "temporary" feature branch that you target until you feel it's ready to be merged into the default branch. By (temporarly) targeting the prev branch problem (3) is solved in most UIs. The communication problem (2) is simply done by: This adds X and is scoped to X, look at request-Y to see how feature Y builds on it. > Do you add all 4 Request reviews at once, or do you wait until the previous one has been approved before adding the next one? This is the tricky bit, in your example I wouldn't go so far and create all MRs. I'd create one MR ahead at maximum, because if you need to change something based on the review that influences a later feature, you might end up doing work twice. So maybe a timeline would look like this: - (Skipping a dependency MR, because I feel one should introduce dependency when it is used and not beforehand) - request-2: Scaffold application / feature, create an MR targeting default branch (request-2) - request-3: Start working on feature A, once feature A is 80% done => Create an MR targeting request-1 - Ask for review on request-2 with sharing a link to request-3 - Continue working on request-3 while you wait on request-2 review - Incorporate request-2 review feedback, get it merged - Rebase request-3 on merged request-2 (or merge default branch back), resolve conflicts and put it into review - Start working on request-4 based on the current request-2 branch. - Rinse and repeat. There are obvious intricacies, e.g. if legal needs to review dependencies, review times of your reviewers. I generally try to work with the same set of reviewers over a sensible time frame, rather than getting new ones for every MR relating to the same feature set. So check beforehand whether they are available for the foreseeable future.
q3k 6y ago
4 CRs stacked on eachother in Gerrit. It's that one killer feature of Gerrit that you cannot live without in other systems once you realize how powerful it is. Phabricator's Differential also does this, but it's comparatively extremely janky. You literally just push four stacked commits to refs/for/master, they end up as 4 separate Change Requests (equivalent to what others call Pull Requests, each with a separate review process), each one explicitly marked as being dependent on another, and showing a diff only against that particular other base CR as if it were merged onto the target branch already. Practically this just means you push your current HEAD to refs/for/master, Gerrit will take care of automatically creating all CRs for you and print out their URLs. Done. Ready for review. Then, to update any of them, just amend them locally (address code reviews, rebase stack on top of current master/main, ...), make sure they're still stacked on eachother in Git (as four commits), and push them all again to the same endpoint. Gerrit will automatically update all affected CRs. It will deduce the intended order and state of CRs based on the local Git history pushed, mapping all commits to existing CRs based on the Change-Id present in each commit (automatically generated by Gerrit's local git hook). None of the hacks that other siblings comments mention are required. It just works.
- zamalek 6y ago
  I imagine Pijul could do this quite naturally, as it describes everything as causal chains/DAGs (as I understand it). E.g. You could publish this[1] patch alongside all of its dependencies, and the UI could gate it behind it's dependencies. [1]: https://nest.pijul.com/pijul/pijul/changes/VO5OQW4W2656DIYYRNZ3PO7TQ4JOKQ3GVWE5ALUTYVMX3WMXJOYQC https://nest.pijul.com/pijul/pijul/changes/VO5OQW4W2656DIYYR...
  - pmeunier 6y ago
    In the entire history of Pijul, you probably picked the best example of how to get your reviewer stop being in love with you. That change fixed several hundred errors caused by the removal of a dependency.
    - zamalek 6y ago
jasonpeacock 6y ago
Use branches. Branches are cheap in Git, and you can do PRs between branches so that the reviewers are only seeing the new changes in that branch. In your example, I'd create a branch for R1, and publish a PR for R1 -> mainline when it's ready. Then I create a branch for R2 from the R1 branch, using R1 as the upstream. When R2 is ready, I publish a PR for R2 -> R1, and create a branch for R3 to start working on it. When there is feedback for R1, I switch to that branch and make those changes. I'll squash those changes back into the original commit(s) of R1, and update the PR. Then you rebase R2 with the changes you made from R1, rebase R3 with the changes from R2, etc. As long as the changes between each branch are not on the same lines of code, the rebases are painless and quick. I name the branches with a key to support ordering, something like: 1_add_dependencies 2_add_scaffolding 3_feature_A 4_feature_B This way they show up sorted when you list branches, and it's obvious what their dependency ordering is.

cbhl 6y ago

Ideally you want tooling that allows you to toggle between the diff between 2 and 3 and HEAD and 3, and to specify that 2 is the parent of 3. Some folks use "stacked diffs" or "stacked commits" to do this. Others use feature branches, or a patch series.

onorton 6y ago

To add to the sibling comments, you should avoid this scenario in the first place by hiding things behind feature flags that only apply in dev. Request 1 can go in fine on its own and reviewed separately Request 2 can be added feature flagged. Requests 3 and 4 would remove the respective flags for the scaffolding.

nullsense 6y ago
What about structural stuff that just can't be hidden behind a feature flag? What do people do in those scenarios?

eat_veggies 6y ago

This is a hard problem and I usually try to avoid it, but when I have to do it I usually have them branch off each other so that I can merge the PRs in reverse order once they're approved (4 -> 3 -> 2 -> 1 -> main). The diffs are all based off the previous branch in the "stack" so they aren't duplicated. It's really easy to mess up and merge the wrong PR first, and it's a pain in the ass to rebase all the way back up if you make a change in Request 1. I'd love to know how other people are handling this.

jan_g 6y ago
I'm using 'git rebase --onto one HEAD~<number of new commits in branch two>', repeat for 3 and 4.

leipert 6y ago

Create request-1 targeting the default branch, request-2 targeting request-1, etc. Once request-1 is merged, I'd change the target of request-2 to the default branch and merge the branch back. This is under the assumption that e.g. you can feature flag the feature away, so that e.g. the scaffolded app doesn't show in production yet. An alternative would be to have a "temporary" feature branch that you target until you feel it's ready to be merged into the default branch. By (temporarly) targeting the prev branch problem (3) is solved in most UIs. The communication problem (2) is simply done by: This adds X and is scoped to X, look at request-Y to see how feature Y builds on it. > Do you add all 4 Request reviews at once, or do you wait until the previous one has been approved before adding the next one? This is the tricky bit, in your example I wouldn't go so far and create all MRs. I'd create one MR ahead at maximum, because if you need to change something based on the review that influences a later feature, you might end up doing work twice. So maybe a timeline would look like this: - (Skipping a dependency MR, because I feel one should introduce dependency when it is used and not beforehand) - request-2: Scaffold application / feature, create an MR targeting default branch (request-2) - request-3: Start working on feature A, once feature A is 80% done => Create an MR targeting request-1 - Ask for review on request-2 with sharing a link to request-3 - Continue working on request-3 while you wait on request-2 review - Incorporate request-2 review feedback, get it merged - Rebase request-3 on merged request-2 (or merge default branch back), resolve conflicts and put it into review - Start working on request-4 based on the current request-2 branch. - Rinse and repeat. There are obvious intricacies, e.g. if legal needs to review dependencies, review times of your reviewers. I generally try to work with the same set of reviewers over a sensible time frame, rather than getting new ones for every MR relating to the same feature set. So check beforehand whether they are available for the foreseeable future.

q3k 6y ago

4 CRs stacked on eachother in Gerrit. It's that one killer feature of Gerrit that you cannot live without in other systems once you realize how powerful it is. Phabricator's Differential also does this, but it's comparatively extremely janky. You literally just push four stacked commits to refs/for/master, they end up as 4 separate Change Requests (equivalent to what others call Pull Requests, each with a separate review process), each one explicitly marked as being dependent on another, and showing a diff only against that particular other base CR as if it were merged onto the target branch already. Practically this just means you push your current HEAD to refs/for/master, Gerrit will take care of automatically creating all CRs for you and print out their URLs. Done. Ready for review. Then, to update any of them, just amend them locally (address code reviews, rebase stack on top of current master/main, ...), make sure they're still stacked on eachother in Git (as four commits), and push them all again to the same endpoint. Gerrit will automatically update all affected CRs. It will deduce the intended order and state of CRs based on the local Git history pushed, mapping all commits to existing CRs based on the Change-Id present in each commit (automatically generated by Gerrit's local git hook). None of the hacks that other siblings comments mention are required. It just works.

zamalek 6y ago
I imagine Pijul could do this quite naturally, as it describes everything as causal chains/DAGs (as I understand it). E.g. You could publish this[1] patch alongside all of its dependencies, and the UI could gate it behind it's dependencies. [1]: https://nest.pijul.com/pijul/pijul/changes/VO5OQW4W2656DIYYRNZ3PO7TQ4JOKQ3GVWE5ALUTYVMX3WMXJOYQC https://nest.pijul.com/pijul/pijul/changes/VO5OQW4W2656DIYYR...
- pmeunier 6y ago
  In the entire history of Pijul, you probably picked the best example of how to get your reviewer stop being in love with you. That change fixed several hundred errors caused by the removal of a dependency.
  - zamalek 6y ago

jasonpeacock 6y ago

Use branches. Branches are cheap in Git, and you can do PRs between branches so that the reviewers are only seeing the new changes in that branch. In your example, I'd create a branch for R1, and publish a PR for R1 -> mainline when it's ready. Then I create a branch for R2 from the R1 branch, using R1 as the upstream. When R2 is ready, I publish a PR for R2 -> R1, and create a branch for R3 to start working on it. When there is feedback for R1, I switch to that branch and make those changes. I'll squash those changes back into the original commit(s) of R1, and update the PR. Then you rebase R2 with the changes you made from R1, rebase R3 with the changes from R2, etc. As long as the changes between each branch are not on the same lines of code, the rebases are painless and quick. I name the branches with a key to support ordering, something like: 1_add_dependencies 2_add_scaffolding 3_feature_A 4_feature_B This way they show up sorted when you list branches, and it's obvious what their dependency ordering is.