1040173 - Reject merge commits

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Gregory Szorc [:gps]]

An IRC conversation pondered how we should handle merge commits in ReviewBoard.

The general consensus seems to be that we should refuse to review merge commits.

The primary rationale being that we don't introduce user-generated merge commits in the Firefox repo. A secondary rationale is that merge commits are just hard to deal with. They can produce a massive diff. Hopefully those changes come from the merge itself, but other changes can creep in and it's nearly impossible to identify those.

A drawback is that some workflows (people accustomed to the Git/GitHub workflow) may involve lots of merges. If this becomes an issue, perhaps we can establish per-repo conventions.

For now, let's just refuse to review commit series with merge commits.

Comment 1

[Gregory Szorc [:gps]]

Attachment: MozReview Request: bz://1040173/gps

Comment 2

[Gregory Szorc [:gps]]

/r/292 - reviewboard: reject merge commits (bug 1040173)

Pull down this commit:

hg pull review -r 0994ef832da1e0021557158e6eddfc77f6027c20

Comment 3

[Mark Côté [:mcote]]

https://reviewboard-dev.allizom.org/r/291/#review213

Ship It!

Comment 4

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/76e1f0c44f90

Comment 5

[Gregory Szorc [:gps]]

Will leave review open in case smacleod cares to post review.

Comment 6

[Ed Morley [:emorley]]

(In reply to Gregory Szorc [:gps] from comment #0)
> A drawback is that some workflows (people accustomed to the Git/GitHub
> workflow) may involve lots of merges. 

I would count this as a bonus, rather than a drawback - but I'm not a fan of the Github excessive merge commit approach :-)

Comment 7

[Gregory Szorc [:gps]]

So it is stated here for posterity, there are technical hurdles to reviewing merges. Namely, how do you show a diff? Merges against old parents will have massive diffs. You can cheat and prune out files whose end state matches both parents (un-merged files). But then if merged files contain lots of changes, you are staring at a massive diff and the reviewer doesn't have an easy way of knowing what code was changed as part of the patch series or what code received merge resolution help.

Exposing unclear diffs flies in the face of the objective of a code review tool and since we can't produce clear diffs with merge commits, we disallow them.

Furthermore, I believe that for a project the size of Firefox that merge commits are a really bad idea. I'm on a crusade to eliminate merge commits from the Firefox repository. But this will require autoland and probably another year or two before the automation is in a state where this is viable.

Comment 8

[Steven MacLeod [:smacleod]]

https://reviewboard-dev.allizom.org/r/291/#review218

Looks fine to me, feel free to close this now.