Scenario
- I'm working on a big feature that I know can be broken into two parts
- these parts are, as is often the case, dependent on each other.
- I branch off
masterand create branchfeat/part1 - I finish coding part1, and create a PR with this branch
-
feat/part1->master - this is
PR-1
-
- While I'm still checked out in
feat/part1, I create a new branchfeat/part2 - I finish coding part 2 and create a second, "stacked" PR
-
feat/part2->feat/part1. - this is
PR-2
-
Let's say both PRs get reviewed and approved. I now merge PR-2.
- PR-1 now contains all the changes.
Question
- Can I assume that PR-1 is now approved and merge-able or do I need to re-review it?
Background
- As for why someone would do this, this technique of breaking up PRs is a way to breakup large changes into more manageable chunks
- That way the rest of the team can give feedback more easily.
- Can read more about it here: Stacked PRs To Keep Github Diffs Small | graysonkoonce.com
Top comments (5)
I think that your way of breaking a feature apart is error prone. According to your example, I understand you don't use a development branch, and push straight to master - that would make your master branch the "synchronization branch" - i.e. the branch by which you sync your work with others on your team. In that case I think you should not use another branch, in this case
feat/part1as a middle branch.Another issue is that your PRs are actually approved in a reversed order - PR-1, which has laid the foundation of the feature must be approved and merged after PR-2, which is dependant on the foundation, essentially making it difficult to make real changes (if some would come up in PR-1).
If you insist on going about your way, I don't think you can assume PR-1 is merge-able automatically, or that if it had passed a code review, it is safe to assume that it is still OK. In my opinion the PR must be revalidated (maybe more briefly), and must pass your test suite again.
The way I suggest is to merge PR-1 to master (and maybe hide behind a feature flag), obviously with a code review and after having it pass all of your test suite, and then branching off from
masterforfeat/part2.I feel like this problem would occur even if there was a
developor other intermediate branch, but I take your point that the fact that this is going intomasterraises the stakes somewhat.This is true, changes in PR-1 would need to be propagated to the "downstream" branches manually.
So I'm not really married to this process, I was drawn to it because it makes the review-experience better for the rest of the team because I've done this additional planning to chunk up a big task into cognitively smaller pieces.
Assume that TDD is followed, and test-suite was passed for both PR-1, PR-2 and (PR-1 + PR-2). How would that impact your confidence in (PR-1 + PR-2)?
In an ideal case I would prefer this, but sometimes there's pressure on time and I might not have the luxury to wait for part1 to be merged before beginning work on part2.
Sometimes part1 might just be a refactor, and part2 is the actual feature in which case it becomes even more important to have a workflow that optimizes for throughput.
I've combined both statements because to me, they are both supportive of merging to master (or another shared branch). If you are working on a refactor, I'm sure other developers on your team would love to have it merged sooner, so that they may start using it.
I'm all for chunking up big tasks - it is really difficult to give a constructive review when you look at a diff that contains dozens of files.
Well, then I think you should talk to your team leader about it, and find a way to justify it. In my opinion, it might shorten the review period if all goes well, but if not the price will be higher (again - IMO).
In that case it a much safer merge. I would still give it a quick look before merging, but it's passable.
Yes, it would definitely be safer if TDD was followed but I was hoping there would be a general way to answer the question: Is the sum of two approved PRs also an approved PR?
The answer is probably some function of tests, thoroughness of review process and a few other factors (perhaps). Thoughts?
Like much of what we do, there isn't a single right answer here. It depends on your confidence in your code and your tests.
My general answer would be no -
PR-1 + PR-2 =/> PR-1 * PR-2(sum meaning both approved, multiplication meaning one merged into the other).But, that does not mean that under certain conditions (which I believe were met in your case), it can't be assumed.