Code Review Strategy Before Merging to Master from Feature Branches

branchingcode-reviewsgit

Me and my team use feature branches (with git). I'm wondering which is the best strategy for code review before merge to master.

  1. I checkout a new branch from master, lets call it fb_#1
  2. I commit a few times and than want to merge it back to master
  3. Before I merge someone is supposed to make a code review

Now there are 2 possibilities:

1st

  1. I merge master to fb_#1 (not fb_#1 to master) to make it as up-to-date as possible
  2. A teammate reviews changes between master head and fb_#1 head
  3. If fb_#1 is ok we merge fb_#1 to master
  4. Pros: no obsolete code in review
  5. Cons: if someone else merges something between "1." and "2." his changes appear in the review, though they belong to another review.

2nd

  1. A teammate reviews changes between the point of checkout (git merge-base master fb_#1) and fb_#1 head
  2. Pros: we see exactly what was changed during working on the feature branch
  3. Cons: some obsolete code may appear in the review.

Which way do you think is better and why? Maybe there is another more suitable approach?

Best Answer

There's a variation of your 1st option:

  1. merge master to fb_#1 (not fb_#1 to master) to make it as up-to-date as possible
  2. A teammate reviews changes between master at the point you merged and fb_#1 head
  3. If fb_#1 is ok we merge fb_#1 to master
  4. quick check that the merge is ok

eg.

... ma -- ... -- mm -- ... -- mf  <- master
      \            \         /
       f1 ... fn -- fm -----      <-- fb_#1

where:

  • ma is the ancestor of master and fb_#1.
  • fn is the last change on your branch
  • mm is the commit that was master/HEAD at the time you merged onto your branch (giving fm).

So, you compare mm and fm in your initial review, and then quickly check after merging back mf to make sure nothing significant changed on master during steps 1-3. This seems to have all of the pros and none of the cons for the initial review.

This assumes the review is quick compared to the normal frequency of changes pushed to master, so fm -> mf would often be a fast-forward.

If that is not the case, for whatever reason, the cons will just move from the initial review to the post-merge review, and it may be simpler just to merge directly onto master and do a single review there.

Related Topic