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.

• Andrzej Gis: How do I get "the point you merged"? Will "git merge-base master head" be ok, or will it show the initial branching point?
• Useless: Unless you deliberately update master after the merge, it will just be master.
• Andrzej Gis: Yes, but how to get that point if someone else updates it?
• Useless: When you're on the fb branch, use git show HEAD. Since that will be the merge commit fm, it will list both parents. So, you have the hash of mm. Alternatively, you can trivially see the parent in gitk or any other git browser

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.

• stijn: +1 also the OP didn't ask about the next steps, but to get your feature into master you could use a merge --no-ff which will clearly show that branch on master instead of the feature just disappearing in the rest of the commits
• Jan Hudec: @stijn: --no-ff has it's up and downsides. I personally find it more noise than anything. YMMV.
• stijn: yeah it's a matter of preference.. If master normally is clean and straight I don't mind large features having a separate 'bubble'. If master is a mess already, no-ff only makes it worse
• Andrzej Gis: Wish I could accept mode than one answer. A hybrid solution would be ideal. Using rebase and than compare with the branch point seems to be the best way to go.
• sixtyfootersdude: Second Con - I don't think you can do this if you have already shared your branch with anyone. When you rewrite history there will be inconsistencies.

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.

• Jan Hudec: There should be no topic-0, topic-1 and topic-2 branches. The second the rebase is complete, the previous version is irrelevant. So all there would be is topic@{1}, topic@{2}, topic@{yesterday}, topic@{3.days.ago} etc. to save your butt in case you find you screwed conflict resolution in the rebase.
• Michaël Le Barbier: The three branches exist, but have no name (no ref). Maybe I should emphasize this.
• Jan Hudec: The revisions exist and are pointed to by reflog entries. But as branches there is just one, topic. Because branch in git is just the name.
• Andrzej Gis: How does it save me from "foreign merges"? What if someone merges to master after I send topic-2 to a teammate and that the teammate reviews it against the tip of the master?
• Michaël Le Barbier: @JanHudec At any time, there is only one branch called topic in GIT, it is always one of the branches (a branch as in commit tree, not as in GIT reference) I labelled topic-0, topic-1, topic-2.