Difference between revisions of "Committing Guidelines"

Revision as of 07:55, 19 January 2009

(This page is just a brain dump for xb95 at present, it should be cleaned up, if someone feels up to the task...)

Basics of Committing

A few people have/are/will be given commit access. 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!)

What To Commit

Picking something to commit is generally pretty straightforward. Either you can troll Bugzilla for patches that are flagged "review+ commit?" and then spot-check and commit those, or you can look for "review?" patches and take on the review yourself. Obviously we need as many reviewers as we can get, so the latter is highly encouraged!

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. (Which should be linked here...)

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.

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.

Mechanics of Committing

Committing with Mercurial (heretofore called "hg") is pretty simple. The big change from some other RCS is that a commit is actually local only. The process of getting a change into the central repository is several short and easy steps.

   $ hg commit
   $ hg log -l 1
   $ hg push -r REVISION

Now, let's talk about the steps one by one so you have some more information about how each step is supposed to work.

Pre-Commit

My general process for applying patches goes something like this...

1. Find the patch I want to apply, it is going to be an attachment on Bugzilla.
2. Get the attachment ID, and try to apply it
3. Assuming it cleanly applies, validate the diff (spot-check review with cvsreport -d)
4. If all looks good, copy over to repository (cvsreport -s)
5. If any NEW files were added, copy those manually (cvsreport won't!) to the repository

I've automated a number of these steps in my own setup, but I don't want to pretty the scripts up for general consumption right now. (They assume a lot about setup, i.e., my LJHOME is not /home/dw, I use symlinks so I can have various versions hopping about...)

FIXME: add notes about verifying CLA before commit...

The Commit

Once you have the files copied over to your repository in the cvs/dw-free directory, then you can verify that the diff looks good.

1. hg add to add any new files
2. hg status to verify only files you expect to change are changing
3. hg diff to look at the diff once more
4. hg commit

You will then be prompted for a commit message. The format MUST be:

   http://bugs.dwscoalition.org/show_bug.cgi?id=XXXXX
   
   This thing does something really awesome.
   
   Patch by Some Contributor <email@email.com>.

The description doesn't have to be too hairy, since the bug link should always serve to get the curious person back to the Bugzilla installation to see what exactly is being changed/touched/etc.

You must must must must include attribution. This ensures that we can go back and verify who contributed lines of code!

The Push

Now I verify that everything committed correctly...

   $ hg log -l 1

If that looks correct, I then take the revision in question and push it up to the central repository.

   $ hg push -r REVISION

Finally, go mark the patch "review+ commit+" in Bugzilla, paste in the link to the pushed location so the person in Bugzilla can go see their commit and make sure it looks right, and call it good.

If it's a small change, you can now resolve the bug. If it's a big change that needs to be tested, marking the bug as "needs-testing?" is appropriate at this point, to indicate that someone should now come and test this again.