loading...

Do Stacked-PRs require re-review after merge?

jlouzado profile image Joel Louzado ・1 min read

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 master and create branch feat/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 branch feat/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

Posted on by:

Discussion

markdown guide
 

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/part1 as 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 master for feat/part2.

 

I understand you don't use a development branch, and push straight to master

I feel like this problem would occur even if there was a develop or other intermediate branch, but I take your point that the fact that this is going into master raises the stakes somewhat.

...essentially making it difficult to make real changes (if some would come up in PR-1).

This is true, changes in PR-1 would need to be propagated to the "downstream" branches manually.

If you insist on going about your way...

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.

In my opinion the PR must be revalidated (maybe more briefly), and must pass your test suite again.

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)?

...and then branching off from master for feat/part2.

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.

 

This is true, changes in PR-1 would need to be propagated to the "downstream" branches manually.

Sometimes part1 might just be a refactor, and part2 is the actual feature...

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 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.

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.

In an ideal case I would prefer this, but sometimes there's pressure on time...

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).

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 that case it a much safer merge. I would still give it a quick look before merging, but it's passable.

In that case it a much safer merge.

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.