4 ms·

How does this workflow work when there are 3 commits that need to be applied in order (A, B, and C), when C gets approved from a code-review standpoint before A

How does this workflow work when there are 3 commits that need to be applied in order (A, B, and C), when C gets approved from a code-review standpoint before A and B?

dom96 8y ago
You are prevented from merging commits at the top of the stack until the commits below are accepted. The advantage is that each commit can be reviewed separately. You cannot achieve this with pulled requests as far as I know. I often see people creating a pull request and writing "this depends on PR #1233", the second PR is unreviewable because it contains changes from. The first PR. So the first PR has to merged first... It's a mess. But maybe I am just missing a big part of the PR workflow that solves this problem.
- geofft 8y ago
  One way I've solved this is to open the second PR against the branch for the first PR, and explain what's going on in the PR message. Then, once the first PR is merged, update the second PR to be against the real target branch. This works better with a system that requires the PR owner to actually hit merge (so it's not accidentally merged into the first PR's branch) and where approvals stick across PR changes (which is undesirable in general, though).
- WorldMaker 8y ago
  If the branches are properly merged so that the dependency is represented in the git DAG, most good PR tools just handle this correctly: if you merge the dependent PR first the dependee updates to show the commits as already merged and "simplify" their diff views appropriately; if you merge the dependee PR first, the dependent PR simply gets marked as completed/merged for you. I find it is still useful to include helpful hints like "this PR includes all of PR #1234, if you prefer to review a piece at a time", but that's for the reviewers' sake, not the tools'.
  - dom96 8y ago
    Yes, that's nice. But the problem is that these are still shown as separate PRs, GitHub for example doesn't tie these together unlike Phabricator.
    - WorldMaker 8y ago
      GitHub has enough information that it could surface that better. It's a useful feature request.

How does this workflow work when there are 3 commits that need to be applied in order (A, B, and C), when C gets approved from a code-review standpoint before A and B?

dom96 8y ago

You are prevented from merging commits at the top of the stack until the commits below are accepted. The advantage is that each commit can be reviewed separately. You cannot achieve this with pulled requests as far as I know. I often see people creating a pull request and writing "this depends on PR #1233", the second PR is unreviewable because it contains changes from. The first PR. So the first PR has to merged first... It's a mess. But maybe I am just missing a big part of the PR workflow that solves this problem.

geofft 8y ago
One way I've solved this is to open the second PR against the branch for the first PR, and explain what's going on in the PR message. Then, once the first PR is merged, update the second PR to be against the real target branch. This works better with a system that requires the PR owner to actually hit merge (so it's not accidentally merged into the first PR's branch) and where approvals stick across PR changes (which is undesirable in general, though).
WorldMaker 8y ago
If the branches are properly merged so that the dependency is represented in the git DAG, most good PR tools just handle this correctly: if you merge the dependent PR first the dependee updates to show the commits as already merged and "simplify" their diff views appropriately; if you merge the dependee PR first, the dependent PR simply gets marked as completed/merged for you. I find it is still useful to include helpful hints like "this PR includes all of PR #1234, if you prefer to review a piece at a time", but that's for the reviewers' sake, not the tools'.
- dom96 8y ago
  Yes, that's nice. But the problem is that these are still shown as separate PRs, GitHub for example doesn't tie these together unlike Phabricator.
  - WorldMaker 8y ago
    GitHub has enough information that it could surface that better. It's a useful feature request.