Difference between revisions of "Committing Guidelines"

From Dreamwidth Notes
Jump to: navigation, search
(add warning for hg revert)
m (added cat)
 
(16 intermediate revisions by 9 users not shown)
Line 1: Line 1:
−
(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 ==
 
== 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.
+
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:
 
In general, the responsibilities of someone with commit are to make sure that:
Line 13: Line 11:
 
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!)
 
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 ===
+
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.
−
 
+
−
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 Smith <mark@dreamwidth.org>
+
−
 
+
−
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 name as you use in Bugzilla, e.g., "pauamma <email@email.com>" or "Andrea Nall <email@email.com>".  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 ==
 
== 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!
+
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. (Which should be linked here...)
+
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.
 
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.
 
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.)
+
When in doubt, talk to <dwuser>Mark</dwuser> and/or <dwuser>Denise</dwuser> 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 ==
+
== Actually 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.
+
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.
  
−
First, make sure you have pulled the latest version, so that you're committing to the tip. This will prevent messy merges later:
+
Either way, you can point the submitter to [[Git How To#How_to_update_a_pull_request_after_it.27s_been_reviewed|How to update a pull request after it's been reviewed]] and have them take care of it.
−
    $ hg pull
+
  
−
Then the actual commit:
+
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.
−
    $ 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.
+
(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)
  
−
=== Pre-Commit ===
+
== Committing via the Commandline ==
  
−
My general process for applying patches goes something like this...
+
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.
  
−
# Find the patch I want to apply, it is going to be an attachment on Bugzilla.
+
=== Environment Configuration ===
−
# Get the attachment ID, and try to apply it
+
Before you commit, please setup your environment as follows. First, make sure you have your username set for your commits:
−
# Assuming it cleanly applies, validate the diff (spot-check review with cvsreport -d)
+
−
# If all looks good, copy over to repository (cvsreport -s)
+
−
# 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...)
+
    $ cd $LJHOME
 +
    $ git config user.name USERNAME
 +
    $ git config user.email USERNAME&#64;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.
  
−
FIXME: add notes about verifying CLA before commit...
+
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:
  
−
=== The Commit ===
+
    $ cd $LJHOME
 +
    $ git remote set-url dreamwidth git&#64;github.com:dreamwidth/dw-free.git
 +
    $ cd $LJHOME/ext/dw-nonfree
 +
    $ git remote set-url dreamwidth git&#64;github.com:dreamwidth/dw-nonfree.git
  
−
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.
+
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&#64;github.com format.
  
−
# hg addremove to add any new files
+
=== Working with Pull Requests Locally ===
−
# hg status to verify only files you expect to change are changing
+
See [https://help.github.com/articles/checking-out-pull-requests-locally Github's documentation].
−
# hg diff to look at the diff once more
+
−
# 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
+
−
 
+
−
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
+
−
}
+
  
 
[[Category: Development]]
 
[[Category: Development]]

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 [info]Mark and/or [info]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.

Retrieved from "http://wiki.dreamwidth.net/wiki/index.php?title=Committing_Guidelines&oldid=12342"