Difference between revisions of "Committing Guidelines"

Latest revision as of 19:22, 12 August 2015

Basics of Committing

A few people have/are/will be given commit access to the official Dreamwidth repositories. Generally it's an invite-only sort of thing, if the existing pool of committers decide that you are familiar enough with the code and generally produce quality patches, you will be offered a commit access.

In general, the responsibilities of someone with commit are to make sure that:

1. All code being committed has been reviewed, either by the committer or by a known reviewer.
2. All code being committed is either a bug fix for an existing feature, or if it's a new feature, the feature has been approved by the people that do feature approvals.
3. The committer must be completely comfortable with the code in question, and comfortable that it will not eat data, destroy lives, or harm the site.

It is worth remembering that these are guidelines. If possible, follow them. But realistically speaking, especially when we're up against an aggressive launch schedule, they may be bent in some cases. (Which we will hopefully make up for by having many beta testers and quality coders!)

In all instances below, "commit" is shorthand for "merge into the official Dreamwidth repositories", and is not simply about making a commit to your own personal repository.

What To Commit

Picking something to commit is generally pretty straightforward. Look at the pull request queue and see if there's anything that needs reviewing, or if it's been reviewed already and updated according to review, needs commit.Obviously we need as many reviewers as we can get, so we highly encouraged doing reviews!

Performing a quality review is outside the scope of this document. For now, it is assumed that if you review something, you follow the guidelines on how to do a good review.

You should make sure that you only commit things you are comfortable having your name on. If you commit something you are going to be one of the go-to people for any questions on "what exactly is that" and "why do we have that", and probably some others. Again, review the guidelines in the first section, specifically around 'feature requests' - please do not take liberties about what should or should not be committed.

Also, if needed, verify that the person who submitted the pull request has a CLA. This is pretty important! If they've had previous commits then you can assume that they do have a CLA. If they're new and you're not sure, ask <dw user>denise</dwuser>.

Bug fixes: okay. Go for it. Tweaking the way things work in the support system at the request of administrators/staff: sounds good. Adding phone posting without having a business audit, design audit, etc etc, that just causes more pain down the road and delays releases and all sorts of stuff.

When in doubt, talk to Mark and/or Denise about whether or not a patch meets all of the business requirements. (Hopefully by that point in the process, someone will have caught it if it doesn't. But it's always good to check again if you're not sure.)

Actually Committing

Committing is pretty simple: near the bottom of the pull request, just above the comment textbox, is a merge button. If it says you can merge, and you've reviewed the changes and confirmed they're good, then go ahead and merge. If it says that it's unable to merge, that's likely because of a merge conflict. The pull request is out of date either because it's old and lots of things have been merged since then and one of them has caused a conflict or, pretty rarely. because two pull requests are working in the same area and one of them has just been merged.

Either way, you can point the submitter to How to update a pull request after it's been reviewed and have them take care of it.

Merging the pull request automatically closes it. If the submitter has included "Fixes #xxx" in the correct format somewhere in their commit, the original issue will also automatically be closed. If they haven't, then please look go to the original issue and close that too.

(One exception: pull requests merged into a release branch won't automatically close the associated request. The automatic closing will happen when the pull request is merged into the develop branch)

Committing via the Commandline

This is no longer strictly necessary. Most of the time you can just use the pull request page. But sometimes you'll want to do something via the commandline: a tricky merge, some cleanup before commit, or most likely creating and handling release branches.

Environment Configuration

Before you commit, please setup your environment as follows. First, make sure you have your username set for your commits:

   $ cd $LJHOME
   $ git config user.name USERNAME
   $ git config user.email USERNAME@dreamwidth.org
   # repeat for dw-nonfree

This sets your username in your global git config file (usually in ~/.gitconfig). The email should be the same as your username on Dreamwidth. This helps us track who commits something.

The second thing you probably want to configure is for SSH. You probably already have the dreamwidth repositories cloned, but with a read-only URL. You'll need to change it to a read-write URL:

   $ cd $LJHOME
   $ git remote set-url dreamwidth git@github.com:dreamwidth/dw-free.git
   $ cd $LJHOME/ext/dw-nonfree
   $ git remote set-url dreamwidth git@github.com:dreamwidth/dw-nonfree.git

Note by afuna: I find it easier to have separate repositories -- one where I do my development work and one where I do anything that involves touching the repository directly (like creating new release branches, or merging them into develop). To do this, don't modify your development repository. Instead, clone the dw-free and dw-nonfree repositories into a new folder using the git@github.com format.

Working with Pull Requests Locally

See Github's documentation.