Difference between revisions of "Committing Guidelines"

Revision as of 14:23, 7 March 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!)

Environment Configuration

Okay, before you commit, please setup your environment as follows. First, create a file like this:

   [dw @ DreamwidthStaging - ~/current/cvs/dw-free] -> cat ~/.hgrc
   [ui]
   username = mark

The file is basically an INI file, if you're familiar with those. "[ui]" as a line, then your username. This 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 so you don't have to type the username in constantly.

   [dw @ DreamwidthStaging - ~/current/cvs/dw-free] -> cat ~/.ssh/config
   Host hg.dwscoalition.org
     User hg

If you don't do this, you can use "hg push ssh://hg@hg.dwscoalition.org/" if you want. Really up to you.

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 [[Dev Reviewing Guidelines|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.

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.)

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.

First, make sure you have pulled the latest version, so that you're committing to the tip. This will prevent messy merges later:

   $ hg pull

Then the actual commit:

   $ 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 addremove 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.

If you change that last line to "Patch by dreamwidth_username." exactly (note the period at the end), it will link to their account on Dreamwidth.

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

Note that your push might fail with an error saying "ssl required." If this is the case, you can tell it where to push, like so:

   $ hg push -r 105:9f00d0e0ebba ssh://hg.dwscoalition.org/dw-free

Alternately (and what I do) is just remove the cvs/dw-free that exists and get a new one:

   $ hg clone ssh://hg.dwscoalition.org/dw-free cvs/dw-free

Then, all future pulls/pushes will Just Work without manually specifying where to push it to.

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.

Avoiding Merge Problems

If push ever tells you it's going to create extra heads, that's a bad sign. You can generally fix it by doing

   $ hg rollback
   $ hg revert --all
   $ hg pull
   $ (reapply your patch)
   $ hg commit
   $ hg push -r REVISION

The rollback will undo your last change (the commit), but it will not erase the changed files. So if you rollback, then do hg status or hg diff, it will look exactly like it did before you committed.

hg revert then removes all the changes you've done, to prevent any conflicts when running pulling in the latest changes from the repo, so make sure you have a patch of your changes before running this set of commands. You can grab the patch again from Bugzilla. You can also generate a patch from your own repo by doing

   $ hg diff > ~/.tmp.patch

Commit Tool

Please DO NOT use this as is. This is just an example of one tool that might be helpful for commit. You'll see this is not full featured, only does a few things, but gives you an idea.

#!/usr/bin/perl
 
use strict;
use Getopt::Long;
 
my ( $repo, $bugid, $author, $msg );
GetOptions(
        'repo=s' => \$repo,
        'bugid=i' => \$bugid,
        'author=s' => \$author,
        'message=s' => \$msg,
    ) or usage();
 
usage() unless $repo || $bugid || $author || $msg;
 
$repo ||= 'dw-free';
die "repo not found\n" unless -d "$ENV{LJHOME}/cvs/$repo";
die "bugid invalid\n" unless $bugid =~ /^\d*$/;
die "no message given\n" unless $msg;
 
my $auth = get_author($author);
die "author invalid\n" unless $auth;
 
open FILE, ">/tmp/commit" or die;
if ( $bugid > 0 ) {
    print FILE "http://bugs.dwscoalition.org/show_bug.cgi?id=$bugid\n\n";
}
print FILE "$msg\n\n";
print FILE "Patch by $auth.\n";
 
chdir("$ENV{LJHOME}/cvs/$repo");
system("/usr/bin/hg addremove");
system("/usr/bin/hg commit -l /tmp/commit");
system("hg log -l 1 | grep change | awk '{ print $2 }' | cut -d : -f 2 | xargs hg push -r");
 
sub get_author {
    return {
            mark => 'Mark Smith <mark@dreamwidth.org>',
            exor => 'Andrea Nall <anall@andreanall.com>',
            pau => 'Pau Amma <pauamma@cpan.org>',
            janine => 'Janine Costanzo <janine@netrophic.com>',
            afuna => 'Afuna <coder.dw@afunamatata.com>',
            jd => 'JD <gameboyguy13@gmail.com>',
            sophie => 'Sophie <dw-bugzilla@theblob.org>',
            denise => 'Denise Paolucci <denise@dreamwidth.org>',
        }->{shift()};
}
 
sub usage {
    die <<EOF;
$0 - commit helper
 
usage:
 
  $0 -r dw-free -b 195 -a mark -m "Some message"
 
You may omit -r and we assume dw-free, if you omit a message we will open
up VIM to ask for one.  If you omit the bug, we skip that.  If you omit the
author, we cry a bit inside.
 
We lied about launching VIM, just give us a message.
EOF
}