I would like to see a formal change to R-T-C too. I include the useful notes
that Cloudant follows for this model below. Obviously not all this text belongs
in the bylaws. I suggest it, or a variant of it, belongs on our wiki but is
referenced in the bylaws, which should also cover the rules around changing the
policy. That is, the bylaws should be able to remain static while the R-T-C
page might evolve over time.
---
We follows a R-T-C protocol for making code changes. Our reviews happen in the
form of Pull Requests on GitHub. Pull Requests (PRs) are a central point of
coordination around developing GitHub and as such we have a well established
policy for handling them. This document outlines the current policies and best
practices and each team member is expected to follow them.
Characteristics of a Good Pull Request
• A logical progression of commits
• Each commit is a single logical change
• If a commit message says "Also did XYZ while in this code"
that's a strong smell that it should be two commits
• Do not mix logic and style changes in a commit
• Ie, make whitespace changes in their own commit
• Feel free to commit often and haphazardly when writing code but use
tools to reorder and rewrite commits before issuing a Pull Request
• Learn how to do interactive rebase
• git add -p is also useful
• When responding to Pull Request comments use new commits to address
the comments so that the discussion is kept sane
• Before merging a Pull Request when all reviewers have given a thumbs
up rewrite the history of the PR and force push it to GitHub
• Pull Requests are a discussion. You don't have to take everything
every reviewer says as a command but you may be asked for a technical argument
on why you don't want to make suggested changes.
Merging Pull Requests
• We use a rough equivalent of Apache Voting system
• Generally a +1 is taken to mean the reviewer understands the change,
agrees with the principle behind it, and is confident that it does what is
claimed (and only what is claimed)
• Generally speaking, if one or two people +1 a change, feel free to
merge it. It's rare to merge without a single +1
• If someone suggests a change to a PR then that blocks the merge until
either the change is made or there's an agreement on why to not make the change
• If you suggest a change on a PR it can be helpful to illustrate the
level of care you have for the change
• Style changes are usually low priority (unless they're
egregiously bad violations) so things like "I disagree with this variable name"
shouldn't be merge-stopping issue unless there's a good reason (ie, don't use a
variable "count" to store an average)
• Algorithmic changes are generally more important
• Its also perfectly valid to suggest a change with a "Feel
free to ignore this" disclaimer. Gathering reactions to code in the discussion
is important even if we don't act on them
• Use the 'merge' button.
• Do not merge a branch onto itself.
• Never move a tag once made public, burn a version number if you have
to instead
• Don't delete the topic branch after merge
——
Proper Commit Messages
First, read http://tbaggery.com/2008/04/19/a-note-about-git-commit-messages.html
We pride ourselves on having valuable and useful commit messages. This means
that we adhere to a standard and make use of the various sections of a commit
message for tooling and other automated capturing of our development history.
Its important to remember that a Git commit message is one of the most
permanent things a developer will create in their day to day activities. If the
commit message is not useful then it is wasting precious opportunity for
conveying historical information about the project.
* Commit messages should reference JIRA tickets
* Bad commit messages are grounds for veto
—
On 19 Jul 2014, at 06:49, Joan Touzet <[email protected]> wrote:
> Jan says:
>
> ----- Original Message -----
> On CTR (commit-then-review): it’s a leftover from the cvs/svn days, in our
> git world, what was equivalent to commit in the old model is merge to master
> (and release branches) in ours. And for that we do distinctly follow
> review-then-commit (http://wiki.apache.org/couchdb/Merge_Procedure) to ensure
> to the highest degree that master and release branches are always in a
> shippable/stable state. Git, branches and our merge procedure make all this
> not much of a problem that it was in the old cvs/svn days. I wonder then, if
> we should reword the CTR section to reflect our situation? (Even if
> committing to, say, a feature branch, that then is reviewed before it is
> merged into master or a release branch could be seen as CTR, it is not quite
> capturing the same intent).
> --
>
> I generally agree here, but I'm going to leave this thread open for 24h
> before changing the text. The proposal would be to explicitly state that we
> follow a review-then-commit model using git branches and pull requests,
> generally following the GitHub Flow pattern
> (http://scottchacon.com/2011/08/31/github-flow.html), and that detail on the
> process is outside the scope of the Bylaws.
>
> -Joan
