Strategy for code review before merge to master from feature branches

Question

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?

Answer 1

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.

Answer 2

3rd

• You rebase the branch on master to both make it up-to-date and keep the changes separate.

  This creates a new history of the branch. It will be new revisions with new IDs that will have the same content, but will be derived from latest master and will not link to the old revisions. The old revisions are still accessible in "reflog" if you need to refer to them e.g. because you find you made mistake in conflict resolution. Besides that they are worthless. Git by default prunes the reflog after 3 months and discards the old revisions.

• You use interactive rebase (git rebase -i and git commit --amend) to reorder, edit and clean up the changes so each does a logically closed change.

  This again creates new history, this time with the added benefit that you can restructure the changes to make most sense during review.

• Pros:

  • no obsolete code in review
  • we see exactly what was changed during working on the feature branch
• Cons:
  • a bit more work
  • you have to take care not to rebase anything that is already merged or that you shared and the recipient does not expect it to be rewound.

Usually the extra work means you review the code thoroughly yourself first and that will catch many problems too.

This is what Linux and Git do. And it's not unusual to see series of 20 to 25 patches being submitted for review and rewritten several times over in those projects.

Actually Linux did it from early on in the project when their version control of choice was tarballs and patches. When many years later Linus set out to create git, it was the prime reason for implementing the rebase command and it's interactive variant. Also because of it git has separate notion of author and committer. Author is who first created the revision and committer is who last touched it. Since in both Linux and Git the patches are still submitted by email, the two are almost never the same person.

Answer 3

There is actually a third possbility—and most probably plenty of others, since GIT is more an implementation of a SCM framework than an implementation of a SCM methodology. This third possibility is based on rebase.

The rebase GIT subcommand takes a series of commit (typically from your branching point to the tip of your topic branch topic) and replay them somewhere else (typically at the tip of your integration branch, e.g. master). The rebase subcommand produces new commits, which gives the opportunity of rearranging commits in a form that is easier to review. This yields a new series of commit, similar to what topic used to be but appearing rooted at the top of the integration branch. This new branch is still called topic by GIT, so that the old reference is discarded. I informally label topic-0 the original state of your branch and topic-1 and so on its various refactoring.

Here is my suggestion for your topic branch:

1. (Optional step) You interactively rebase your topic branch topic on its branching point (see --fixup option for commit and the -i and --autosquash options on rebase), which gives you the opportunity to rewrite your commits in a way that is easier to review. This results in a branch topic-1.

2. You rebase your topic branch at the top of your integration branch, is similar to doing a merge, but “does not pollute” history with a merge that is merely a software engineering artifact. This results in a branch topic-2.

3. Send topic-2 to a teammate that reviews it against the tip of master.

4. If topic-2 is okay then merge it to master.

NOTE The branches—where branch refers to the commit tree—will all be called the same by GIT, thus, at the end of the process, only the branch topic-2 has a name in GIT.

Pros:

• No obsolete code in review.
• No spurious “foreign merges” reviews (the phenomenon you described in 1st).
• Opportunity to rewrite commits in a clean way.

Cons:

• Instead of one branch topic-0, there is three branches artifacts topic-0, topic-1 and topic-2 that are created in the commit tree. (Though at any time, only one of them has a name in GIT.)

In your 1st scenario «if someone merged something between "1." and "2."» refers to time spanning between the creation of the branch point and the time when you decide to merge. In this scenario «if someone merged something between "1." and "2."» refers to the time spanning between the rebase and the merge, which is usually very short. Thus in the scenario I provide, you can «lock» the master branch for the time of the merge without disturbing signigicantly your workflow, while it is impractical in the 1st scenario.

If you are doing systematic code reviews, it is probably a good idea to rearrange commits in an adequate way (optional step).

Managing the intermediate branch artifacts only presents a difficulty if you share them between repositories.