Closed Bug 1040173 Opened 7 years ago Closed 7 years ago

Reject merge commits

Categories

(MozReview Graveyard :: General, defect)

Production
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: gps, Assigned: gps)

Details

Attachments

(1 file)

42 bytes, text/x-review-board-request
smacleod
: review+
mcote
: review+
Details
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.
Product: bugzilla.mozilla.org → Developer Services
Attachment #8509772 - Flags: review?(smacleod)
/r/292 - reviewboard: reject merge commits (bug 1040173)

Pull down this commit:

hg pull review -r 0994ef832da1e0021557158e6eddfc77f6027c20
Assignee: nobody → gps
Status: NEW → ASSIGNED
Attachment #8509772 - Flags: review+
Will leave review open in case smacleod cares to post review.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
(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 :-)
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.
Attachment #8509772 - Flags: review?(smacleod) → review+
https://reviewboard-dev.allizom.org/r/291/#review218

Looks fine to me, feel free to close this now.
Product: Developer Services → MozReview
You need to log in before you can comment on or make changes to this bug.