CCL | Software | Download | Manuals | Forum | Papers
CCL Home

Research

Software Community Operations

Style for Integrating Git Pull Requests

Introduction

We have a heterogenous group of developers, with varying levels of proficiency on programming, and git usage. A git policy for our group should:
  • Allow people to contribute, regardless of their skill level, without compromising the quality of the code too much. Our first objective as a lab is to train people in computer science, thus we have to make allowances on code/repository quality if somebody is learning something.
  • Allow experienced people to use the workflow they are most comfortable with, without creating a big mess of a repository (a little mess is fine).
  • Encorage a clean enough main-line history, that makes easier to extend/debug code (specially of people not longer contributing), and that makes picking features for official releases less prone to errors.
Given the nature of the lab, it would be counter-productive to give hard rules on what accounts to be "little mess", or "passable code", and we have to trust the judgment of the members of the lab that are most experienced with both the code and git (at this moment, Prof. Thain, Patrick, and Ben). That said, here are some general guidelines when submitting a pull request:
  • Anybody can make a pull request.
  • The request should come from a feature branch, and not from master.
  • All things being equal, the feature branch should be rebased to the current main-line head before the request.
  • All things being equal, the lifetime of the branch ends when the pull request is accepted.
  • All things being equal, the branch should not contain any merge commits from master.
  • Ideally, the pull request message should thought as the message that will appear in the merge commit, and should summarize what the request is about.
Some general guidelines on when incorporating a pull request:
  • Only commiters may merge pull-requests. (Prof. Thain, Patrick, Dinesh, and Ben.)
  • Should a rebase be a better option, communicate with the original author about your plans. Either the author can do the rebase themselves, or at least they would know to look for changes.
  • The less experienced the person doing the request, the more the person doing the merge has to own the code.
  • If the original pull request message is informative, include it in the merge commit (Ben's example). Otherwise, please summarize the pull request (Dinesh's example).
Other general information:
  • Avoid commit/request messages that do not provide any context. The commit/request message should make clear which part of the code is being affected. (Bad: "Fail on disk space". Good: "wq_worker aborts when local disk space is below a threshold).
  • If commiting directly, without a pull request, please consider opening an issue before. You can then refer to the corresponding merge commit when closing the issue. This is very advantegeous when making releases, as it is easy to assign issues to milestones. (Ideally, we would be able to directly mark commits with milestones, but github does not allow that.)
Here are some examples of pull requests, and some guidelines for their solution:
Regular commit Pull request Base of the request Head of main-line master Previous head of main-line master Merge with no conflicts Merge with conflicts Solve conflicts pre-merge
Base of the pull request is the current head of master, more than one commit: encouraged
Merge: encouraged Solve conflicts, if any, before merge: encouraged
Merge commit with conflicts: discouraged

Pull request is a single commit: encouraged
Merge: encouraged Solve conflicts, if any, before merge: encouraged
Rebase/Fast-forward: if you must Merge commit with conflicts: discouraged

Base of the request is the behind head of master, more than one commit: if you must
Rebase to current master head / Merge: encouraged
Mege without conflicts: if you must Solve conflicts, if any, before merge: if you must
Rebase to current master head / Fast-forward: discoraged Merge commit with conflicts: discouraged

Request contains other merges: discouraged
Rebase to current master head, solve as above: encouraged
Solve conflicts, if any, before merge: if you must Merge commit with conflicts: discouraged