Merge lp:~blair/ubuntu/precise/rbtools/rbtools-fix-886436 into lp:ubuntu/precise/rbtools

[Blair Zajac]

Hello,

This is a commit for updating rbtools to the latest upstream release.

This is my first work with bzr and committing into launchpad, so some questions and notes on the work:

1) The initial checkout of rbtools had a .pc directory with two files in it, applied-patches and fix_tarball. Since upstream's source tarball and Debian's source didn't have this, I deleted it as part of the commit.

2) I rsynced my copy of the build that successfully did a pbuilder run into the bzr checkout to make it identical.

3) I didn't bump the policy to the latest version given I'm not familiar with them. I did do a diff between 0.9.2 and 0.9.3 in git://git.debian.org/git/dbnpolicy/policy.git, but this didn't seem to show anything relevant to this package (in a cursory reading).

4) Given that Debian is at 0.2, should this package have "ubuntu" in the version number?

Thanks,
Blair

[Blair Zajac (blair)]

One other thing, I put in the changelog that this resolves a Debian bug for the same issue. Is that appropriate, given that's a separate step to get this change into Debian?

[Daniel Holbach (dholbach)]

I get an error message trying to merge this. Did you use "bzr merge-upstream"? (http://developer.ubuntu.com/packaging/html/udd-merging.html)

review: Needs Fixing

[Blair Zajac (blair)]

Hi Daniel,

What's the error message?

No, I didn't the instructions on that that page, didn't know it existed. I followed the instructions here:

https://wiki.ubuntu.com/PackagingGuide/Recipes/PackageUpdate
https://wiki.ubuntu.com/PackagingGuide/Recipes/UseBzrAndBzrBuildpackage

which suggests to manually copy the upstream tarball into the bzr checkout.

Blair

[Blair Zajac (blair)]

Could the merge issue be due to the fact that I branched from lp:ubuntu/precise/rbtools? What branch should I merge from?

[Blair Zajac (blair)]

Also, could you give me the branch you're merging into? Is it possible to attempt the merge in my own bzr checkout? That would help.

[Daniel Holbach (dholbach)]

This is the output I get when I try to review your branch:

daniel@daydream:~$ bzr branch lp:ubuntu/rbtools
Most recent Ubuntu version: 0.2-1
Packaging branch status: CURRENT
Branched 2 revision(s).
daniel@daydream:~$ cd rbtools/
daniel@daydream:~/rbtools$ bzr merge-package lp:~blair/ubuntu/precise/rbtools/rbtools-fix-886436
bzr: ERROR: None 0.3.4 was not found in <PristineTarSource at bzr+ssh://bazaar.launchpad.net/~blair/ubuntu/precise/rbtools/rbtools-fix-886436/>.
daniel@daydream:~/rbtools$

[Stéphane Graber (stgraber)]

Based on comments above, marking as work in progress, please switch back to needs review once the merge issue solved.

Thanks for your work!

Unmerged revisions

3. By Blair Zajac on 2011-11-06

* New upstream release. Closes: #613669, LP #886436.
* debian/watch: update URL.
* debian/patches: delete since upstream fixed missing files from
  tarball.

=== removed directory '.pc'
=== removed file '.pc/.version'
--- .pc/.version	2010-07-31 18:31:05 +0000
+++ .pc/.version	1970-01-01 00:00:00 +0000
@@ -1,1 +0,0 @@
-2

=== removed file '.pc/applied-patches'
--- .pc/applied-patches	2010-07-31 18:31:05 +0000
+++ .pc/applied-patches	1970-01-01 00:00:00 +0000
@@ -1,1 +0,0 @@
-fix_tarball

=== removed directory '.pc/fix_tarball'
=== removed file '.pc/fix_tarball/.reviewboardrc'
=== removed file '.pc/fix_tarball/COPYING'
=== removed file '.pc/fix_tarball/INSTALL'
=== removed file '.pc/fix_tarball/setup.cfg'
--- .pc/fix_tarball/setup.cfg	2010-07-31 18:31:05 +0000
+++ .pc/fix_tarball/setup.cfg	1970-01-01 00:00:00 +0000
@@ -1,16 +0,0 @@
-[egg_info]
-tag_build = 
-tag_date = 0
-tag_svn_revision = 0
-
-[aliases]
-alpha2 = egg_info -DRb alpha2
-alpha1 = egg_info -DRb alpha1
-rc1 = egg_info -DRb rc1
-rc2 = egg_info -DRb rc2
-nightly = egg_info -dR
-snapshot = egg_info -Dr
-beta2 = egg_info -DRb beta2
-beta1 = egg_info -DRb beta1
-release = egg_info -DRb ''
-

=== removed file '.reviewboardrc'
--- .reviewboardrc	2010-07-31 18:31:05 +0000
+++ .reviewboardrc	1970-01-01 00:00:00 +0000
@@ -1,1 +0,0 @@
-REVIEWBOARD_URL = "http://reviews.reviewboard.org"

=== modified file 'AUTHORS'
--- AUTHORS	2010-07-31 18:31:05 +0000
+++ AUTHORS	2011-11-06 07:43:24 +0000
@@ -6,13 +6,23 @@
 
 Contributors:
 
+	* Andrew Stitcher
 	* Anthony Cruz
+	* Ben Hollis
 	* Bryan Halter
 	* Chris Clark
 	* Dan Savilonis
 	* Dana Lacoste
+	* Daniel Cestari
+	* Daniel LaMotte
+	* David Gardner
+	* Dick Porter
 	* Eric Huss
 	* Flavio Castelli
+	* Gyula Faller
+	* Holden Karau
+	* Ian Monroe
+	* Jan Koprowski
 	* Jason Felice
 	* Jeremy Bettis
 	* Laurent Nicolas
@@ -21,14 +31,20 @@
 	* Luke Robison
 	* Matthew Woehlke
 	* Mike Crute
+	* Nathan Dimmock
 	* Nathan Heijermans
+	* Noah Kantrowitz
 	* Paul Scott
+	* Peter Ward
 	* Petr Novák
 	* Raghu Kaippully
 	* Ravi Kondamuru
 	* Ryan Oblak
+	* Ryan Shelley
+	* Severin Gehwolf
 	* Stacey Sheldon
 	* Stefan Ring
+	* Steven Ihde
 	* Steven Russell
 	* Thilo-Alexander Ginkel
 	* Tom Saeger

=== modified file 'MANIFEST.in'
--- MANIFEST.in	2010-07-31 18:31:05 +0000
+++ MANIFEST.in	2011-11-06 07:43:24 +0000
@@ -1,5 +1,7 @@
 recursive-include contrib *.py *.txt README*
 include ez_setup.py
 include AUTHORS
+include COPYING
+include INSTALL
 include NEWS
 include README

=== modified file 'PKG-INFO'
--- PKG-INFO	2010-07-31 18:31:05 +0000
+++ PKG-INFO	2011-11-06 07:43:24 +0000
@@ -1,12 +1,12 @@
 Metadata-Version: 1.0
 Name: RBTools
-Version: 0.2
+Version: 0.3.4
 Summary: Command line tools for use with Review Board
 Home-page: http://www.reviewboard.org/
 Author: Christian Hammond
 Author-email: [e-mail]
 License: MIT
-Download-URL: http://downloads.reviewboard.org/releases/RBTools/0.2/
+Download-URL: http://downloads.reviewboard.org/releases/RBTools/0.3/
 Description: UNKNOWN
 Platform: UNKNOWN
 Classifier: Development Status :: 4 - Beta

=== removed file 'RBTools.egg-info/PKG-INFO'
--- RBTools.egg-info/PKG-INFO	2010-07-31 18:31:05 +0000
+++ RBTools.egg-info/PKG-INFO	1970-01-01 00:00:00 +0000
@@ -1,18 +0,0 @@
-Metadata-Version: 1.0
-Name: RBTools
-Version: 0.2
-Summary: Command line tools for use with Review Board
-Home-page: http://www.reviewboard.org/
-Author: Christian Hammond
-Author-email: [e-mail]
-License: MIT
-Download-URL: http://downloads.reviewboard.org/releases/RBTools/0.2/
-Description: UNKNOWN
-Platform: UNKNOWN
-Classifier: Development Status :: 4 - Beta
-Classifier: Environment :: Console
-Classifier: Intended Audience :: Developers
-Classifier: License :: OSI Approved :: MIT License
-Classifier: Operating System :: OS Independent
-Classifier: Programming Language :: Python
-Classifier: Topic :: Software Development

=== removed file 'RBTools.egg-info/SOURCES.txt'
--- RBTools.egg-info/SOURCES.txt	2010-07-31 18:31:05 +0000
+++ RBTools.egg-info/SOURCES.txt	1970-01-01 00:00:00 +0000
@@ -1,18 +0,0 @@
-AUTHORS
-MANIFEST.in
-NEWS
-README
-ez_setup.py
-setup.cfg
-setup.py
-RBTools.egg-info/PKG-INFO
-RBTools.egg-info/SOURCES.txt
-RBTools.egg-info/dependency_links.txt
-RBTools.egg-info/entry_points.txt
-RBTools.egg-info/top_level.txt
-contrib/P4Tool.txt
-contrib/README.P4Tool
-contrib/internal/release.py
-rbtools/__init__.py
-rbtools/postreview.py
-rbtools/tests.py
\ No newline at end of file

=== removed file 'RBTools.egg-info/dependency_links.txt'
--- RBTools.egg-info/dependency_links.txt	2010-07-31 18:31:05 +0000
+++ RBTools.egg-info/dependency_links.txt	1970-01-01 00:00:00 +0000
@@ -1,1 +0,0 @@
-http://downloads.reviewboard.org/releases/RBTools/0.2/

=== removed file 'RBTools.egg-info/entry_points.txt'
--- RBTools.egg-info/entry_points.txt	2010-07-31 18:31:05 +0000
+++ RBTools.egg-info/entry_points.txt	1970-01-01 00:00:00 +0000
@@ -1,3 +0,0 @@
-[console_scripts]
-post-review = rbtools.postreview:main
-

=== removed file 'RBTools.egg-info/top_level.txt'
--- RBTools.egg-info/top_level.txt	2010-07-31 18:31:05 +0000
+++ RBTools.egg-info/top_level.txt	1970-01-01 00:00:00 +0000
@@ -1,1 +0,0 @@
-rbtools

=== modified file 'contrib/internal/release.py'
--- contrib/internal/release.py	2010-07-31 18:31:05 +0000
+++ contrib/internal/release.py	2011-11-06 07:43:24 +0000
@@ -4,17 +4,20 @@
 # developers with release permissions.
 #
 
+import hashlib
+import mimetools
 import os
-import re
 import shutil
+import subprocess
 import sys
 import tempfile
+import urllib2
 
 sys.path.insert(0, os.path.join(os.path.dirname(__file__), "..", ".."))
 from rbtools import __version__, __version_info__, is_release
 
 
-PY_VERSIONS = ["2.4", "2.5", "2.6"]
+PY_VERSIONS = ["2.4", "2.5", "2.6", "2.7"]
 
 LATEST_PY_VERSION = PY_VERSIONS[-1]
 
@@ -26,17 +29,65 @@
                                    __version_info__[0],
                                    __version_info__[1])
 
+RBWEBSITE_API_URL = 'http://www.reviewboard.org/api/'
+RELEASES_API_URL = '%sproducts/rbtools/releases/' % RBWEBSITE_API_URL
+
 
 built_files = []
 
 
+def load_config():
+    filename = os.path.join(os.path.expanduser('~'), '.rbwebsiterc')
+
+    if not os.path.exists(filename):
+        sys.stderr.write("A .rbwebsiterc file must exist in the form of:\n")
+        sys.stderr.write("\n")
+        sys.stderr.write("USERNAME = '<username>'\n")
+        sys.stderr.write("PASSWORD = '<password>'\n")
+        sys.exit(1)
+
+    user_config = {}
+
+    try:
+        execfile(filename, user_config)
+    except SyntaxError, e:
+        sys.stderr.write('Syntax error in config file: %s\n'
+                         'Line %i offset %i\n' % (filename, e.lineno, e.offset))
+        sys.exit(1)
+
+    auth_handler = urllib2.HTTPBasicAuthHandler()
+    auth_handler.add_password(realm='Web API',
+                              uri=RBWEBSITE_API_URL,
+                              user=user_config['USERNAME'],
+                              passwd=user_config['PASSWORD'])
+    opener = urllib2.build_opener(auth_handler)
+    urllib2.install_opener(opener)
+
+
 def execute(cmdline):
-    print ">>> %s" % cmdline
-
-    if os.system(cmdline) != 0:
+    if isinstance(cmdline, list):
+        print ">>> %s" % subprocess.list2cmdline(cmdline)
+    else:
+        print ">>> %s" % cmdline
+
+    p = subprocess.Popen(cmdline,
+                         shell=True,
+                         stdout=subprocess.PIPE)
+
+    s = ''
+
+    for data in p.stdout.readlines():
+        s += data
+        sys.stdout.write(data)
+
+    rc = p.wait()
+
+    if rc != 0:
         print "!!! Error invoking command."
         sys.exit(1)
 
+    return s
+
 
 def run_setup(target, pyver = LATEST_PY_VERSION):
     execute("python%s ./setup.py release %s" % (pyver, target))
@@ -62,6 +113,23 @@
                        (PACKAGE_NAME, __version__))
 
 
+def build_checksums():
+    sha_filename = 'dist/%s-%s.sha256sum' % (PACKAGE_NAME, __version__)
+    out_f = open(sha_filename, 'w')
+
+    for filename in built_files:
+        m = hashlib.sha256()
+
+        in_f = open(filename, 'r')
+        m.update(in_f.read())
+        in_f.close()
+
+        out_f.write('%s  %s\n' % (m.hexdigest(), os.path.basename(filename)))
+
+    out_f.close()
+    built_files.append(sha_filename)
+
+
 def upload_files():
     execute("scp %s %s" % (" ".join(built_files), RELEASES_URL))
 
@@ -71,7 +139,51 @@
 
 
 def register_release():
-    run_setup("register")
+    if __version_info__[4] == 'final':
+        run_setup("register")
+
+    scm_revision = execute(['git rev-parse', 'release-%s' % __version__])
+
+    data = {
+        'major_version': __version_info__[0],
+        'minor_version': __version_info__[1],
+        'micro_version': __version_info__[2],
+        'release_type': __version_info__[3],
+        'release_num': __version_info__[4],
+        'scm_revision': scm_revision,
+    }
+
+    boundary = mimetools.choose_boundary()
+    content = ''
+
+    for key, value in data.iteritems():
+        content += '--%s\r\n' % boundary
+        content += 'Content-Disposition: form-data; name="%s"\r\n' % key
+        content += '\r\n'
+        content += str(value) + '\r\n'
+
+    content += '--%s--\r\n' % boundary
+    content += '\r\n'
+
+    headers = {
+        'Content-Type': 'multipart/form-data; boundary=%s' % boundary,
+        'Content-Length': str(len(content)),
+    }
+
+    print 'Posting release to reviewboard.org'
+    try:
+        f = urllib2.urlopen(urllib2.Request(url=RELEASES_API_URL, data=content,
+                                            headers=headers))
+        f.read()
+    except urllib2.HTTPError, e:
+        print "Error uploading. Got HTTP code %d:" % e.code
+        print e.read()
+    except urllib2.URLError, e:
+        try:
+            print "Error uploading. Got URL error:" % e.code
+            print e.read()
+        except AttributeError:
+            pass
 
 
 def main():
@@ -80,6 +192,8 @@
                          "Djblets tree.\n")
         sys.exit(1)
 
+    load_config()
+
     if not is_release():
         sys.stderr.write('This has not been marked as a release in '
                          'rbtools/__init__.py\n')
@@ -89,6 +203,7 @@
     git_dir = clone_git_tree(cur_dir)
 
     build_targets()
+    build_checksums()
     upload_files()
 
     os.chdir(cur_dir)

=== modified file 'debian/changelog'
--- debian/changelog	2010-07-31 18:31:05 +0000
+++ debian/changelog	2011-11-06 07:43:24 +0000
@@ -1,3 +1,12 @@
+rbtools (0.3.4-1) precise; urgency=low
+
+  * New upstream release.  Closes: #613669, LP #886436.
+  * debian/watch: update URL.
+  * debian/patches: delete since upstream fixed missing files from
+    tarball.
+
+ -- Blair Zajac <[e-mail]>  Sun, 06 Nov 2011 00:16:52 -0700
+
 rbtools (0.2-1) unstable; urgency=low
 
   * Initial release. (Closes: #573485)

=== removed directory 'debian/patches'
=== removed file 'debian/patches/fix_tarball'
--- debian/patches/fix_tarball	2010-07-31 18:31:05 +0000
+++ debian/patches/fix_tarball	1970-01-01 00:00:00 +0000
@@ -1,73 +0,0 @@
-Description: Fix missing files in the tarball
- Some files that are missing from the tarball, but are included in
- the upstream release-0.2 branch.
-Author: Christian Hammond <[e-mail]>
-
----
-
---- /dev/null
-+++ rbtools-0.2/INSTALL
-@@ -0,0 +1,11 @@
-+Installation
-+============
-+
-+To install rbtools, simply run the following as root:
-+
-+    $ python setup.py install
-+
-+
-+Or to automatically download and install the latest version, you can run:
-+
-+    $ easy_install -U RBTools
---- rbtools-0.2.orig/setup.cfg
-+++ rbtools-0.2/setup.cfg
-@@ -1,16 +1,14 @@
- [egg_info]
--tag_build = 
--tag_date = 0
--tag_svn_revision = 0
-+tag_build = .dev
-+tag_svn_revision = 1
- 
- [aliases]
-+snapshot = egg_info -Dr
-+nightly = egg_info -dR
- alpha2 = egg_info -DRb alpha2
- alpha1 = egg_info -DRb alpha1
--rc1 = egg_info -DRb rc1
--rc2 = egg_info -DRb rc2
--nightly = egg_info -dR
--snapshot = egg_info -Dr
- beta2 = egg_info -DRb beta2
- beta1 = egg_info -DRb beta1
-+rc1 = egg_info -DRb rc1
-+rc2 = egg_info -DRb rc2
- release = egg_info -DRb ''
--
---- /dev/null
-+++ rbtools-0.2/.reviewboardrc
-@@ -0,0 +1 @@
-+REVIEWBOARD_URL = "http://reviews.reviewboard.org"
---- /dev/null
-+++ rbtools-0.2/COPYING
-@@ -0,0 +1,20 @@
-+Copyright (c) 2007-2010  Christian Hammond
-+Copyright (c) 2007-2010  David Trowbridge
-+
-+Permission is hereby granted, free of charge, to any person obtaining a copy of
-+this software and associated documentation files (the "Software"), to deal in
-+the Software without restriction, including without limitation the rights to
-+use, copy, modify, merge, publish, distribute, sublicense, and/or sell copies
-+of the Software, and to permit persons to whom the Software is furnished to do
-+so, subject to the following conditions:
-+
-+The above copyright notice and this permission notice shall be included in all
-+copies or substantial portions of the Software.
-+
-+THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
-+IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
-+FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE
-+AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
-+LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM,
-+OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
-+SOFTWARE.

=== removed file 'debian/patches/series'
--- debian/patches/series	2010-07-31 18:31:05 +0000
+++ debian/patches/series	1970-01-01 00:00:00 +0000
@@ -1,1 +0,0 @@
-fix_tarball

=== modified file 'debian/watch'
--- debian/watch	2010-07-31 18:31:05 +0000
+++ debian/watch	2011-11-06 07:43:24 +0000
@@ -1,5 +1,4 @@
 version=3
 
-# FIXME: Does not produce the latest
 opts="dversionmangle=s/~rc/rc/,uversionmangle=s/rc/~rc/" \
-  http://downloads.reviewboard.org/releases/RBTools/0.2/ RBTools-(.*)\.tar\.gz
+  http://downloads.reviewboard.org/releases/RBTools/0.3/ RBTools-(.*)\.tar\.gz

=== modified file 'ez_setup.py'
--- ez_setup.py	2010-07-31 18:31:05 +0000
+++ ez_setup.py	2011-11-06 07:43:24 +0000
@@ -14,7 +14,7 @@
 This file can also be run as a script to install or upgrade setuptools.
 """
 import sys
-DEFAULT_VERSION = "0.6c8"
+DEFAULT_VERSION = "0.6c11"
 DEFAULT_URL     = "http://pypi.python.org/packages/%s/s/setuptools/" % sys.version[:3]
 
 md5_data = {
@@ -28,6 +28,14 @@
     'setuptools-0.6b4-py2.4.egg': '4cb2a185d228dacffb2d17f103b3b1c4',
     'setuptools-0.6c1-py2.3.egg': 'b3f2b5539d65cb7f74ad79127f1a908c',
     'setuptools-0.6c1-py2.4.egg': 'b45adeda0667d2d2ffe14009364f2a4b',
+    'setuptools-0.6c10-py2.3.egg': 'ce1e2ab5d3a0256456d9fc13800a7090',
+    'setuptools-0.6c10-py2.4.egg': '57d6d9d6e9b80772c59a53a8433a5dd4',
+    'setuptools-0.6c10-py2.5.egg': 'de46ac8b1c97c895572e5e8596aeb8c7',
+    'setuptools-0.6c10-py2.6.egg': '58ea40aef06da02ce641495523a0b7f5',
+    'setuptools-0.6c11-py2.3.egg': '2baeac6e13d414a9d28e7ba5b5a596de',
+    'setuptools-0.6c11-py2.4.egg': 'bd639f9b0eac4c42497034dec2ec0c2b',
+    'setuptools-0.6c11-py2.5.egg': '64c94f3bf7a72a13ec83e0b24f2749b2',
+    'setuptools-0.6c11-py2.6.egg': 'bfa92100bd772d5a213eedd356d64086',
     'setuptools-0.6c2-py2.3.egg': 'f0064bf6aa2b7d0f3ba0b43f20817c27',
     'setuptools-0.6c2-py2.4.egg': '616192eec35f47e8ea16cd6a122b7277',
     'setuptools-0.6c3-py2.3.egg': 'f181fa125dfe85a259c9cd6f1d7b78fa',
@@ -48,13 +56,18 @@
     'setuptools-0.6c8-py2.3.egg': '50759d29b349db8cfd807ba8303f1902',
     'setuptools-0.6c8-py2.4.egg': 'cba38d74f7d483c06e9daa6070cce6de',
     'setuptools-0.6c8-py2.5.egg': '1721747ee329dc150590a58b3e1ac95b',
+    'setuptools-0.6c9-py2.3.egg': 'a83c4020414807b496e4cfbe08507c03',
+    'setuptools-0.6c9-py2.4.egg': '260a2be2e5388d66bdaee06abec6342a',
+    'setuptools-0.6c9-py2.5.egg': 'fe67c3e5a17b12c0e7c541b7ea43a8e6',
+    'setuptools-0.6c9-py2.6.egg': 'ca37b1ff16fa2ede6e19383e7b59245a',
 }
 
 import sys, os
+try: from hashlib import md5
+except ImportError: from md5 import md5
 
 def _validate_md5(egg_name, data):
     if egg_name in md5_data:
-        from md5 import md5
         digest = md5(data).hexdigest()
         if digest != md5_data[egg_name]:
             print >>sys.stderr, (
@@ -64,7 +77,6 @@
             sys.exit(2)
     return data
 
-
 def use_setuptools(
     version=DEFAULT_VERSION, download_base=DEFAULT_URL, to_dir=os.curdir,
     download_delay=15
@@ -100,11 +112,11 @@
             "\n\n(Currently using %r)"
             ) % (version, e.args[0])
             sys.exit(2)
-        else:
-            del pkg_resources, sys.modules['pkg_resources']    # reload ok
-            return do_download()
     except pkg_resources.DistributionNotFound:
-        return do_download()
+        pass
+
+    del pkg_resources, sys.modules['pkg_resources']    # reload ok
+    return do_download()
 
 def download_setuptools(
     version=DEFAULT_VERSION, download_base=DEFAULT_URL, to_dir=os.curdir,
@@ -233,7 +245,6 @@
     """Update our built-in md5 registry"""
 
     import re
-    from md5 import md5
 
     for name in filenames:
         base = os.path.basename(name)
@@ -270,3 +281,4 @@
 
 
 
+

=== modified file 'rbtools/__init__.py'
--- rbtools/__init__.py	2010-07-31 18:31:05 +0000
+++ rbtools/__init__.py	2011-11-06 07:43:24 +0000
@@ -31,7 +31,7 @@
 #
 #   (Major, Minor, Micro, alpha/beta/rc/final, Release Number, Released)
 #
-VERSION = (0, 2, 0, 'final', 0, True)
+VERSION = (0, 3, 4, 'final', 0, True)
 
 
 def get_version_string():

=== modified file 'rbtools/postreview.py'
--- rbtools/postreview.py	2010-07-31 18:31:05 +0000
+++ rbtools/postreview.py	2011-11-06 07:43:24 +0000
@@ -1,10 +1,9 @@
 #!/usr/bin/env python
+import base64
 import cookielib
-import difflib
 import getpass
 import marshal
 import mimetools
-import ntpath
 import os
 import re
 import socket
@@ -14,7 +13,9 @@
 import tempfile
 import urllib
 import urllib2
+from datetime import datetime
 from optparse import OptionParser
+from pkg_resources import parse_version
 from tempfile import mkstemp
 from urlparse import urljoin, urlparse
 
@@ -25,9 +26,11 @@
     from md5 import md5
 
 try:
-    import json
+    # Specifically import json_loads, to work around some issues with
+    # installations containing incompatible modules named "json".
+    from json import loads as json_loads
 except ImportError:
-    import simplejson as json
+    from simplejson import loads as json_loads
 
 # This specific import is necessary to handle the paths for
 # cygwin enabled machines.
@@ -111,6 +114,7 @@
 user_config = None
 tempfiles = []
 options = None
+configs = []
 
 ADD_REPOSITORY_DOCS_URL = \
     'http://www.reviewboard.org/docs/manual/dev/admin/management/repositories/'
@@ -136,6 +140,15 @@
             return code_str
 
 
+class HTTPRequest(urllib2.Request):
+    def __init__(self, url, body='', headers={}, method="PUT"):
+        urllib2.Request.__init__(self, url, body, headers)
+        self.method = method
+
+    def get_method(self):
+        return self.method
+
+
 class RepositoryInfo:
     """
     A representation of a source code repository.
@@ -229,7 +242,7 @@
 
         # If one of the directories doesn't match, then path is not relative
         # to root.
-        if rootdirs != pathdirs:
+        if rootdirs != pathdirs[:len(rootdirs)]:
             return None
 
         # All the directories matched, so the relative path is whatever
@@ -238,7 +251,7 @@
         if len(pathdirs) == len(rootdirs):
             return '/'
         else:
-            return '/'.join(pathdirs[len(rootdirs):])
+            return '/' + '/'.join(pathdirs[len(rootdirs):])
 
     def _split_on_slash(self, path):
         # Split on slashes, but ignore multiple slashes and throw away any
@@ -248,6 +261,168 @@
             split = split[0:-1]
         return split
 
+class ClearCaseRepositoryInfo(RepositoryInfo):
+    """
+    A representation of a ClearCase source code repository. This version knows
+    how to find a matching repository on the server even if the URLs differ.
+    """
+
+    def __init__(self, path, base_path, vobstag, supports_parent_diffs=False):
+        RepositoryInfo.__init__(self, path, base_path,
+                                supports_parent_diffs=supports_parent_diffs)
+        self.vobstag = vobstag
+
+    def find_server_repository_info(self, server):
+        """
+        The point of this function is to find a repository on the server that
+        matches self, even if the paths aren't the same. (For example, if self
+        uses an 'http' path, but the server uses a 'file' path for the same
+        repository.) It does this by comparing VOB's name. If the
+        repositories use the same path, you'll get back self, otherwise you'll
+        get a different ClearCaseRepositoryInfo object (with a different path).
+        """
+
+        # Find VOB's family uuid based on VOB's tag
+        uuid = self._get_vobs_uuid(self.vobstag)
+        debug("Repositorie's %s uuid is %r" % (self.vobstag, uuid))
+
+        repositories = server.get_repositories()
+        for repository in repositories:
+            if repository['tool'] != 'ClearCase':
+                continue
+
+            info = self._get_repository_info(server, repository)
+
+            if not info or uuid != info['uuid']:
+                continue
+
+            debug('Matching repository uuid:%s with path:%s' %(uuid,
+                  info['repopath']))
+            return ClearCaseRepositoryInfo(info['repopath'],
+                    info['repopath'], uuid)
+
+        # We didn't found uuid but if version is >= 1.5.3
+        # we can try to use VOB's name hoping it is better
+        # than current VOB's path.
+        if server.rb_version >= '1.5.3':
+            self.path = cpath.split(self.vobstag)[1]
+
+        # We didn't find a matching repository on the server.
+        # We'll just return self and hope for the best.
+        return self
+
+    def _get_vobs_uuid(self, vobstag):
+        """Return family uuid of VOB."""
+
+        property_lines = execute(["cleartool", "lsvob", "-long", vobstag],
+                                 split_lines=True)
+        for line  in property_lines:
+            if line.startswith('Vob family uuid:'):
+                return  line.split(' ')[-1].rstrip()
+
+    def _get_repository_info(self, server, repository):
+        try:
+            return server.get_repository_info(repository['id'])
+        except APIError, e:
+            # If the server couldn't fetch the repository info, it will return
+            # code 210. Ignore those.
+            # Other more serious errors should still be raised, though.
+            if e.error_code == 210:
+                return None
+
+            raise e
+
+
+class PresetHTTPAuthHandler(urllib2.BaseHandler):
+    """urllib2 handler that conditionally presets the use of HTTP Basic Auth.
+
+    This is used when specifying --username= on the command line. It will
+    force an HTTP_AUTHORIZATION header with the user info, asking the user
+    for any missing info beforehand. It will then try this header for that
+    first request.
+
+    It will only do this once.
+    """
+    handler_order = 480 # After Basic auth
+
+    def __init__(self, url, password_mgr):
+        self.url = url
+        self.password_mgr = password_mgr
+        self.used = False
+
+    def reset(self):
+        self.password_mgr.rb_user = options.http_username
+        self.password_mgr.rb_pass = options.http_password
+        self.used = False
+
+    def http_request(self, request):
+        if options.username and not self.used:
+            # Note that we call password_mgr.find_user_password to get the
+            # username and password we're working with. This allows us to
+            # prompt if, say, --username was specified but --password was not.
+            username, password = \
+                self.password_mgr.find_user_password('Web API', self.url)
+            raw = '%s:%s' % (username, password)
+            request.add_header(
+                urllib2.HTTPBasicAuthHandler.auth_header,
+                'Basic %s' % base64.b64encode(raw).strip())
+            self.used = True
+
+        return request
+
+    https_request = http_request
+
+
+class ReviewBoardHTTPErrorProcessor(urllib2.HTTPErrorProcessor):
+    """Processes HTTP error codes.
+
+    Python 2.6 gets HTTP error code processing right, but 2.4 and 2.5 only
+    accepts HTTP 200 and 206 as success codes. This handler ensures that
+    anything in the 200 range is a success.
+    """
+    def http_response(self, request, response):
+        if not (200 <= response.code < 300):
+            response = self.parent.error('http', request, response,
+                                         response.code, response.msg,
+                                         response.info())
+
+        return response
+
+    https_response = http_response
+
+
+class ReviewBoardHTTPBasicAuthHandler(urllib2.HTTPBasicAuthHandler):
+    """Custom Basic Auth handler that doesn't retry excessively.
+
+    urllib2's HTTPBasicAuthHandler retries over and over, which is useless.
+    This subclass only retries once to make sure we've attempted with a
+    valid username and password. It will then fail so we can use
+    tempt_fate's retry handler.
+    """
+    def __init__(self, *args, **kwargs):
+        urllib2.HTTPBasicAuthHandler.__init__(self, *args, **kwargs)
+        self._retried = False
+        self._lasturl = ""
+
+    def retry_http_basic_auth(self, *args, **kwargs):
+        if self._lasturl != args[0]:
+            self._retried = False
+
+        self._lasturl = args[0]
+
+        if not self._retried:
+            self._retried = True
+            self.retried = 0
+            response = urllib2.HTTPBasicAuthHandler.retry_http_basic_auth(
+                self, *args, **kwargs)
+
+            if response.code != 401:
+                self._retried = False
+
+            return response
+        else:
+            return None
+
 
 class ReviewBoardHTTPPasswordMgr(urllib2.HTTPPasswordMgr):
     """
@@ -260,11 +435,11 @@
 
     See: http://bugs.python.org/issue974757
     """
-    def __init__(self, reviewboard_url):
+    def __init__(self, reviewboard_url, rb_user=None, rb_pass=None):
         self.passwd  = {}
         self.rb_url  = reviewboard_url
-        self.rb_user = None
-        self.rb_pass = None
+        self.rb_user = rb_user
+        self.rb_pass = rb_pass
 
     def find_user_password(self, realm, uri):
         if uri.startswith(self.rb_url):
@@ -274,10 +449,14 @@
                         'used with --diff-filename=-')
 
                 print "==> HTTP Authentication Required"
-                print 'Enter username and password for "%s" at %s' % \
+                print 'Enter authorization information for "%s" at %s' % \
                     (realm, urlparse(uri)[1])
-                self.rb_user = raw_input('Username: ')
-                self.rb_pass = getpass.getpass('Password: ')
+
+                if not self.rb_user:
+                    self.rb_user = raw_input('Username: ')
+
+                if not self.rb_pass:
+                    self.rb_pass = getpass.getpass('Password: ')
 
             return self.rb_user, self.rb_pass
         else:
@@ -296,59 +475,110 @@
             self.url += '/'
         self._info = info
         self._server_info = None
+        self.root_resource = None
+        self.deprecated_api = False
         self.cookie_file = cookie_file
         self.cookie_jar  = cookielib.MozillaCookieJar(self.cookie_file)
 
+        if self.cookie_file:
+            try:
+                self.cookie_jar.load(self.cookie_file, ignore_expires=True)
+            except IOError:
+                pass
+
         # Set up the HTTP libraries to support all of the features we need.
         cookie_handler      = urllib2.HTTPCookieProcessor(self.cookie_jar)
-        password_mgr        = ReviewBoardHTTPPasswordMgr(self.url)
-        basic_auth_handler  = urllib2.HTTPBasicAuthHandler(password_mgr)
+        password_mgr        = ReviewBoardHTTPPasswordMgr(self.url,
+                                                         options.username,
+                                                         options.password)
+        basic_auth_handler  = ReviewBoardHTTPBasicAuthHandler(password_mgr)
         digest_auth_handler = urllib2.HTTPDigestAuthHandler(password_mgr)
+        self.preset_auth_handler = PresetHTTPAuthHandler(self.url, password_mgr)
+        http_error_processor = ReviewBoardHTTPErrorProcessor()
 
         opener = urllib2.build_opener(cookie_handler,
                                       basic_auth_handler,
-                                      digest_auth_handler)
+                                      digest_auth_handler,
+                                      self.preset_auth_handler,
+                                      http_error_processor)
         opener.addheaders = [('User-agent', 'RBTools/' + get_package_version())]
         urllib2.install_opener(opener)
 
+    def check_api_version(self):
+        """Checks the API version on the server to determine which to use."""
+        try:
+            root_resource = self.api_get('api/')
+            rsp = self.api_get(root_resource['links']['info']['href'])
+
+            self.rb_version = rsp['info']['product']['package_version']
+
+            if parse_version(self.rb_version) >= parse_version('1.5.2'):
+                self.deprecated_api = False
+                self.root_resource = root_resource
+                debug('Using the new web API')
+                return True
+        except APIError, e:
+            if e.http_status not in (401, 404):
+                # We shouldn't reach this. If there's a permission denied
+                # from lack of logging in, then the basic auth handler
+                # should have hit it.
+                #
+                # However in some versions it wants you to be logged in
+                # and returns a 401 from the application after you've
+                # done your http basic auth
+                die("Unable to access the root /api/ URL on the server.")
+
+            return False
+
+        # This is an older Review Board server with the old API.
+        self.deprecated_api = True
+        debug('Using the deprecated Review Board 1.0 web API')
+        return True
+
     def login(self, force=False):
         """
         Logs in to a Review Board server, prompting the user for login
         information if needed.
         """
-        if not force and self.has_valid_cookie():
-            return
-
         if (options.diff_filename == '-' and
             not options.username and not options.submit_as and
             not options.password):
             die('Authentication information needs to be provided on '
                 'the command line when using --diff-filename=-')
 
-        print "==> Review Board Login Required"
-        print "Enter username and password for Review Board at %s" % self.url
-        if options.username:
-            username = options.username
-        elif options.submit_as:
-            username = options.submit_as
-        else:
-            username = raw_input('Username: ')
-
-        if not options.password:
-            password = getpass.getpass('Password: ')
-        else:
-            password = options.password
-
-        debug('Logging in with username "%s"' % username)
-        try:
-            self.api_post('api/json/accounts/login/', {
-                'username': username,
-                'password': password,
-            })
-        except APIError, e:
-            die("Unable to log in: %s" % e)
-
-        debug("Logged in.")
+        if self.deprecated_api:
+            print "==> Review Board Login Required"
+            print "Enter username and password for Review Board at %s" % \
+                  self.url
+
+            if options.username:
+                username = options.username
+            elif options.submit_as:
+                username = options.submit_as
+            elif not force and self.has_valid_cookie():
+                # We delay the check for a valid cookie until after looking
+                # at args, so that it doesn't override the command line.
+                return
+            else:
+                username = raw_input('Username: ')
+
+            if not options.password:
+                password = getpass.getpass('Password: ')
+            else:
+                password = options.password
+
+            debug('Logging in with username "%s"' % username)
+            try:
+                self.api_post('api/json/accounts/login/', {
+                    'username': username,
+                    'password': password,
+                })
+            except APIError, e:
+                die("Unable to log in: %s" % e)
+
+            debug("Logged in.")
+        elif force:
+            self.preset_auth_handler.reset()
 
     def has_valid_cookie(self):
         """
@@ -365,9 +595,12 @@
             # get rid of the port number if it's present.
             host = host.split(":")[0]
 
+            # Cookie files also append .local to bare hostnames
+            if '.' not in host:
+                host += '.local'
+
             debug("Looking for '%s %s' cookie in %s" % \
                   (host, path, self.cookie_file))
-            self.cookie_jar.load(self.cookie_file, ignore_expires=True)
 
             try:
                 cookie = self.cookie_jar._cookies[host][path]['rbsessionid']
@@ -384,6 +617,13 @@
 
         return False
 
+    def get_configured_repository(self):
+        for config in configs:
+            if 'REPOSITORY' in config:
+                return config['REPOSITORY']
+
+        return None
+
     def new_review_request(self, changenum, submit_as=None):
         """
         Creates a review request on a Review Board server, updating an
@@ -407,10 +647,39 @@
                     self.info.path = repository['path']
                     break
 
+            if isinstance(self.info.path, list):
+                sys.stderr.write('\n')
+                sys.stderr.write('There was an error creating this review '
+                                 'request.\n')
+                sys.stderr.write('\n')
+                sys.stderr.write('There was no matching repository path'
+                                 'found on the server.\n')
+                sys.stderr.write('List of configured repositories:\n')
+
+                for repository in repositories:
+                    sys.stderr.write('\t%s\n' % repository['path'])
+
+                sys.stderr.write('Unknown repository paths found:\n')
+
+                for foundpath in self.info.path:
+                    sys.stderr.write('\t%s\n' % foundpath)
+
+                sys.stderr.write('Ask the administrator to add one of '
+                                 'these repositories\n')
+                sys.stderr.write('to the Review Board server.\n')
+                sys.stderr.write('For information on adding repositories, '
+                                 'please read\n')
+                sys.stderr.write(ADD_REPOSITORY_DOCS_URL + '\n')
+                die()
+
+        repository = options.repository_url \
+                     or self.get_configured_repository() \
+                     or self.info.path
+
         try:
             debug("Attempting to create review request on %s for %s" %
-                  (self.info.path, changenum))
-            data = { 'repository_path': self.info.path }
+                  (repository, changenum))
+            data = {}
 
             if changenum:
                 data['changenum'] = changenum
@@ -419,7 +688,16 @@
                 debug("Submitting the review request as %s" % submit_as)
                 data['submit_as'] = submit_as
 
-            rsp = self.api_post('api/json/reviewrequests/new/', data)
+            if self.deprecated_api:
+                data['repository_path'] = repository
+                rsp = self.api_post('api/json/reviewrequests/new/', data)
+            else:
+                data['repository'] = repository
+
+                links = self.root_resource['links']
+                assert 'review_requests' in links
+                review_request_href = links['review_requests']['href']
+                rsp = self.api_post(review_request_href, data)
         except APIError, e:
             if e.error_code == 204: # Change number in use
                 rsp = e.rsp
@@ -429,9 +707,8 @@
                     debug("Review request already exists.")
                 else:
                     debug("Review request already exists. Updating it...")
-                    rsp = self.api_post(
-                        'api/json/reviewrequests/%s/update_from_changenum/' %
-                        rsp['review_request']['id'])
+                    self.update_review_request_from_changenum(
+                        changenum, rsp['review_request'])
             elif e.error_code == 206: # Invalid repository
                 sys.stderr.write('\n')
                 sys.stderr.write('There was an error creating this review '
@@ -454,6 +731,16 @@
 
         return rsp['review_request']
 
+    def update_review_request_from_changenum(self, changenum, review_request):
+        if self.deprecated_api:
+            self.api_post(
+                'api/json/reviewrequests/%s/update_from_changenum/'
+                % review_request['id'])
+        else:
+            self.api_put(review_request['links']['self']['href'], {
+                'changenum': review_request['changenum'],
+            })
+
     def set_review_request_field(self, review_request, field, value):
         """
         Sets a field in a review request to the specified value.
@@ -463,37 +750,75 @@
         debug("Attempting to set field '%s' to '%s' for review request '%s'" %
               (field, value, rid))
 
-        self.api_post('api/json/reviewrequests/%s/draft/set/' % rid, {
-            field: value,
-        })
+        if self.deprecated_api:
+            self.api_post('api/json/reviewrequests/%s/draft/set/' % rid, {
+                field: value,
+            })
+        else:
+            self.api_put(review_request['links']['draft']['href'], {
+                field: value,
+            })
 
     def get_review_request(self, rid):
         """
         Returns the review request with the specified ID.
         """
-        rsp = self.api_get('api/json/reviewrequests/%s/' % rid)
+        if self.deprecated_api:
+            url = 'api/json/reviewrequests/%s/' % rid
+        else:
+            url = '%s%s/' % (
+                self.root_resource['links']['review_requests']['href'], rid)
+
+        rsp = self.api_get(url)
+
         return rsp['review_request']
 
     def get_repositories(self):
         """
         Returns the list of repositories on this server.
         """
-        rsp = self.api_get('/api/json/repositories/')
-        return rsp['repositories']
+        if self.deprecated_api:
+            rsp = self.api_get('api/json/repositories/')
+            repositories = rsp['repositories']
+        else:
+            rsp = self.api_get(
+                self.root_resource['links']['repositories']['href'])
+            repositories = rsp['repositories']
+
+            while 'next' in rsp['links']:
+                rsp = self.api_get(rsp['links']['next']['href'])
+                repositories.extend(rsp['repositories'])
+
+        return repositories
 
     def get_repository_info(self, rid):
         """
         Returns detailed information about a specific repository.
         """
-        rsp = self.api_get('/api/json/repositories/%s/info/' % rid)
+        if self.deprecated_api:
+            url = 'api/json/repositories/%s/info/' % rid
+        else:
+            rsp = self.api_get(
+                '%s%s/' % (self.root_resource['links']['repositories']['href'],
+                           rid))
+            url = rsp['repository']['links']['info']['href']
+
+        rsp = self.api_get(url)
+
         return rsp['info']
 
     def save_draft(self, review_request):
         """
         Saves a draft of a review request.
         """
-        self.api_post("api/json/reviewrequests/%s/draft/save/" %
-                      review_request['id'])
+        if self.deprecated_api:
+            self.api_post('api/json/reviewrequests/%s/draft/save/' % \
+                          review_request['id'])
+        else:
+            self.api_put(review_request['links']['draft']['href'], {
+                'public': 1,
+            })
+
         debug("Review request draft saved")
 
     def upload_diff(self, review_request, diff_content, parent_diff_content):
@@ -522,16 +847,40 @@
                 'content': parent_diff_content
             }
 
-        self.api_post('api/json/reviewrequests/%s/diff/new/' %
-                      review_request['id'], fields, files)
+        if self.deprecated_api:
+            self.api_post('api/json/reviewrequests/%s/diff/new/' %
+                          review_request['id'], fields, files)
+        else:
+            self.api_post(review_request['links']['diffs']['href'],
+                          fields, files)
+
+    def reopen(self, review_request):
+        """
+        Reopen discarded review request.
+        """
+        debug("Reopening")
+
+        if self.deprecated_api:
+            self.api_post('api/json/reviewrequests/%s/reopen/' %
+                          review_request['id'])
+        else:
+            self.api_put(review_request['links']['self']['href'], {
+                'status': 'pending',
+            })
 
     def publish(self, review_request):
         """
         Publishes a review request.
         """
         debug("Publishing")
-        self.api_post('api/json/reviewrequests/%s/publish/' %
-                      review_request['id'])
+
+        if self.deprecated_api:
+            self.api_post('api/json/reviewrequests/%s/publish/' %
+                          review_request['id'])
+        else:
+            self.api_put(review_request['links']['draft']['href'], {
+                'public': 1,
+            })
 
     def _get_server_info(self):
         if not self._server_info:
@@ -546,9 +895,12 @@
         Loads in a JSON file and returns the data if successful. On failure,
         APIError is raised.
         """
-        rsp = json.loads(data)
+        rsp = json_loads(data)
 
         if rsp['stat'] == 'fail':
+            # With the new API, we should get something other than HTTP
+            # 200 for errors, in which case we wouldn't get this far.
+            assert self.deprecated_api
             self.process_error(200, data)
 
         return rsp
@@ -556,7 +908,7 @@
     def process_error(self, http_status, data):
         """Processes an error, raising an APIError with the information."""
         try:
-            rsp = json.loads(data)
+            rsp = json_loads(data)
 
             assert rsp['stat'] == 'fail'
 
@@ -578,12 +930,21 @@
 
         url = self._make_url(path)
         rsp = urllib2.urlopen(url).read()
-        self.cookie_jar.save(self.cookie_file)
+
+        try:
+            self.cookie_jar.save(self.cookie_file)
+        except IOError, e:
+            debug('Failed to write cookie file: %s' % e)
         return rsp
 
     def _make_url(self, path):
         """Given a path on the server returns a full http:// style url"""
+        if path.startswith('http'):
+            # This is already a full path.
+            return path
+
         app = urlparse(self.url)[2]
+
         if path[0] == '/':
             url = urljoin(self.url, app[:-1] + path)
         else:
@@ -624,7 +985,66 @@
         }
 
         try:
-            r = urllib2.Request(url, body, headers)
+            r = urllib2.Request(str(url), body, headers)
+            data = urllib2.urlopen(r).read()
+            try:
+                self.cookie_jar.save(self.cookie_file)
+            except IOError, e:
+                debug('Failed to write cookie file: %s' % e)
+            return data
+        except urllib2.HTTPError, e:
+            # Re-raise so callers can interpret it.
+            raise e
+        except urllib2.URLError, e:
+            try:
+                debug(e.read())
+            except AttributeError:
+                pass
+
+            die("Unable to access %s. The host path may be invalid\n%s" % \
+                (url, e))
+
+    def http_put(self, path, fields):
+        """
+        Performs an HTTP PUT on the specified path, storing any cookies that
+        were set.
+        """
+        url = self._make_url(path)
+        debug('HTTP PUTting to %s: %s' % (url, fields))
+
+        content_type, body = self._encode_multipart_formdata(fields, None)
+        headers = {
+            'Content-Type': content_type,
+            'Content-Length': str(len(body))
+        }
+
+        try:
+            r = HTTPRequest(url, body, headers, method='PUT')
+            data = urllib2.urlopen(r).read()
+            self.cookie_jar.save(self.cookie_file)
+            return data
+        except urllib2.HTTPError, e:
+            # Re-raise so callers can interpret it.
+            raise e
+        except urllib2.URLError, e:
+            try:
+                debug(e.read())
+            except AttributeError:
+                pass
+
+            die("Unable to access %s. The host path may be invalid\n%s" % \
+                (url, e))
+
+    def http_delete(self, path):
+        """
+        Performs an HTTP DELETE on the specified path, storing any cookies that
+        were set.
+        """
+        url = self._make_url(path)
+        debug('HTTP DELETing %s' % url)
+
+        try:
+            r = HTTPRequest(url, method='DELETE')
             data = urllib2.urlopen(r).read()
             self.cookie_jar.save(self.cookie_file)
             return data
@@ -649,6 +1069,24 @@
         except urllib2.HTTPError, e:
             self.process_error(e.code, e.read())
 
+    def api_put(self, path, fields=None):
+        """
+        Performs an API call using HTTP PUT at the specified path.
+        """
+        try:
+            return self.process_json(self.http_put(path, fields))
+        except urllib2.HTTPError, e:
+            self.process_error(e.code, e.read())
+
+    def api_delete(self, path):
+        """
+        Performs an API call using HTTP DELETE at the specified path.
+        """
+        try:
+            return self.process_json(self.http_delete(path))
+        except urllib2.HTTPError, e:
+            self.process_error(e.code, e.read())
+
     def _encode_multipart_formdata(self, fields, files):
         """
         Encodes data for use in an HTTP POST.
@@ -663,7 +1101,7 @@
             content += "--" + BOUNDARY + "\r\n"
             content += "Content-Disposition: form-data; name=\"%s\"\r\n" % key
             content += "\r\n"
-            content += fields[key] + "\r\n"
+            content += str(fields[key]) + "\r\n"
 
         for key in files:
             filename = files[key]['filename']
@@ -690,25 +1128,29 @@
     def get_repository_info(self):
         return None
 
+    def check_options(self):
+        pass
+
     def scan_for_server(self, repository_info):
         """
         Scans the current directory on up to find a .reviewboard file
         containing the server path.
         """
-        server_url = self._get_server_from_config(user_config, repository_info)
-        if server_url:
-            return server_url
-
-        for path in walk_parents(os.getcwd()):
-            filename = os.path.join(path, ".reviewboardrc")
-            if os.path.exists(filename):
-                config = load_config_file(filename)
+        server_url = None
+
+        if user_config:
+            server_url = self._get_server_from_config(user_config,
+                                                      repository_info)
+
+        if not server_url:
+            for config in configs:
                 server_url = self._get_server_from_config(config,
                                                           repository_info)
+
                 if server_url:
-                    return server_url
+                    break
 
-        return None
+        return server_url
 
     def diff(self, args):
         """
@@ -723,7 +1165,7 @@
         """
         Returns the generated diff between revisions in the repository.
         """
-        return None
+        return (None, None)
 
     def _get_server_from_config(self, config, repository_info):
         if 'REVIEWBOARD_URL' in config:
@@ -773,7 +1215,7 @@
         if i != -1:
             repository_path = repository_path[i + 1:]
 
-        i = repository_path.find(":")
+        i = repository_path.rfind(":")
         if i != -1:
             host = repository_path[:i]
             try:
@@ -804,7 +1246,7 @@
         for rev in revision_range.split(":"):
             revs += ["-r", rev]
 
-        return self.do_diff(revs)
+        return (self.do_diff(revs + args), None)
 
     def do_diff(self, params):
         """
@@ -822,308 +1264,324 @@
     information and generates compatible diffs.
     This client assumes that cygwin is installed on windows.
     """
-    ccroot_path = "/view/reviewboard.diffview/vobs/"
-    viewinfo = ""
-    viewtype = "snapshot"
-
-    def get_filename_hash(self, fname):
-        # Hash the filename string so its easy to find the file later on.
-        return md5(fname).hexdigest()
+    viewtype = None
 
     def get_repository_info(self):
+        """Returns information on the Clear Case repository.
+
+        This will first check if the cleartool command is
+        installed and in the path, and post-review was run
+        from inside of the view.
+        """
         if not check_install('cleartool help'):
             return None
 
-        # We must be running this from inside a view.
-        # Otherwise it doesn't make sense.
-        self.viewinfo = execute(["cleartool", "pwv", "-short"])
-        if self.viewinfo.startswith('\*\* NONE'):
+        viewname = execute(["cleartool", "pwv", "-short"]).strip()
+        if viewname.startswith('** NONE'):
             return None
 
-        # Returning the hardcoded clearcase root path to match the server
-        #   respository path.
-        # There is no reason to have a dynamic path unless you have
-        #   multiple clearcase repositories. This should be implemented.
-        return RepositoryInfo(path=self.ccroot_path,
-                              base_path=self.ccroot_path,
+        # Now that we know it's ClearCase, make sure we have GNU diff installed,
+        # and error out if we don't.
+        check_gnu_diff()
+
+        property_lines = execute(["cleartool", "lsview", "-full", "-properties",
+                                  "-cview"], split_lines=True)
+        for line in property_lines:
+            properties = line.split(' ')
+            if properties[0] == 'Properties:':
+                # Determine the view type and check if it's supported.
+                #
+                # Specifically check if webview was listed in properties
+                # because webview types also list the 'snapshot'
+                # entry in properties.
+                if 'webview' in properties:
+                    die("Webviews are not supported. You can use post-review"
+                        " only in dynamic or snapshot view.")
+                if 'dynamic' in properties:
+                    self.viewtype = 'dynamic'
+                else:
+                    self.viewtype = 'snapshot'
+
+                break
+
+        # Find current VOB's tag
+        vobstag = execute(["cleartool", "describe", "-short", "vob:."],
+                            ignore_errors=True).strip()
+        if "Error: " in vobstag:
+            die("To generate diff run post-review inside vob.")
+
+        # From current working directory cut path to VOB.
+        # VOB's tag contain backslash character before VOB's name.
+        # I hope that first character of VOB's tag like '\new_proj'
+        # won't be treat as new line character but two separate:
+        # backslash and letter 'n'
+        cwd = os.getcwd()
+        base_path = cwd[:cwd.find(vobstag) + len(vobstag)]
+
+        return ClearCaseRepositoryInfo(path=base_path,
+                              base_path=base_path,
+                              vobstag=vobstag,
                               supports_parent_diffs=False)
 
-    def get_previous_version(self, files):
-        file = []
-        curdir = os.getcwd()
-
-        # Cygwin case must transform a linux-like path to windows like path
-        #   including drive letter.
-        if 'cygdrive' in curdir:
-            where = curdir.index('cygdrive') + 9
-            drive_letter = curdir[where:where+1]
-            curdir = drive_letter + ":\\" + curdir[where+2:len(curdir)]
-
-        for key in files:
-            # Sometimes there is a quote in the filename. It must be removed.
-            key = key.replace('\'', '')
-            elem_path = cpath.normpath(os.path.join(curdir, key))
-
-            # Removing anything before the last /vobs
-            #   because it may be repeated.
-            elem_path_idx = elem_path.rfind("/vobs")
-            if elem_path_idx != -1:
-                elem_path = elem_path[elem_path_idx:len(elem_path)].strip("\"")
-
-            # Call cleartool to get this version and the previous version
-            #   of the element.
-            curr_version, pre_version = execute(
-                ["cleartool", "desc", "-pre", elem_path], split_lines=True)
-            curr_version = cpath.normpath(curr_version)
-            pre_version = pre_version.split(':')[1].strip()
-
-            # If a specific version was given, remove it from the path
-            #   to avoid version duplication
-            if "@@" in elem_path:
-                elem_path = elem_path[:elem_path.rfind("@@")]
-            file.append(elem_path + "@@" + pre_version)
-            file.append(curr_version)
-
-        # Determnine if the view type is snapshot or dynamic.
-        if os.path.exists(file[0]):
-            self.viewtype = "dynamic"
-
-        return file
-
-    def get_extended_namespace(self, files):
-        """
-        Parses the file path to get the extended namespace
-        """
-        versions = self.get_previous_version(files)
-
-        evfiles = []
-        hlist = []
-
-        for vkey in versions:
-            # Verify if it is a checkedout file.
-            if "CHECKEDOUT" in vkey:
-                # For checkedout files just add it to the file list
-                #   since it cannot be accessed outside the view.
-                splversions = vkey[:vkey.rfind("@@")]
-                evfiles.append(splversions)
-            else:
-                # For checkedin files.
-                ext_path = []
-                ver = []
-                fname = ""      # fname holds the file name without the version.
-                (bpath, fpath) = cpath.splitdrive(vkey)
-                if bpath :
-                    # Windows.
-                    # The version (if specified like file.c@@/main/1)
-                    #   should be kept as a single string
-                    #   so split the path and concat the file name
-                    #   and version in the last position of the list.
-                    ver = fpath.split("@@")
-                    splversions = fpath[:vkey.rfind("@@")].split("\\")
-                    fname = splversions.pop()
-                    splversions.append(fname + ver[1])
-                else :
-                    # Linux.
-                    bpath = vkey[:vkey.rfind("vobs")+4]
-                    fpath = vkey[vkey.rfind("vobs")+5:]
-                    ver = fpath.split("@@")
-                    splversions =  ver[0][:vkey.rfind("@@")].split("/")
-                    fname = splversions.pop()
-                    splversions.append(fname + ver[1])
-
-                filename = splversions.pop()
-                bpath = cpath.normpath(bpath + "/")
-                elem_path = bpath
-
-                for key in splversions:
-                    # For each element (directory) in the path,
-                    #   get its version from clearcase.
-                    elem_path = cpath.join(elem_path, key)
-
-                    # This is the version to be appended to the extended
-                    #   path list.
-                    this_version = execute(
-                        ["cleartool", "desc", "-fmt", "%Vn",
-                        cpath.normpath(elem_path)])
-                    if this_version:
-                        ext_path.append(key + "/@@" + this_version + "/")
-                    else:
-                        ext_path.append(key + "/")
-
-                # This must be done in case we haven't specified
-                #   the version on the command line.
-                ext_path.append(cpath.normpath(fname + "/@@" +
-                    vkey[vkey.rfind("@@")+2:len(vkey)]))
-                epstr = cpath.join(bpath, cpath.normpath(''.join(ext_path)))
-                evfiles.append(epstr)
-
-                """
-                In windows, there is a problem with long names(> 254).
-                In this case, we hash the string and copy the unextended
-                  filename to a temp file whose name is the hash.
-                This way we can get the file later on for diff.
-                The same problem applies to snapshot views where the
-                  extended name isn't available.
-                The previous file must be copied from the CC server
-                  to a local dir.
-                """
-                if cpath.exists(epstr) :
-                    pass
-                else:
-                    if len(epstr) > 254 or self.viewtype == "snapshot":
-                        name = self.get_filename_hash(epstr)
-                        # Check if this hash is already in the list
-                        try:
-                            i = hlist.index(name)
-                            die("ERROR: duplicate value %s : %s" %
-                                (name, epstr))
-                        except ValueError:
-                            hlist.append(name)
-
-                        normkey = cpath.normpath(vkey)
-                        td = tempfile.gettempdir()
-                        # Cygwin case must transform a linux-like path to
-                        # windows like path including drive letter
-                        if 'cygdrive' in td:
-                            where = td.index('cygdrive') + 9
-                            drive_letter = td[where:where+1] + ":"
-                            td = cpath.join(drive_letter, td[where+1:])
-                        tf = cpath.normpath(cpath.join(td, name))
-                        if cpath.exists(tf):
-                            debug("WARNING: FILE EXISTS")
-                            os.unlink(tf)
-                        execute(["cleartool", "get", "-to", tf, normkey])
-                    else:
-                        die("ERROR: FILE NOT FOUND : %s" % epstr)
-
-        return evfiles
-
-    def get_files_from_label(self, label):
-        voblist=[]
-        # Get the list of vobs for the current view
-        allvoblist = execute(["cleartool", "lsvob", "-short"]).split()
-        # For each vob, find if the label is present
-        for vob in allvoblist:
-            try:
-                execute(["cleartool", "describe", "-local",
-                    "lbtype:%s@%s" % (label, vob)]).split()
-                voblist.append(vob)
-            except:
-                pass
-
-        filelist=[]
-        # For each vob containing the label, get the file list
-        for vob in voblist:
-            try:
-                res = execute(["cleartool", "find", vob, "-all", "-version",
-                    "lbtype(%s)" % label, "-print"])
-                filelist.extend(res.split())
-            except :
-                pass
-
-        # Return only the unique itens
-        return set(filelist)
+    def check_options(self):
+        if ((options.revision_range or options.tracking)
+            and self.viewtype != "dynamic"):
+            die("To generate diff using parent branch or by passing revision "
+                "ranges, you must use a dynamic view.")
+
+    def _determine_version(self, version_path):
+        """Determine numeric version of revision.
+
+        CHECKEDOUT is marked as infinity to be treated
+        always as highest possible version of file.
+        CHECKEDOUT, in ClearCase, is something like HEAD.
+        """
+        branch, number = cpath.split(version_path)
+        if number == 'CHECKEDOUT':
+            return float('inf')
+        return int(number)
+
+    def _construct_extended_path(self, path, version):
+        """Combine extended_path from path and version.
+
+        CHECKEDOUT must be removed becasue this one version
+        doesn't exists in MVFS (ClearCase dynamic view file
+        system). Only way to get content of checked out file
+        is to use filename only."""
+        if not version or version.endswith('CHECKEDOUT'):
+            return path
+
+        return "%s@@%s" % (path, version)
+
+    def _sanitize_branch_changeset(self, changeset):
+        """Return changeset containing non-binary, branched file versions.
+
+        Changeset contain only first and last version of file made on branch.
+        """
+        changelist = {}
+
+        for path, previous, current in changeset:
+            version_number = self._determine_version(current)
+
+            if path not in changelist:
+                changelist[path] = {
+                    'highest': version_number,
+                    'current': current,
+                    'previous': previous
+                }
+
+            if version_number == 0:
+                # Previous version of 0 version on branch is base
+                changelist[path]['previous'] = previous
+            elif version_number > changelist[path]['highest']:
+                changelist[path]['highest'] = version_number
+                changelist[path]['current'] = current
+
+        # Convert to list
+        changeranges = []
+        for path, version in changelist.iteritems():
+            changeranges.append(
+                (self._construct_extended_path(path, version['previous']),
+                 self._construct_extended_path(path, version['current']))
+            )
+
+        return changeranges
+
+    def _sanitize_checkedout_changeset(self, changeset):
+        """Return changeset containing non-binary, checkdout file versions."""
+
+        changeranges = []
+        for path, previous, current in changeset:
+            version_number = self._determine_version(current)
+            changeranges.append(
+                (self._construct_extended_path(path, previous),
+                self._construct_extended_path(path, current))
+            )
+
+        return changeranges
+
+    def _directory_content(self, path):
+        """Return directory content ready for saving to tempfile."""
+
+        return ''.join([
+            '%s\n' % s
+            for s in sorted(os.listdir(path))
+        ])
+
+    def _construct_changeset(self, output):
+        return [
+            info.split('\t')
+            for info in output.strip().split('\n')
+        ]
+
+    def get_checkedout_changeset(self):
+        """Return information about the checked out changeset.
+
+        This function returns: kind of element, path to file,
+        previews and current file version.
+        """
+        changeset = []
+        # We ignore return code 1 in order to
+        # omit files that Clear Case can't read.
+        output = execute([
+            "cleartool",
+            "lscheckout",
+            "-all",
+            "-cview",
+            "-me",
+            "-fmt",
+            r"%En\t%PVn\t%Vn\n"],
+            extra_ignore_errors=(1,),
+            with_errors=False)
+
+        if output:
+            changeset = self._construct_changeset(output)
+
+        return self._sanitize_checkedout_changeset(changeset)
+
+    def get_branch_changeset(self, branch):
+        """Returns information about the versions changed on a branch.
+
+        This takes into account the changes on the branch owned by the
+        current user in all vobs of the current view.
+        """
+        changeset = []
+
+        # We ignore return code 1 in order to
+        # omit files that Clear Case can't read.
+        if sys.platform.startswith('win'):
+            CLEARCASE_XPN = '%CLEARCASE_XPN%'
+        else:
+            CLEARCASE_XPN = '$CLEARCASE_XPN'
+
+        output = execute([
+            "cleartool",
+            "find",
+            "-all",
+            "-version",
+            "brtype(%s)" % branch,
+            "-exec",
+            'cleartool descr -fmt ' \
+            r'"%En\t%PVn\t%Vn\n" ' \
+            + CLEARCASE_XPN],
+            extra_ignore_errors=(1,),
+            with_errors=False)
+
+        if output:
+            changeset = self._construct_changeset(output)
+
+        return self._sanitize_branch_changeset(changeset)
 
     def diff(self, files):
-        """
-        Performs a diff of the specified file and its previous version.
-        """
-        # We must be running this from inside a view.
-        # Otherwise it doesn't make sense.
-        return self.do_diff(self.get_extended_namespace(files))
-
-    def diff_label(self, label):
-        """
-        Get the files that are attached to a label and diff them
-        TODO
-        """
-        return self.diff(self.get_files_from_label(label))
+        """Performs a diff of the specified file and its previous version."""
+
+        if options.tracking:
+            changeset = self.get_branch_changeset(options.tracking)
+        else:
+            changeset = self.get_checkedout_changeset()
+
+        return self.do_diff(changeset)
 
     def diff_between_revisions(self, revision_range, args, repository_info):
-        """
-        Performs a diff between 2 revisions of a CC repository.
-        """
-        rev_str = ''
-
-        for rev in revision_range.split(":"):
-            rev_str += "-r %s " % rev
-
-        return self.do_diff(rev_str)
-
-    def do_diff(self, params):
-        # Diff returns "1" if differences were found.
-        # Add the view name and view type to the description
-        if options.description:
-            options.description = ("VIEW: " + self.viewinfo +
-                "VIEWTYPE: " + self.viewtype + "\n" + options.description)
+        """Performs a diff between passed revisions or branch."""
+
+        # Convert revision range to list of:
+        # (previous version, current version) tuples
+        revision_range = revision_range.split(';')
+        changeset = zip(revision_range[0::2], revision_range[1::2])
+
+        return (self.do_diff(changeset)[0], None)
+
+    def diff_files(self, old_file, new_file):
+        """Return unified diff for file.
+
+        Most effective and reliable way is use gnu diff.
+        """
+        diff_cmd = ["diff", "-uN", old_file, new_file]
+        dl = execute(diff_cmd, extra_ignore_errors=(1,2),
+                     translate_newlines=False)
+
+        # If the input file has ^M characters at end of line, lets ignore them.
+        dl = dl.replace('\r\r\n', '\r\n')
+        dl = dl.splitlines(True)
+
+        # Special handling for the output of the diff tool on binary files:
+        #     diff outputs "Files a and b differ"
+        # and the code below expects the output to start with
+        #     "Binary files "
+        if (len(dl) == 1 and
+            dl[0].startswith('Files %s and %s differ' % (old_file, new_file))):
+            dl = ['Binary files %s and %s differ\n' % (old_file, new_file)]
+
+        # We need oids of files to translate them to paths on reviewboard repository
+        old_oid = execute(["cleartool", "describe", "-fmt", "%On", old_file])
+        new_oid = execute(["cleartool", "describe", "-fmt", "%On", new_file])
+
+        if dl == [] or dl[0].startswith("Binary files "):
+            if dl == []:
+                dl = ["File %s in your changeset is unmodified\n" % new_file]
+
+            dl.insert(0, "==== %s %s ====\n" % (old_oid, new_oid))
+            dl.append('\n')
         else:
-            options.description = (self.viewinfo +
-                "VIEWTYPE: " + self.viewtype + "\n")
-
-        o = []
-        Feol = False
-        while len(params) > 0:
-            # Read both original and modified files.
-            onam = params.pop(0)
-            mnam = params.pop(0)
-            file_data = []
-            do_rem = False
-            # If the filename length is greater than 254 char for windows,
-            #   we copied the file to a temp file
-            #   because the open will not work for path greater than 254.
-            # This is valid for the original and
-            #   modified files if the name size is > 254.
-            for filenam in (onam, mnam) :
-                if cpath.exists(filenam) and self.viewtype == "dynamic":
-                    do_rem = False
-                    fn = filenam
-                elif len(filenam) > 254 or self.viewtype == "snapshot":
-                    fn = self.get_filename_hash(filenam)
-                    fn = cpath.join(tempfile.gettempdir(), fn)
-                    do_rem = True
-                fd = open(cpath.normpath(fn))
-                fdata = fd.readlines()
-                fd.close()
-                file_data.append(fdata)
-                # If the file was temp, it should be removed.
-                if do_rem:
-                    os.remove(filenam)
-
-            modi = file_data.pop()
-            orig = file_data.pop()
-
-            # For snapshot views, the local directories must be removed because
-            #   they will break the diff on the server. Just replacing
-            #   everything before the view name (including the view name) for
-            #   vobs do the work.
-            if (self.viewtype == "snapshot"
-                and (sys.platform.startswith('win')
-                  or sys.platform.startswith('cygwin'))):
-                    vinfo = self.viewinfo.rstrip("\r\n")
-                    mnam = "c:\\\\vobs" + mnam[mnam.rfind(vinfo) + len(vinfo):]
-                    onam = "c:\\\\vobs" + onam[onam.rfind(vinfo) + len(vinfo):]
-            # Call the diff lib to generate a diff.
-            # The dates are bogus, since they don't natter anyway.
-            # The only thing is that two spaces are needed to the server
-            #   so it can identify the heades correctly.
-            diff = difflib.unified_diff(orig, modi, onam, mnam,
-               '  2002-02-21 23:30:39.942229878 -0800',
-               '  2002-02-21 23:30:50.442260588 -0800', lineterm=' \n')
-            # Transform the generator output into a string output
-            #   Use a comprehension instead of a generator,
-            #   so 2.3.x doesn't fail to interpret.
-            diffstr = ''.join([str(l) for l in diff])
-            # Workaround for the difflib no new line at end of file
-            #   problem.
-            if not diffstr.endswith('\n'):
-                diffstr = diffstr + ("\n\\ No newline at end of file\n")
-            o.append(diffstr)
-
-        ostr = ''.join(o)
-        return (ostr, None) # diff, parent_diff (not supported)
+            dl.insert(2, "==== %s %s ====\n" % (old_oid, new_oid))
+
+        return dl
+
+    def diff_directories(self, old_dir, new_dir):
+        """Return uniffied diff between two directories content.
+
+        Function save two version's content of directory to temp
+        files and treate them as casual diff between two files.
+        """
+        old_content = self._directory_content(old_dir)
+        new_content = self._directory_content(new_dir)
+
+        old_tmp = make_tempfile(content=old_content)
+        new_tmp = make_tempfile(content=new_content)
+
+        diff_cmd = ["diff", "-uN", old_tmp, new_tmp]
+        dl = execute(diff_cmd,
+                     extra_ignore_errors=(1,2),
+                     translate_newlines=False,
+                     split_lines=True)
+
+        # Replacing temporary filenames to
+        # real directory names and add ids
+        if dl:
+            dl[0] = dl[0].replace(old_tmp, old_dir)
+            dl[1] = dl[1].replace(new_tmp, new_dir)
+            old_oid = execute(["cleartool", "describe", "-fmt", "%On", old_dir])
+            new_oid = execute(["cleartool", "describe", "-fmt", "%On", new_dir])
+            dl.insert(2, "==== %s %s ====\n" % (old_oid, new_oid))
+
+        return dl
+
+    def do_diff(self, changeset):
+        """Generates a unified diff for all files in the changeset."""
+
+        diff = []
+        for old_file, new_file in changeset:
+            dl = []
+            if cpath.isdir(new_file):
+                dl = self.diff_directories(old_file, new_file)
+            elif cpath.exists(new_file):
+                dl = self.diff_files(old_file, new_file)
+            else:
+                debug("File %s does not exist or access is denied." % new_file)
+                continue
+
+            if dl:
+                diff.append(''.join(dl))
+
+        return (''.join(diff), None)
 
 
 class SVNClient(SCMClient):
+    # Match the diff control lines generated by 'svn diff'.
+    DIFF_ORIG_FILE_LINE_RE = re.compile(r'^---\s+.*\s+\(.*\)')
+    DIFF_NEW_FILE_LINE_RE = re.compile(r'^\+\+\+\s+.*\s+\(.*\)')
+
     """
     A wrapper around the svn Subversion tool that fetches repository
     information and generates compatible diffs.
@@ -1164,6 +1622,15 @@
 
         return SvnRepositoryInfo(path, base_path, m.group(1))
 
+    def check_options(self):
+        if (options.repository_url and
+            not options.revision_range and
+            not options.diff_filename):
+            sys.stderr.write("The --repository-url option requires either the "
+                             "--revision-range option or the --diff-filename "
+                             "option.\n")
+            sys.exit(1)
+
     def scan_for_server(self, repository_info):
         # Scan first for dot files, since it's faster and will cover the
         # user's $HOME/.reviewboardrc
@@ -1199,6 +1666,13 @@
         return (self.do_diff(["svn", "diff", "--diff-cmd=diff"] + files),
                 None)
 
+    def diff_changelist(self, changelist):
+        """
+        Performs a diff for a local changelist.
+        """
+        return (self.do_diff(["svn", "diff", "--changelist", changelist]),
+                None)
+
     def diff_between_revisions(self, revision_range, args, repository_info):
         """
         Performs a diff between 2 revisions of a Subversion repository.
@@ -1233,14 +1707,14 @@
 
             old_url = url + '@' + revisions[0]
 
-            return self.do_diff(["svn", "diff", "--diff-cmd=diff", old_url,
-                                 new_url] + files,
-                                repository_info)
+            return (self.do_diff(["svn", "diff", "--diff-cmd=diff", old_url,
+                                  new_url] + files,
+                                 repository_info), None)
         # Otherwise, perform the revision range diff using a working copy
         else:
-            return self.do_diff(["svn", "diff", "--diff-cmd=diff", "-r",
-                                 revision_range],
-                                repository_info)
+            return (self.do_diff(["svn", "diff", "--diff-cmd=diff", "-r",
+                                  revision_range],
+                                 repository_info), None)
 
     def do_diff(self, cmd, repository_info=None):
         """
@@ -1272,12 +1746,12 @@
 
         from_line = ""
         for line in diff_content:
-            if line.startswith('--- '):
+            if self.DIFF_ORIG_FILE_LINE_RE.match(line):
                 from_line = line
                 continue
 
             # This is where we decide how mangle the previous '--- '
-            if line.startswith('+++ '):
+            if self.DIFF_NEW_FILE_LINE_RE.match(line):
                 to_file, _ = self.parse_filename_header(line[4:])
                 info       = self.svn_info(to_file)
                 if info.has_key("Copied From URL"):
@@ -1306,7 +1780,9 @@
 
         for line in diff_content:
             front = None
-            if line.startswith('+++ ') or line.startswith('--- ') or line.startswith('Index: '):
+            if (self.DIFF_NEW_FILE_LINE_RE.match(line)
+                or self.DIFF_ORIG_FILE_LINE_RE.match(line)
+                or line.startswith('Index: ')):
                 front, line = line.split(" ", 1)
 
             if front:
@@ -1451,15 +1927,20 @@
         return None
 
     def get_changenum(self, args):
-        if len(args) == 1:
+        if len(args) == 0:
+            return "default"
+        elif len(args) == 1:
             if args[0] == "default":
                 return "default"
 
             try:
                 return str(int(args[0]))
             except ValueError:
-                pass
-        return None
+                # (if it isn't a number, it can't be a cln)
+                return None
+        # there are multiple args (not a cln)
+        else:
+            return None
 
     def diff(self, args):
         """
@@ -1474,6 +1955,9 @@
         if options.p4_port:
            os.environ['P4PORT'] = options.p4_port
 
+        if options.p4_passwd:
+            os.environ['P4PASSWD'] = options.p4_passwd
+
         changenum = self.get_changenum(args)
         if changenum is None:
             return self._path_diff(args)
@@ -1539,7 +2023,7 @@
             # 'depotFile': '...'
             # 'change': '123456'
             for record in records:
-                if record['action'] != 'delete':
+                if record['action'] not in ('delete', 'move/delete'):
                     if revision2:
                         files[record['depotFile']] = [record, None]
                     else:
@@ -1550,7 +2034,7 @@
                 second_rev_path = m.group('path') + revision2[1:]
                 records = self._run_p4(['files', second_rev_path])
                 for record in records:
-                    if record['action'] != 'delete':
+                    if record['action'] not in ('delete', 'move/delete'):
                         try:
                             m = files[record['depotFile']]
                             m[1] = record
@@ -1574,6 +2058,11 @@
                     old_file = tmp_diff_from_filename
                     changetype_short = 'D'
                     base_revision = int(first_record['rev'])
+                elif first_record['rev'] == second_record['rev']:
+                    # We when we know the revisions are the same, we don't need
+                    # to do any diffing. This speeds up large revision-range
+                    # diffs quite a bit.
+                    continue
                 else:
                     self._write_file(depot_path + '#' + first_record['rev'],
                                      tmp_diff_from_filename)
@@ -1644,8 +2133,15 @@
             v = self.p4d_version
 
             if v[0] < 2002 or (v[0] == "2002" and v[1] < 2):
-                description = execute(["p4", "describe", "-s", changenum],
-                                      split_lines=True)
+                describeCmd = ["p4"]
+
+                if options.p4_passwd:
+                    describeCmd.append("-P")
+                    describeCmd.append(options.p4_passwd)
+
+                describeCmd = describeCmd + ["describe", "-s", changenum]
+
+                description = execute(describeCmd, split_lines=True)
 
                 if '*pending*' in description[0]:
                     return None
@@ -1672,8 +2168,18 @@
         if changenum == "default":
             cl_is_pending = True
         else:
-            description = execute(["p4", "describe", "-s", changenum],
-                                  split_lines=True)
+            describeCmd = ["p4"]
+
+            if options.p4_passwd:
+                describeCmd.append("-P")
+                describeCmd.append(options.p4_passwd)
+
+            describeCmd = describeCmd + ["describe", "-s", changenum]
+
+            description = execute(describeCmd, split_lines=True)
+
+            if re.search("no such changelist", description[0]):
+                die("CLN %s does not exist." % changenum)
 
             # Some P4 wrappers are addding an extra line before the description
             if '*pending*' in description[0] or '*pending*' in description[1]:
@@ -1689,6 +2195,9 @@
             info = execute(["p4", "opened", "-c", str(changenum)],
                            split_lines=True)
 
+            if len(info) == 1 and info[0].startswith("File(s) not opened on this client."):
+                die("Couldn't find any affected files for this change.")
+
             for line in info:
                 data = line.split(" ")
                 description.append("... %s %s" % (data[0], data[2]))
@@ -1912,126 +2421,273 @@
     A wrapper around the hg Mercurial tool that fetches repository
     information and generates compatible diffs.
     """
+
+    def __init__(self):
+        self.hgrc = {}
+        self._type = 'hg'
+        self._hg_root = ''
+        self._remote_path = ()
+        self._hg_env = {
+            'HGRCPATH': os.devnull,
+            'HGPLAIN': '1',
+        }
+
+        # `self._remote_path_candidates` is an ordered set of hgrc
+        # paths that are checked if `parent_branch` option is not given
+        # explicitly.  The first candidate found to exist will be used,
+        # falling back to `default` (the last member.)
+        self._remote_path_candidates = ['reviewboard', 'origin', 'parent',
+                                        'default']
+
     def get_repository_info(self):
         if not check_install('hg --help'):
             return None
 
-        data = execute(["hg", "root"], ignore_errors=True)
-        if data.startswith('abort:'):
+        self._load_hgrc()
+
+        if not self.hg_root:
             # hg aborted => no mercurial repository here.
             return None
 
-        # Elsewhere, hg root output give us the repository path.
-
-        # We save data here to use it as a fallback. See below
-        local_data = data.strip()
-
-        svn = execute(["hg", "svn", "info", ], ignore_errors=True)
-
-        if (not svn.startswith('abort:') and
-            not svn.startswith("hg: unknown command")):
-            self.type = 'svn'
-            m = re.search(r'^Repository Root: (.+)$', svn, re.M)
-
-            if not m:
-                return None
-
-            path = m.group(1)
-            m2 = re.match(r'^(svn\+ssh|http|https)://([-a-zA-Z0-9.]*@)(.*)$',
-                          path)
-            if m2:
-                path = '%s://%s' % (m2.group(1), m2.group(3))
-
-            m = re.search(r'^URL: (.+)$', svn, re.M)
-
-            if not m:
-                return None
-
-            base_path = m.group(1)[len(path):] or "/"
-            return RepositoryInfo(path=path,
-                                  base_path=base_path,
-                                  supports_parent_diffs=True)
-
-        self.type = 'hg'
-
-        # We are going to search .hg/hgrc for the default path.
-        file_name = os.path.join(local_data,'.hg', 'hgrc')
-
-        if not os.path.exists(file_name):
-            return RepositoryInfo(path=local_data, base_path='/',
-                                  supports_parent_diffs=True)
-
-        f = open(file_name)
-        data = f.read()
-        f.close()
-
-        m = re.search(r'^default\s+=\s+(.+)$', data, re.M)
+        svn_info = execute(["hg", "svn", "info"], ignore_errors=True)
+
+        if (not svn_info.startswith('abort:') and
+            not svn_info.startswith("hg: unknown command") and
+            not svn_info.lower().startswith('not a child of')):
+            return self._calculate_hgsubversion_repository_info(svn_info)
+
+        self._type = 'hg'
+
+        path = self.hg_root
+        base_path = '/'
+
+        if self.hgrc:
+            self._calculate_remote_path()
+
+            if self._remote_path:
+                path = self._remote_path[1]
+                base_path = ''
+
+        return RepositoryInfo(path=path, base_path=base_path,
+                              supports_parent_diffs=True)
+
+    def _calculate_remote_path(self):
+        for candidate in self._remote_path_candidates:
+
+            rc_key = 'paths.%s' % candidate
+
+            if (not self._remote_path and self.hgrc.get(rc_key)):
+                self._remote_path = (candidate, self.hgrc.get(rc_key))
+                debug('Using candidate path %r: %r' % self._remote_path)
+
+                return
+
+    def _calculate_hgsubversion_repository_info(self, svn_info):
+        self._type = 'svn'
+        m = re.search(r'^Repository Root: (.+)$', svn_info, re.M)
 
         if not m:
-            # Return the local path, if no default value is found.
-            return RepositoryInfo(path=local_data, base_path='/',
-                                  supports_parent_diffs=True)
+            return None
 
-        path = m.group(1).strip()
-        m2 = re.match(r'^(svn\+ssh|http|https)://([-a-zA-Z0-9.]*@)(.*)$',
-                      path)
+        path = m.group(1)
+        m2 = re.match(r'^(svn\+ssh|http|https|svn)://([-a-zA-Z0-9.]*@)(.*)$',
+                        path)
         if m2:
             path = '%s://%s' % (m2.group(1), m2.group(3))
 
-        return RepositoryInfo(path=path, base_path='',
+        m = re.search(r'^URL: (.+)$', svn_info, re.M)
+
+        if not m:
+            return None
+
+        base_path = m.group(1)[len(path):] or "/"
+        return RepositoryInfo(path=path, base_path=base_path,
                               supports_parent_diffs=True)
 
+    @property
+    def hg_root(self):
+        if not self._hg_root:
+            root = execute(['hg', 'root'], env=self._hg_env,
+                           ignore_errors=True)
+
+            if not root.startswith('abort:'):
+                self._hg_root = root.strip()
+            else:
+                return None
+
+        return self._hg_root
+
+    def _load_hgrc(self):
+        for line in execute(['hg', 'showconfig'], split_lines=True):
+            key, value = line.split('=', 1)
+            self.hgrc[key] = value.strip()
+
     def extract_summary(self, revision):
         """
         Extracts the first line from the description of the given changeset.
         """
         return execute(['hg', 'log', '-r%s' % revision, '--template',
-                        r'{desc|firstline}\n'])
+                        r'{desc|firstline}\n'], env=self._hg_env)
 
     def extract_description(self, rev1, rev2):
         """
         Extracts all descriptions in the given revision range and concatenates
         them, most recent ones going first.
         """
-        numrevs = len(execute(['hg', 'log', '-r%s:%s' % (rev2, rev1),
-                               '--follow', '--template',
-                               r'{rev}\n']).strip().split('\n'))
+        numrevs = len(execute([
+            'hg', 'log', '-r%s:%s' % (rev2, rev1),
+            '--follow', '--template', r'{rev}\n'], env=self._hg_env
+        ).strip().split('\n'))
+
         return execute(['hg', 'log', '-r%s:%s' % (rev2, rev1),
                         '--follow', '--template',
                         r'{desc}\n\n', '--limit',
-                        str(numrevs - 1)]).strip()
+                        str(numrevs - 1)], env=self._hg_env).strip()
 
     def diff(self, files):
         """
         Performs a diff across all modified files in a Mercurial repository.
         """
-        # We don't support parent diffs with Mercurial yet, so we always
-        # return None for the parent diff.
-        if self.type == 'svn':
-            parent = execute(['hg', 'parent', '--svn', '--template',
-                              '{node}\n']).strip()
-
-            if options.parent_branch:
-                parent = options.parent_branch
-
-            if options.guess_summary and not options.summary:
-                options.summary = self.extract_summary(".")
-
-            if options.guess_description and not options.description:
-                options.description = self.extract_description(parent, ".")
-
-            return (execute(["hg", "diff", "--svn", '-r%s:.' % parent]), None)
-
-        return (execute(["hg", "diff"] + files), None)
+        files = files or []
+
+        if self._type == 'svn':
+            return self._get_hgsubversion_diff(files)
+        else:
+            return self._get_outgoing_diff(files)
+
+    def _get_hgsubversion_diff(self, files):
+        parent = execute(['hg', 'parent', '--svn', '--template',
+                          '{node}\n']).strip()
+
+        if options.parent_branch:
+            parent = options.parent_branch
+
+        if options.guess_summary and not options.summary:
+            options.summary = self.extract_summary(".")
+
+        if options.guess_description and not options.description:
+            options.description = self.extract_description(parent, ".")
+
+        return (execute(["hg", "diff", "--svn", '-r%s:.' % parent]), None)
+
+    def _get_outgoing_diff(self, files):
+        """
+        When working with a clone of a Mercurial remote, we need to find
+        out what the outgoing revisions are for a given branch.  It would
+        be nice if we could just do `hg outgoing --patch <remote>`, but
+        there are a couple of problems with this.
+
+        For one, the server-side diff parser isn't yet equipped to filter out
+        diff headers such as "comparing with..." and "changeset: <rev>:<hash>".
+        Another problem is that the output of `outgoing` potentially includes
+        changesets across multiple branches.
+
+        In order to provide the most accurate comparison between one's local
+        clone and a given remote -- something akin to git's diff command syntax
+        `git diff <treeish>..<treeish>` -- we have to do the following:
+
+            - get the name of the current branch
+            - get a list of outgoing changesets, specifying a custom format
+            - filter outgoing changesets by the current branch name
+            - get the "top" and "bottom" outgoing changesets
+            - use these changesets as arguments to `hg diff -r <rev> -r <rev>`
+
+
+        Future modifications may need to be made to account for odd cases like
+        having multiple diverged branches which share partial history -- or we
+        can just punish developers for doing such nonsense :)
+        """
+        files = files or []
+
+        remote = self._remote_path[0]
+
+        if not remote and options.parent_branch:
+            remote = options.parent_branch
+
+        current_branch = execute(['hg', 'branch'], env=self._hg_env).strip()
+
+        outgoing_changesets = \
+            self._get_outgoing_changesets(current_branch, remote)
+
+        top_rev, bottom_rev = \
+            self._get_top_and_bottom_outgoing_revs(outgoing_changesets)
+
+        if options.guess_summary and not options.summary:
+            options.summary = self.extract_summary(top_rev).rstrip("\n")
+
+        if options.guess_description and not options.description:
+            options.description = self.extract_description(bottom_rev, top_rev)
+
+        full_command = ['hg', 'diff', '-r', str(bottom_rev), '-r',
+                        str(top_rev)] + files
+
+        return (execute(full_command, env=self._hg_env), None)
+
+    def _get_outgoing_changesets(self, current_branch, remote):
+        """
+        Given the current branch name and a remote path, return a list
+        of outgoing changeset numbers.
+        """
+        outgoing_changesets = []
+        raw_outgoing = execute(['hg', '-q', 'outgoing', '--template',
+                                'b:{branches}\nr:{rev}\n\n', remote],
+                               env=self._hg_env)
+
+        for pair in raw_outgoing.split('\n\n'):
+            if not pair.strip():
+                continue
+
+            branch, rev = pair.strip().split('\n')
+
+            branch_name = branch[len('b:'):].strip()
+            branch_name = branch_name or 'default'
+            revno = rev[len('r:'):]
+
+            if branch_name == current_branch and revno.isdigit():
+                debug('Found outgoing changeset %s for branch %r'
+                      % (revno, branch_name))
+                outgoing_changesets.append(int(revno))
+
+        return outgoing_changesets
+
+    def _get_top_and_bottom_outgoing_revs(self, outgoing_changesets):
+        # This is a classmethod rather than a func mostly just to keep the
+        # module namespace clean.  Pylint told me to do it.
+        top_rev = max(outgoing_changesets)
+        bottom_rev = min(outgoing_changesets)
+
+        parents = execute(["hg", "log", "-r", str(bottom_rev),
+                           "--template", "{parents}"],
+                          env=self._hg_env)
+        parents = parents.rstrip("\n").split(":")
+
+        if len(parents) > 1:
+            bottom_rev = parents[0]
+        else:
+            bottom_rev = bottom_rev - 1
+
+        bottom_rev = max(0, bottom_rev)
+
+        return top_rev, bottom_rev
 
     def diff_between_revisions(self, revision_range, args, repository_info):
         """
         Performs a diff between 2 revisions of a Mercurial repository.
         """
-        if self.type != 'hg':
+        if self._type != 'hg':
             raise NotImplementedError
 
-        r1, r2 = revision_range.split(':')
+        if ':' in revision_range:
+            r1, r2 = revision_range.split(':')
+        else:
+            # If only 1 revision is given, we find the first parent and use
+            # that as the second revision.
+            #
+            # We could also use "hg diff -c r1", but then we couldn't reuse the
+            # code for extracting descriptions.
+            r2 = revision_range
+            r1 = execute(["hg", "parents", "-r", r2,
+                          "--template", "{rev}\n"]).split()[0]
 
         if options.guess_summary and not options.summary:
             options.summary = self.extract_summary(r2)
@@ -2039,7 +2695,27 @@
         if options.guess_description and not options.description:
             options.description = self.extract_description(r1, r2)
 
-        return execute(["hg", "diff", "-r", r1, "-r", r2])
+        return (execute(["hg", "diff", "-r", r1, "-r", r2],
+                        env=self._hg_env), None)
+
+    def scan_for_server(self, repository_info):
+        # Scan first for dot files, since it's faster and will cover the
+        # user's $HOME/.reviewboardrc
+        server_url = \
+            super(MercurialClient, self).scan_for_server(repository_info)
+
+        if not server_url and self.hgrc.get('reviewboard.url'):
+            server_url = self.hgrc.get('reviewboard.url').strip()
+
+        if not server_url and self._type == "svn":
+            # Try using the reviewboard:url property on the SVN repo, if it
+            # exists.
+            prop = SVNClient().scan_for_server_property(repository_info)
+
+            if prop:
+                return prop
+
+        return server_url
 
 
 class GitClient(SCMClient):
@@ -2048,93 +2724,143 @@
     compatible diffs. This will attempt to generate a diff suitable for the
     remote repository, whether git, SVN or Perforce.
     """
+    def __init__(self):
+        SCMClient.__init__(self)
+        # Store the 'correct' way to invoke git, just plain old 'git' by default
+        self.git = 'git'
+
+    def _strip_heads_prefix(self, ref):
+        """ Strips prefix from ref name, if possible """
+        return re.sub(r'^refs/heads/', '', ref)
+
     def get_repository_info(self):
         if not check_install('git --help'):
-            return None
+            # CreateProcess (launched via subprocess, used by check_install)
+            # does not automatically append .cmd for things it finds in PATH.
+            # If we're on Windows, and this works, save it for further use.
+            if sys.platform.startswith('win') and check_install('git.cmd --help'):
+                self.git = 'git.cmd'
+            else:
+                return None
 
-        git_dir = execute(["git", "rev-parse", "--git-dir"],
-                          ignore_errors=True).strip()
+        git_dir = execute([self.git, "rev-parse", "--git-dir"],
+                          ignore_errors=True).rstrip("\n")
 
         if git_dir.startswith("fatal:") or not os.path.isdir(git_dir):
             return None
+        self.bare = execute([self.git, "config", "core.bare"]).strip() == 'true'
 
         # post-review in directories other than the top level of
         # of a work-tree would result in broken diffs on the server
-        os.chdir(os.path.dirname(os.path.abspath(git_dir)))
+        if not self.bare:
+            os.chdir(os.path.dirname(os.path.abspath(git_dir)))
 
-        self.head_ref = execute(['git', 'symbolic-ref', '-q', 'HEAD']).strip()
+        self.head_ref = execute([self.git, 'symbolic-ref', '-q', 'HEAD']).strip()
 
         # We know we have something we can work with. Let's find out
-        # what it is. We'll try SVN first.
-        data = execute(["git", "svn", "info"], ignore_errors=True)
-
-        m = re.search(r'^Repository Root: (.+)$', data, re.M)
-        if m:
-            path = m.group(1)
-            m = re.search(r'^URL: (.+)$', data, re.M)
+        # what it is. We'll try SVN first, but only if there's a .git/svn
+        # directory. Otherwise, it may attempt to create one and scan
+        # revisions, which can be slow.
+        git_svn_dir = os.path.join(git_dir, 'svn')
+
+        if os.path.isdir(git_svn_dir) and len(os.listdir(git_svn_dir)) > 0:
+            data = execute([self.git, "svn", "info"], ignore_errors=True)
+
+            m = re.search(r'^Repository Root: (.+)$', data, re.M)
 
             if m:
-                base_path = m.group(1)[len(path):] or "/"
-                m = re.search(r'^Repository UUID: (.+)$', data, re.M)
+                path = m.group(1)
+                m = re.search(r'^URL: (.+)$', data, re.M)
 
                 if m:
-                    uuid = m.group(1)
-                    self.type = "svn"
-                    self.upstream_branch = options.parent_branch or 'master'
-
-                    return SvnRepositoryInfo(path=path, base_path=base_path,
-                                             uuid=uuid,
-                                             supports_parent_diffs=True)
-        else:
-            # Versions of git-svn before 1.5.4 don't (appear to) support
-            # 'git svn info'.  If we fail because of an older git install,
-            # here, figure out what version of git is installed and give
-            # the user a hint about what to do next.
-            version = execute(["git", "svn", "--version"], ignore_errors=True)
-            version_parts = re.search('version (\d+)\.(\d+)\.(\d+)',
-                                      version)
-            svn_remote = execute(["git", "config", "--get",
-                                  "svn-remote.svn.url"], ignore_errors=True)
-
-            if (version_parts and
-                not self.is_valid_version((int(version_parts.group(1)),
-                                           int(version_parts.group(2)),
-                                           int(version_parts.group(3))),
-                                          (1, 5, 4)) and
-                svn_remote):
-                die("Your installation of git-svn must be upgraded to " + \
-                    "version 1.5.4 or later")
+                    base_path = m.group(1)[len(path):] or "/"
+                    m = re.search(r'^Repository UUID: (.+)$', data, re.M)
+
+                    if m:
+                        uuid = m.group(1)
+                        self.type = "svn"
+
+                        # Get SVN tracking branch
+                        if options.parent_branch:
+                            self.upstream_branch = options.parent_branch
+                        else:
+                            data = execute([self.git, "svn", "rebase", "-n"],
+                                           ignore_errors=True)
+                            m = re.search(r'^Remote Branch:\s*(.+)$', data, re.M)
+
+                            if m:
+                                self.upstream_branch = m.group(1)
+                            else:
+                                sys.stderr.write('Failed to determine SVN tracking '
+                                                 'branch. Defaulting to "master"\n')
+                                self.upstream_branch = 'master'
+
+                        return SvnRepositoryInfo(path=path,
+                                                 base_path=base_path,
+                                                 uuid=uuid,
+                                                 supports_parent_diffs=True)
+            else:
+                # Versions of git-svn before 1.5.4 don't (appear to) support
+                # 'git svn info'.  If we fail because of an older git install,
+                # here, figure out what version of git is installed and give
+                # the user a hint about what to do next.
+                version = execute([self.git, "svn", "--version"],
+                                  ignore_errors=True)
+                version_parts = re.search('version (\d+)\.(\d+)\.(\d+)',
+                                          version)
+                svn_remote = execute([self.git, "config", "--get",
+                                      "svn-remote.svn.url"],
+                                      ignore_errors=True)
+
+                if (version_parts and
+                    not self.is_valid_version((int(version_parts.group(1)),
+                                               int(version_parts.group(2)),
+                                               int(version_parts.group(3))),
+                                              (1, 5, 4)) and
+                    svn_remote):
+                    die("Your installation of git-svn must be upgraded to "
+                        "version 1.5.4 or later")
 
         # Okay, maybe Perforce.
         # TODO
 
         # Nope, it's git then.
         # Check for a tracking branch and determine merge-base
-        short_head = self.head_ref.split('/')[-1]
-        merge = execute(['git', 'config', '--get',
+        short_head = self._strip_heads_prefix(self.head_ref)
+        merge = execute([self.git, 'config', '--get',
                          'branch.%s.merge' % short_head],
                         ignore_errors=True).strip()
-        remote = execute(['git', 'config', '--get',
+        remote = execute([self.git, 'config', '--get',
                           'branch.%s.remote' % short_head],
                          ignore_errors=True).strip()
 
-        HEADS_PREFIX = 'refs/heads/'
-
-        if merge.startswith(HEADS_PREFIX):
-            merge = merge[len(HEADS_PREFIX):]
-
+        merge = self._strip_heads_prefix(merge)
         self.upstream_branch = ''
 
         if remote and remote != '.' and merge:
             self.upstream_branch = '%s/%s' % (remote, merge)
 
-        self.upstream_branch, origin_url = self.get_origin(self.upstream_branch,
-                                                       True)
-
-        if not origin_url or origin_url.startswith("fatal:"):
-            self.upstream_branch, origin_url = self.get_origin()
-
+        url = None
+        if options.repository_url:
+            url = options.repository_url
+        else:
+            self.upstream_branch, origin_url = \
+                self.get_origin(self.upstream_branch, True)
+
+            if not origin_url or origin_url.startswith("fatal:"):
+                self.upstream_branch, origin_url = self.get_origin()
+
+            url = origin_url.rstrip('/')
+
+        # Central bare repositories don't have origin URLs.
+        # We return git_dir instead and hope for the best.
         url = origin_url.rstrip('/')
+
+        if not url:
+            url = os.path.abspath(git_dir)
+
+            # There is no remote, so skip this part of upstream_branch.
+            self.upstream_branch = self.upstream_branch.split('/')[-1]
         if url:
             self.type = "git"
             return RepositoryInfo(path=url, base_path='',
@@ -2150,10 +2876,10 @@
         upstream_branch = options.tracking or default_upstream_branch or \
                           'origin/master'
         upstream_remote = upstream_branch.split('/')[0]
-        origin_url = execute(["git", "config", "remote.%s.url" % upstream_remote],
-                         ignore_errors=ignore_errors)
-
-        return (upstream_branch, origin_url.rstrip('\n'))
+        origin_url = execute([self.git, "config", "--get",
+                              "remote.%s.url" % upstream_remote],
+                              ignore_errors=True).rstrip("\n")
+        return (upstream_branch, origin_url)
 
     def is_valid_version(self, actual, expected):
         """
@@ -2176,7 +2902,7 @@
             return server_url
 
         # TODO: Maybe support a server per remote later? Is that useful?
-        url = execute(["git", "config", "--get", "reviewboard.url"],
+        url = execute([self.git, "config", "--get", "reviewboard.url"],
                       ignore_errors=True).strip()
         if url:
             return url
@@ -2198,7 +2924,7 @@
         """
         parent_branch = options.parent_branch
 
-        self.merge_base = execute(["git", "merge-base", self.upstream_branch,
+        self.merge_base = execute([self.git, "merge-base", self.upstream_branch,
                                    self.head_ref]).strip()
 
         if parent_branch:
@@ -2209,12 +2935,13 @@
             parent_diff_lines = None
 
         if options.guess_summary and not options.summary:
-            options.summary = execute(["git", "log", "--pretty=format:%s",
-                                       "HEAD^.."], ignore_errors=True).strip()
+            s = execute([self.git, "log", "--pretty=format:%s", "HEAD^.."],
+                        ignore_errors=True)
+            options.summary = s.replace('\n', ' ').strip()
 
         if options.guess_description and not options.description:
             options.description = execute(
-                ["git", "log", "--pretty=format:%s%n%n%b",
+                [self.git, "log", "--pretty=format:%s%n%n%b",
                  (parent_branch or self.merge_base) + ".."],
                 ignore_errors=True).strip()
 
@@ -2224,16 +2951,19 @@
         """
         Performs a diff on a particular branch range.
         """
-        rev_range = "%s..%s" % (ancestor, commit)
+        if commit:
+            rev_range = "%s..%s" % (ancestor, commit)
+        else:
+            rev_range = ancestor
 
         if self.type == "svn":
-            diff_lines = execute(["git", "diff", "--no-color", "--no-prefix",
-                                  "-r", "-u", rev_range],
+            diff_lines = execute([self.git, "diff", "--no-color", "--no-prefix",
+                                  "--no-ext-diff", "-r", "-u", rev_range],
                                  split_lines=True)
             return self.make_svn_diff(ancestor, diff_lines)
         elif self.type == "git":
-            return execute(["git", "diff", "--no-color", "--full-index",
-                            rev_range])
+            return execute([self.git, "diff", "--no-color", "--full-index",
+                            "--no-ext-diff", rev_range])
 
         return None
 
@@ -2243,14 +2973,13 @@
         svn diff would generate. This is needed so the SVNTool in Review
         Board can properly parse this diff.
         """
-        rev = execute(["git", "svn", "find-rev", parent_branch]).strip()
+        rev = execute([self.git, "svn", "find-rev", parent_branch]).strip()
 
         if not rev:
             return None
 
         diff_data = ""
         filename = ""
-        revision = ""
         newfile = False
 
         for line in diff_lines:
@@ -2295,33 +3024,400 @@
 
     def diff_between_revisions(self, revision_range, args, repository_info):
         """Perform a diff between two arbitrary revisions"""
+
+        # Make a parent diff to the first of the revisions so that we
+        # never end up with broken patches:
+        self.merge_base = execute([self.git, "merge-base", self.upstream_branch,
+                                   self.head_ref]).strip()
+
         if ":" not in revision_range:
             # only one revision is specified
+
+            # Check if parent contains the first revision and make a
+            # parent diff if not:
+            pdiff_required = execute([self.git, "branch", "-r",
+                                      "--contains", revision_range])
+            parent_diff_lines = None
+
+            if not pdiff_required:
+                parent_diff_lines = self.make_diff(self.merge_base, revision_range)
+
             if options.guess_summary and not options.summary:
-                options.summary = execute(
-                    ["git", "log", "--pretty=format:%s", revision_range + ".."],
-                    ignore_errors=True).strip()
+                s = execute([self.git, "log", "--pretty=format:%s",
+                             revision_range + ".."],
+                            ignore_errors=True)
+                options.summary = s.replace('\n', ' ').strip()
 
             if options.guess_description and not options.description:
                 options.description = execute(
-                    ["git", "log", "--pretty=format:%s%n%n%b", revision_range + ".."],
+                    [self.git, "log", "--pretty=format:%s%n%n%b", revision_range + ".."],
                     ignore_errors=True).strip()
 
-            return self.make_diff(revision_range)
+            return (self.make_diff(revision_range), parent_diff_lines)
         else:
             r1, r2 = revision_range.split(":")
+            # Check if parent contains the first revision and make a
+            # parent diff if not:
+            pdiff_required = execute([self.git, "branch", "-r",
+                                      "--contains", r1])
+            parent_diff_lines = None
+
+            if not pdiff_required:
+                parent_diff_lines = self.make_diff(self.merge_base, r1)
 
             if options.guess_summary and not options.summary:
-                options.summary = execute(
-                    ["git", "log", "--pretty=format:%s", "%s..%s" % (r1, r2)],
-                    ignore_errors=True).strip()
+                s = execute([self.git, "log", "--pretty=format:%s",
+                             "%s..%s" % (r1, r2)],
+                            ignore_errors=True)
+                options.summary = s.replace('\n', ' ').strip()
 
             if options.guess_description and not options.description:
                 options.description = execute(
-                    ["git", "log", "--pretty=format:%s%n%n%b", "%s..%s" % (r1, r2)],
+                    [self.git, "log", "--pretty=format:%s%n%n%b", "%s..%s" % (r1, r2)],
                     ignore_errors=True).strip()
 
-            return self.make_diff(r1, r2)
+            return (self.make_diff(r1, r2), parent_diff_lines)
+
+
+class PlasticClient(SCMClient):
+    """
+    A wrapper around the cm Plastic tool that fetches repository
+    information and generates compatible diffs
+    """
+    def get_repository_info(self):
+        if not check_install('cm version'):
+            return None
+
+        # Get the repository that the current directory is from.  If there
+        # is more than one repository mounted in the current directory,
+        # bail out for now (in future, should probably enter a review
+        # request per each repository.)
+        split = execute(["cm", "ls", "--format={8}"], split_lines=True,
+                        ignore_errors=True)
+        m = re.search(r'^rep:(.+)$', split[0], re.M)
+
+        if not m:
+            return None
+
+        # Make sure the repository list contains only one unique entry
+        if len(split) != split.count(split[0]):
+            # Not unique!
+            die('Directory contains more than one mounted repository')
+
+        path = m.group(1)
+
+        # Get the workspace directory, so we can strip it from the diff output
+        self.workspacedir = execute(["cm", "gwp", ".", "--format={1}"],
+                                    split_lines=False,
+                                    ignore_errors=True).strip()
+
+        debug("Workspace is %s" % self.workspacedir)
+
+        return RepositoryInfo(path,
+                              supports_changesets=True,
+                              supports_parent_diffs=False)
+
+    def get_changenum(self, args):
+        """ Extract the integer value from a changeset ID (cs:1234) """
+        if len(args) == 1 and args[0].startswith("cs:"):
+                try:
+                    return str(int(args[0][3:]))
+                except ValueError:
+                    pass
+
+        return None
+
+    def sanitize_changenum(self, changenum):
+        """ Return a "sanitized" change number.  Currently a no-op """
+        return changenum
+
+    def diff(self, args):
+        """
+        Performs a diff across all modified files in a Plastic workspace
+
+        Parent diffs are not supported (the second value in the tuple).
+        """
+        changenum = self.get_changenum(args)
+
+        if changenum is None:
+            return self.branch_diff(args), None
+        else:
+            return self.changenum_diff(changenum), None
+
+    def diff_between_revisions(self, revision_range, args, repository_info):
+        """
+        Performs a diff between 2 revisions of a Plastic repository.
+
+        Assume revision_range is a branch specification (br:/main/task001)
+        and hand over to branch_diff
+        """
+        return (self.branch_diff(revision_range), None)
+
+    def changenum_diff(self, changenum):
+        debug("changenum_diff: %s" % (changenum))
+        files = execute(["cm", "log", "cs:" + changenum,
+                         "--csFormat={items}",
+                         "--itemFormat={shortstatus} {path} "
+                         "rev:revid:{revid} rev:revid:{parentrevid} "
+                         "src:{srccmpath} rev:revid:{srcdirrevid} "
+                         "dst:{dstcmpath} rev:revid:{dstdirrevid}{newline}"],
+                        split_lines = True)
+
+        debug("got files: %s" % (files))
+
+        # Diff generation based on perforce client
+        diff_lines = []
+
+        empty_filename = make_tempfile()
+        tmp_diff_from_filename = make_tempfile()
+        tmp_diff_to_filename = make_tempfile()
+
+        for f in files:
+            f = f.strip()
+
+            if not f:
+                continue
+
+            m = re.search(r'(?P<type>[ACIMR]) (?P<file>.*) '
+                          r'(?P<revspec>rev:revid:[-\d]+) '
+                          r'(?P<parentrevspec>rev:revid:[-\d]+) '
+                          r'src:(?P<srcpath>.*) '
+                          r'(?P<srcrevspec>rev:revid:[-\d]+) '
+                          r'dst:(?P<dstpath>.*) '
+                          r'(?P<dstrevspec>rev:revid:[-\d]+)$',
+                          f)
+            if not m:
+                die("Could not parse 'cm log' response: %s" % f)
+
+            changetype = m.group("type")
+            filename = m.group("file")
+
+            if changetype == "M":
+                # Handle moved files as a delete followed by an add.
+                # Clunky, but at least it works
+                oldfilename = m.group("srcpath")
+                oldspec = m.group("srcrevspec")
+                newfilename = m.group("dstpath")
+                newspec = m.group("dstrevspec")
+
+                self.write_file(oldfilename, oldspec, tmp_diff_from_filename)
+                dl = self.diff_files(tmp_diff_from_filename, empty_filename,
+                                     oldfilename, "rev:revid:-1", oldspec,
+                                     changetype)
+                diff_lines += dl
+
+                self.write_file(newfilename, newspec, tmp_diff_to_filename)
+                dl = self.diff_files(empty_filename, tmp_diff_to_filename,
+                                     newfilename, newspec, "rev:revid:-1",
+                                     changetype)
+                diff_lines += dl
+            else:
+                newrevspec = m.group("revspec")
+                parentrevspec = m.group("parentrevspec")
+
+                debug("Type %s File %s Old %s New %s" % (changetype,
+                                                         filename,
+                                                         parentrevspec,
+                                                         newrevspec))
+
+                old_file = new_file = empty_filename
+
+                if (changetype in ['A'] or
+                    (changetype in ['C', 'I'] and
+                     parentrevspec == "rev:revid:-1")):
+                    # File was Added, or a Change or Merge (type I) and there
+                    # is no parent revision
+                    self.write_file(filename, newrevspec, tmp_diff_to_filename)
+                    new_file = tmp_diff_to_filename
+                elif changetype in ['C', 'I']:
+                    # File was Changed or Merged (type I)
+                    self.write_file(filename, parentrevspec,
+                                    tmp_diff_from_filename)
+                    old_file = tmp_diff_from_filename
+                    self.write_file(filename, newrevspec, tmp_diff_to_filename)
+                    new_file = tmp_diff_to_filename
+                elif changetype in ['R']:
+                    # File was Removed
+                    self.write_file(filename, parentrevspec,
+                                    tmp_diff_from_filename)
+                    old_file = tmp_diff_from_filename
+                else:
+                    die("Don't know how to handle change type '%s' for %s" %
+                        (changetype, filename))
+
+                dl = self.diff_files(old_file, new_file, filename,
+                                     newrevspec, parentrevspec, changetype)
+                diff_lines += dl
+
+        os.unlink(empty_filename)
+        os.unlink(tmp_diff_from_filename)
+        os.unlink(tmp_diff_to_filename)
+
+        return ''.join(diff_lines)
+
+    def branch_diff(self, args):
+        debug("branch diff: %s" % (args))
+
+        if len(args) > 0:
+            branch = args[0]
+        else:
+            branch = args
+
+        if not branch.startswith("br:"):
+            return None
+
+        if not options.branch:
+            options.branch = branch
+
+        files = execute(["cm", "fbc", branch, "--format={3} {4}"],
+                        split_lines = True)
+        debug("got files: %s" % (files))
+
+        diff_lines = []
+
+        empty_filename = make_tempfile()
+        tmp_diff_from_filename = make_tempfile()
+        tmp_diff_to_filename = make_tempfile()
+
+        for f in files:
+            f = f.strip()
+
+            if not f:
+                continue
+
+            m = re.search(r'^(?P<branch>.*)#(?P<revno>\d+) (?P<file>.*)$', f)
+
+            if not m:
+                die("Could not parse 'cm fbc' response: %s" % f)
+
+            filename = m.group("file")
+            branch = m.group("branch")
+            revno = m.group("revno")
+
+            # Get the base revision with a cm find
+            basefiles = execute(["cm", "find", "revs", "where",
+                                 "item='" + filename + "'", "and",
+                                 "branch='" + branch + "'", "and",
+                                 "revno=" + revno,
+                                 "--format={item} rev:revid:{id} "
+                                 "rev:revid:{parent}", "--nototal"],
+                                split_lines = True)
+
+            # We only care about the first line
+            m = re.search(r'^(?P<filename>.*) '
+                              r'(?P<revspec>rev:revid:[-\d]+) '
+                              r'(?P<parentrevspec>rev:revid:[-\d]+)$',
+                              basefiles[0])
+            basefilename = m.group("filename")
+            newrevspec = m.group("revspec")
+            parentrevspec = m.group("parentrevspec")
+
+            # Cope with adds/removes
+            changetype = "C"
+
+            if parentrevspec == "rev:revid:-1":
+                changetype = "A"
+            elif newrevspec == "rev:revid:-1":
+                changetype = "R"
+
+            debug("Type %s File %s Old %s New %s" % (changetype,
+                                                     basefilename,
+                                                     parentrevspec,
+                                                     newrevspec))
+
+            old_file = new_file = empty_filename
+
+            if changetype == "A":
+                # File Added
+                self.write_file(basefilename, newrevspec,
+                                tmp_diff_to_filename)
+                new_file = tmp_diff_to_filename
+            elif changetype == "R":
+                # File Removed
+                self.write_file(basefilename, parentrevspec,
+                                tmp_diff_from_filename)
+                old_file = tmp_diff_from_filename
+            else:
+                self.write_file(basefilename, parentrevspec,
+                                tmp_diff_from_filename)
+                old_file = tmp_diff_from_filename
+
+                self.write_file(basefilename, newrevspec,
+                                tmp_diff_to_filename)
+                new_file = tmp_diff_to_filename
+
+            dl = self.diff_files(old_file, new_file, basefilename,
+                                 newrevspec, parentrevspec, changetype)
+            diff_lines += dl
+
+        os.unlink(empty_filename)
+        os.unlink(tmp_diff_from_filename)
+        os.unlink(tmp_diff_to_filename)
+
+        return ''.join(diff_lines)
+
+    def diff_files(self, old_file, new_file, filename, newrevspec,
+                   parentrevspec, changetype, ignore_unmodified=False):
+        """
+        Do the work of producing a diff for Plastic (based on the Perforce one)
+
+        old_file - The absolute path to the "old" file.
+        new_file - The absolute path to the "new" file.
+        filename - The file in the Plastic workspace
+        newrevspec - The revid spec of the changed file
+        parentrevspecspec - The revision spec of the "old" file
+        changetype - The change type as a single character string
+        ignore_unmodified - If true, will return an empty list if the file
+            is not changed.
+
+        Returns a list of strings of diff lines.
+        """
+        if filename.startswith(self.workspacedir):
+            filename = filename[len(self.workspacedir):]
+
+        diff_cmd = ["diff", "-urN", old_file, new_file]
+        # Diff returns "1" if differences were found.
+        dl = execute(diff_cmd, extra_ignore_errors=(1,2),
+                     translate_newlines = False)
+
+        # If the input file has ^M characters at end of line, lets ignore them.
+        dl = dl.replace('\r\r\n', '\r\n')
+        dl = dl.splitlines(True)
+
+        # Special handling for the output of the diff tool on binary files:
+        #     diff outputs "Files a and b differ"
+        # and the code below expects the output to start with
+        #     "Binary files "
+        if (len(dl) == 1 and
+            dl[0].startswith('Files %s and %s differ' % (old_file, new_file))):
+            dl = ['Binary files %s and %s differ\n' % (old_file, new_file)]
+
+        if dl == [] or dl[0].startswith("Binary files "):
+            if dl == []:
+                if ignore_unmodified:
+                    return []
+                else:
+                    print "Warning: %s in your changeset is unmodified" % \
+                          filename
+
+            dl.insert(0, "==== %s (%s) ==%s==\n" % (filename, newrevspec,
+                                                    changetype))
+            dl.append('\n')
+        else:
+            dl[0] = "--- %s\t%s\n" % (filename, parentrevspec)
+            dl[1] = "+++ %s\t%s\n" % (filename, newrevspec)
+
+            # Not everybody has files that end in a newline.  This ensures
+            # that the resulting diff file isn't broken.
+            if dl[-1][-1] != '\n':
+                dl.append('\n')
+
+        return dl
+
+    def write_file(self, filename, filespec, tmpfile):
+        """ Grabs a file from Plastic and writes it to a temp file """
+        debug("Writing '%s' (rev %s) to '%s'" % (filename, filespec, tmpfile))
+        execute(["cm", "cat", filespec, "--file=" + tmpfile])
 
 
 SCMCLIENTS = (
@@ -2331,6 +3427,7 @@
     MercurialClient(),
     PerforceClient(),
     ClearCaseClient(),
+    PlasticClient(),
 )
 
 def debug(s):
@@ -2341,12 +3438,14 @@
         print ">>> %s" % s
 
 
-def make_tempfile():
+def make_tempfile(content=None):
     """
     Creates a temporary file and returns the path. The path is stored
     in an array for later cleanup.
     """
     fd, tmpfile = mkstemp()
+    if content:
+        os.write(fd, content)
     os.close(fd)
     tempfiles.append(tmpfile)
     return tmpfile
@@ -2360,10 +3459,10 @@
     instance, 'svn help' or 'git --version').
     """
     try:
-        p = subprocess.Popen(command.split(' '),
-                             stdin=subprocess.PIPE,
-                             stdout=subprocess.PIPE,
-                             stderr=subprocess.PIPE)
+        subprocess.Popen(command.split(' '),
+                         stdin=subprocess.PIPE,
+                         stdout=subprocess.PIPE,
+                         stderr=subprocess.PIPE)
         return True
     except OSError:
         return False
@@ -2395,7 +3494,7 @@
 
 
 def execute(command, env=None, split_lines=False, ignore_errors=False,
-            extra_ignore_errors=(), translate_newlines=True):
+            extra_ignore_errors=(), translate_newlines=True, with_errors=True):
     """
     Utility function to execute a command and return the output.
     """
@@ -2412,11 +3511,16 @@
     env['LC_ALL'] = 'en_US.UTF-8'
     env['LANGUAGE'] = 'en_US.UTF-8'
 
+    if with_errors:
+        errors_output = subprocess.STDOUT
+    else:
+        errors_output = subprocess.PIPE
+
     if sys.platform.startswith('win'):
         p = subprocess.Popen(command,
                              stdin=subprocess.PIPE,
                              stdout=subprocess.PIPE,
-                             stderr=subprocess.STDOUT,
+                             stderr=errors_output,
                              shell=False,
                              universal_newlines=translate_newlines,
                              env=env)
@@ -2424,7 +3528,7 @@
         p = subprocess.Popen(command,
                              stdin=subprocess.PIPE,
                              stdout=subprocess.PIPE,
-                             stderr=subprocess.STDOUT,
+                             stderr=errors_output,
                              shell=False,
                              close_fds=True,
                              universal_newlines=translate_newlines,
@@ -2466,22 +3570,33 @@
         path = os.path.dirname(path)
 
 
-def load_config_file(filename):
-    """
-    Loads data from a config file.
-    """
-    config = {
-        'TREES': {},
-    }
-
-    if os.path.exists(filename):
-        try:
-            execfile(filename, config)
-        except SyntaxError, e:
-            die('Syntax error in config file: %s\n'
-                'Line %i offset %i\n' % (filename, e.lineno, e.offset))
-
-    return config
+def load_config_files(homepath):
+    """Loads data from .reviewboardrc files"""
+    def _load_config(path):
+        config = {
+            'TREES': {},
+        }
+
+        filename = os.path.join(path, '.reviewboardrc')
+
+        if os.path.exists(filename):
+            try:
+                execfile(filename, config)
+            except SyntaxError, e:
+                die('Syntax error in config file: %s\n'
+                    'Line %i offset %i\n' % (filename, e.lineno, e.offset))
+
+            return config
+
+        return None
+
+    for path in walk_parents(os.getcwd()):
+        config = _load_config(path)
+
+        if config:
+            configs.append(config)
+
+    globals()['user_config'] = _load_config(homepath)
 
 
 def tempt_fate(server, tool, changenum, diff_content=None,
@@ -2527,6 +3642,10 @@
         if options.testing_done:
             server.set_review_request_field(review_request, 'testing_done',
                                             options.testing_done)
+
+        if options.change_description:
+            server.set_review_request_field(review_request, 'changedescription',
+                                            options.change_description)
     except APIError, e:
         if e.error_code == 103: # Not logged in
             retries = retries - 1
@@ -2569,10 +3688,13 @@
             die("Your review request still exists, but the diff is not " +
                 "attached.")
 
+    if options.reopen:
+        server.reopen(review_request)
+
     if options.publish:
         server.publish(review_request)
 
-    request_url = 'r/' + str(review_request['id'])
+    request_url = 'r/' + str(review_request['id']) + '/'
     review_url = urljoin(server.url, request_url)
 
     if not review_url.startswith('http'):
@@ -2614,6 +3736,10 @@
                       dest="diff_only", action="store_true", default=False,
                       help="uploads a new diff, but does not update "
                            "info from changelist")
+    parser.add_option("--reopen",
+                      dest="reopen", action="store_true", default=False,
+                      help="reopen discarded review request "
+                           "after update")
     parser.add_option("--target-groups",
                       dest="target_groups", default=TARGET_GROUPS,
                       help="names of the groups who will perform "
@@ -2635,12 +3761,12 @@
                       dest="guess_summary", action="store_true",
                       default=False,
                       help="guess summary from the latest commit (git/"
-                           "hgsubversion only)")
+                           "hg/hgsubversion only)")
     parser.add_option("--guess-description",
                       dest="guess_description", action="store_true",
                       default=False,
                       help="guess description based on commits on this branch "
-                           "(git/hgsubversion only)")
+                           "(git/hg/hgsubversion only)")
     parser.add_option("--testing-done",
                       dest="testing_done", default=None,
                       help="details of testing done ")
@@ -2653,13 +3779,13 @@
     parser.add_option("--bugs-closed",
                       dest="bugs_closed", default=None,
                       help="list of bugs closed ")
+    parser.add_option("--change-description", default=None,
+                      help="description of what changed in this revision of "
+                      "the review request when updating an existing request")
     parser.add_option("--revision-range",
                       dest="revision_range", default=None,
                       help="generate the diff for review based on given "
                            "revision range")
-    parser.add_option("--label",
-                      dest="label", default=None,
-                      help="label (ClearCase Only) ")
     parser.add_option("--submit-as",
                       dest="submit_as", default=SUBMIT_AS, metavar="USERNAME",
                       help="user name to be recorded as the author of the "
@@ -2693,12 +3819,21 @@
     parser.add_option("--p4-port",
                       dest="p4_port", default=None,
                       help="the Perforce servers IP address that the review is on")
+    parser.add_option("--p4-passwd",
+                      dest="p4_passwd", default=None,
+                      help="the Perforce password or ticket of the user in the P4USER environment variable")
+    parser.add_option('--svn-changelist', dest='svn_changelist', default=None,
+                      help='generate the diff for review based on a local SVN '
+                           'changelist')
     parser.add_option("--repository-url",
                       dest="repository_url", default=None,
                       help="the url for a repository for creating a diff "
-                           "outside of a working copy (currently only supported "
-                           "by Subversion). Requires either --revision-range"
-                           "or --diff-filename options")
+                           "outside of a working copy (currently only "
+                           "supported by Subversion with --revision-range or "
+                           "--diff-filename and ClearCase with relative "
+                           "paths outside the view). For git, this specifies"
+                           "the origin url of the current repository, "
+                           "overriding the origin url supplied by the git client.")
     parser.add_option("-d", "--debug",
                       action="store_true", dest="debug", default=DEBUG,
                       help="display debug output")
@@ -2706,6 +3841,12 @@
                       dest="diff_filename", default=None,
                       help='upload an existing diff file, instead of '
                            'generating a new diff')
+    parser.add_option('--http-username',
+                      dest='http_username', default=None, metavar='USERNAME',
+                      help='username for HTTP Basic authentication')
+    parser.add_option('--http-password',
+                      dest='http_password', default=None, metavar='PASSWORD',
+                      help='password for HTTP Basic authentication')
 
     (globals()["options"], args) = parser.parse_args(args)
 
@@ -2739,12 +3880,14 @@
                              options.testing_file)
             sys.exit(1)
 
-    if (options.repository_url and
-        not options.revision_range and
-        not options.diff_filename):
-        sys.stderr.write("The --repository-url option requires either the "
-                         "--revision-range option or the --diff-filename "
-                         "option.\n")
+    if options.reopen and not options.rid:
+        sys.stderr.write("The --reopen option requires "
+                         "--review-request-id option.\n")
+        sys.exit(1)
+
+    if options.change_description and not options.rid:
+        sys.stderr.write("--change-description may only be used "
+                         "when updating an existing review-request\n")
         sys.exit(1)
 
     return args
@@ -2800,14 +3943,19 @@
         homepath = ''
 
     # Load the config and cookie files
-    globals()['user_config'] = \
-        load_config_file(os.path.join(homepath, ".reviewboardrc"))
     cookie_file = os.path.join(homepath, ".post-review-cookies.txt")
+    load_config_files(homepath)
 
     args = parse_options(sys.argv[1:])
 
+    debug('RBTools %s' % get_version_string())
+    debug('Home = %s' % homepath)
+
     repository_info, tool = determine_client()
 
+    # Verify that options specific to an SCM Client have not been mis-used.
+    tool.check_options()
+
     # Try to find a valid Review Board server to use.
     if options.server:
         server_url = options.server
@@ -2820,17 +3968,20 @@
 
     server = ReviewBoardServer(server_url, repository_info, cookie_file)
 
+    # Handle the case where /api/ requires authorization (RBCommons).
+    if not server.check_api_version():
+        die("Unable to log in with the supplied username and password.")
+
     if repository_info.supports_changesets:
         changenum = tool.get_changenum(args)
     else:
         changenum = None
 
     if options.revision_range:
-        diff = tool.diff_between_revisions(options.revision_range, args,
-                                           repository_info)
-        parent_diff = None
-    elif options.label and isinstance(tool, ClearCaseClient):
-        diff, parent_diff = tool.diff_label(options.label)
+        diff, parent_diff = tool.diff_between_revisions(options.revision_range, args,
+                                                        repository_info)
+    elif options.svn_changelist:
+        diff, parent_diff = tool.diff_changelist(options.svn_changelist)
     elif options.diff_filename:
         parent_diff = None
 
@@ -2846,11 +3997,24 @@
     else:
         diff, parent_diff = tool.diff(args)
 
-    if isinstance(tool, PerforceClient) and changenum is not None:
+    if len(diff) == 0:
+        die("There don't seem to be any diffs!")
+
+    if (isinstance(tool, PerforceClient) or
+        isinstance(tool, PlasticClient)) and changenum is not None:
         changenum = tool.sanitize_changenum(changenum)
 
+        # NOTE: In Review Board 1.5.2 through 1.5.3.1, the changenum support
+        #       is broken, so we have to force the deprecated API.
+        if (parse_version(server.rb_version) >= parse_version('1.5.2') and
+            parse_version(server.rb_version) <= parse_version('1.5.3.1')):
+            debug('Using changenums on Review Board %s, which is broken. '
+                  'Falling back to the deprecated 1.0 API' % server.rb_version)
+            server.deprecated_api = True
+
     if options.output_diff_only:
-        print diff
+        # The comma here isn't a typo, but rather suppresses the extra newline
+        print diff,
         sys.exit(0)
 
     # Let's begin.

=== modified file 'rbtools/tests.py'
--- rbtools/tests.py	2010-07-31 18:31:05 +0000
+++ rbtools/tests.py	2011-11-06 07:43:24 +0000
@@ -1,10 +1,13 @@
-import nose
 import os
+import re
 import shutil
 import sys
 import tempfile
+import time
 import unittest
 import urllib2
+from random import randint
+from textwrap import dedent
 
 try:
     from cStringIO import StringIO
@@ -16,66 +19,17 @@
 except ImportError:
     import simplejson as json
 
-from rbtools.postreview import execute, load_config_file
-from rbtools.postreview import APIError, GitClient, RepositoryInfo, \
-                               ReviewBoardServer
+import nose
+
+from rbtools.postreview import execute, load_config_files
+from rbtools.postreview import APIError, GitClient, MercurialClient, \
+                               RepositoryInfo, ReviewBoardServer, \
+                               SvnRepositoryInfo
 import rbtools.postreview
 
 
-FOO = """\
-ARMA virumque cano, Troiae qui primus ab oris
-Italiam, fato profugus, Laviniaque venit
-litora, multum ille et terris iactatus et alto
-vi superum saevae memorem Iunonis ob iram;
-multa quoque et bello passus, dum conderet urbem,
-inferretque deos Latio, genus unde Latinum,
-Albanique patres, atque altae moenia Romae.
-Musa, mihi causas memora, quo numine laeso,
-quidve dolens, regina deum tot volvere casus
-insignem pietate virum, tot adire labores
-impulerit. Tantaene animis caelestibus irae?
-
-"""
-
-FOO1 = """\
-ARMA virumque cano, Troiae qui primus ab oris
-Italiam, fato profugus, Laviniaque venit
-litora, multum ille et terris iactatus et alto
-vi superum saevae memorem Iunonis ob iram;
-multa quoque et bello passus, dum conderet urbem,
-inferretque deos Latio, genus unde Latinum,
-Albanique patres, atque altae moenia Romae.
-Musa, mihi causas memora, quo numine laeso,
-
-"""
-
-FOO2 = """\
-ARMA virumque cano, Troiae qui primus ab oris
-ARMA virumque cano, Troiae qui primus ab oris
-ARMA virumque cano, Troiae qui primus ab oris
-Italiam, fato profugus, Laviniaque venit
-litora, multum ille et terris iactatus et alto
-vi superum saevae memorem Iunonis ob iram;
-multa quoque et bello passus, dum conderet urbem,
-inferretque deos Latio, genus unde Latinum,
-Albanique patres, atque altae moenia Romae.
-Musa, mihi causas memora, quo numine laeso,
-
-"""
-
-FOO3 = """\
-ARMA virumque cano, Troiae qui primus ab oris
-ARMA virumque cano, Troiae qui primus ab oris
-Italiam, fato profugus, Laviniaque venit
-litora, multum ille et terris iactatus et alto
-vi superum saevae memorem Iunonis ob iram;
-dum conderet urbem,
-inferretque deos Latio, genus unde Latinum,
-Albanique patres, atque altae moenia Romae.
-Albanique patres, atque altae moenia Romae.
-Musa, mihi causas memora, quo numine laeso,
-
-"""
+TEMPDIR_SUFFIX = '__' + __name__.replace('.', '_')
+
 
 def is_exe_in_path(name):
     """Checks whether an executable is in the user's search path.
@@ -99,9 +53,17 @@
     return False
 
 
+def _get_tmpdir():
+    return tempfile.mkdtemp(TEMPDIR_SUFFIX)
+
+
 class MockHttpUnitTest(unittest.TestCase):
+    deprecated_api = False
+
     def setUp(self):
         # Save the old http_get and http_post
+        rbtools.postreview.options = OptionsStub()
+
         self.saved_http_get = ReviewBoardServer.http_get
         self.saved_http_post = ReviewBoardServer.http_post
 
@@ -110,19 +72,23 @@
         ReviewBoardServer.http_get = self._http_method
         ReviewBoardServer.http_post = self._http_method
 
-        self.http_response = ""
-
-        rbtools.postreview.options = OptionsStub()
+        self.server.deprecated_api = self.deprecated_api
+        self.http_response = {}
 
     def tearDown(self):
         ReviewBoardServer.http_get = self.saved_http_get
         ReviewBoardServer.http_post = self.saved_http_post
 
-    def _http_method(self, *args, **kwargs):
-        if isinstance(self.http_response, Exception):
-            raise self.http_response
-        else:
-            return self.http_response
+    def _http_method(self, path, *args, **kwargs):
+        if isinstance(self.http_response, dict):
+            http_response = self.http_response[path]
+        else:
+            http_response = self.http_response
+
+        if isinstance(http_response, Exception):
+            raise http_response
+        else:
+            return http_response
 
 
 class OptionsStub(object):
@@ -131,6 +97,9 @@
         self.guess_summary = False
         self.guess_description = False
         self.tracking = None
+        self.username = None
+        self.password = None
+        self.repository_url = None
 
 
 class GitClientTests(unittest.TestCase):
@@ -165,7 +134,7 @@
 
         self.orig_dir = os.getcwd()
 
-        self.git_dir = tempfile.mkdtemp()
+        self.git_dir = _get_tmpdir()
         os.chdir(self.git_dir)
         self._gitcmd(['init'], git_dir=self.git_dir)
         foo = open(os.path.join(self.git_dir, 'foo.txt'), 'w')
@@ -175,13 +144,14 @@
         self._gitcmd(['add', 'foo.txt'])
         self._gitcmd(['commit', '-m', 'initial commit'])
 
-        self.clone_dir = tempfile.mkdtemp()
+        self.clone_dir = _get_tmpdir()
         os.rmdir(self.clone_dir)
         self._gitcmd(['clone', self.git_dir, self.clone_dir])
         self.client = GitClient()
         os.chdir(self.orig_dir)
 
-        rbtools.postreview.user_config = load_config_file('')
+        rbtools.postreview.user_config = {}
+        rbtools.postreview.configs = []
         rbtools.postreview.options = OptionsStub()
         rbtools.postreview.options.parent_branch = None
 
@@ -214,13 +184,14 @@
         rc = open(os.path.join(self.clone_dir, '.reviewboardrc'), 'w')
         rc.write('REVIEWBOARD_URL = "%s"' % self.TESTSERVER)
         rc.close()
+        rbtools.postreview.user_config = load_config_files(self.clone_dir)
 
         ri = self.client.get_repository_info()
         server = self.client.scan_for_server(ri)
         self.assertEqual(server, self.TESTSERVER)
 
     def test_scan_for_server_property(self):
-        """Test GitClientscan_for_server using repo property"""
+        """Test GitClient scan_for_server using repo property"""
         os.chdir(self.clone_dir)
         self._gitcmd(['config', 'reviewboard.url', self.TESTSERVER])
         ri = self.client.get_repository_info()
@@ -243,7 +214,7 @@
                " \n"
 
         os.chdir(self.clone_dir)
-        ri = self.client.get_repository_info()
+        self.client.get_repository_info()
 
         self._git_add_file_commit('foo.txt', FOO1, 'delete and modify stuff')
 
@@ -273,7 +244,7 @@
                " \n"
 
         os.chdir(self.clone_dir)
-        ri = self.client.get_repository_info()
+        self.client.get_repository_info()
 
         self._git_add_file_commit('foo.txt', FOO1, 'commit 1')
         self._git_add_file_commit('foo.txt', FOO2, 'commit 1')
@@ -323,11 +294,11 @@
         self._gitcmd(['checkout', '-b', 'mybranch', '--track', 'origin/master'])
         self._git_add_file_commit('foo.txt', FOO2, 'commit 2')
 
-        ri = self.client.get_repository_info()
+        self.client.get_repository_info()
         self.assertEqual(self.client.diff(None), (diff1, None))
 
         self._gitcmd(['checkout', 'master'])
-        ri = self.client.get_repository_info()
+        self.client.get_repository_info()
         self.assertEqual(self.client.diff(None), (diff2, None))
 
     def test_diff_tracking_no_origin(self):
@@ -352,7 +323,7 @@
         self._gitcmd(['checkout', '-b', 'mybranch', '--track', 'quux/master'])
         self._git_add_file_commit('foo.txt', FOO1, 'delete and modify stuff')
 
-        ri = self.client.get_repository_info()
+        self.client.get_repository_info()
 
         self.assertEqual(self.client.diff(None), (diff, None))
 
@@ -385,7 +356,7 @@
         self._gitcmd(['checkout', '-b', 'mybranch', '--track', 'master'])
         self._git_add_file_commit('foo.txt', FOO2, 'commit 2')
 
-        ri = self.client.get_repository_info()
+        self.client.get_repository_info()
         self.assertEqual(self.client.diff(None), (diff, None))
 
     def test_diff_tracking_override(self):
@@ -412,11 +383,478 @@
 
         self._git_add_file_commit('foo.txt', FOO1, 'commit 1')
 
-        ri = self.client.get_repository_info()
-        self.assertEqual(self.client.diff(None), (diff, None))
+        self.client.get_repository_info()
+        self.assertEqual(self.client.diff(None), (diff, None))
+
+    def test_diff_slash_tracking(self):
+        """Test GitClient diff with tracking branch that has slash in its name"""
+        diff = "diff --git a/foo.txt b/foo.txt\n" \
+               "index 5e98e9540e1b741b5be24fcb33c40c1c8069c1fb..e619c1387f5feb91f0ca83194650bfe4f6c2e347 100644\n" \
+               "--- a/foo.txt\n" \
+               "+++ b/foo.txt\n" \
+               "@@ -1,4 +1,6 @@\n" \
+               " ARMA virumque cano, Troiae qui primus ab oris\n" \
+               "+ARMA virumque cano, Troiae qui primus ab oris\n" \
+               "+ARMA virumque cano, Troiae qui primus ab oris\n" \
+               " Italiam, fato profugus, Laviniaque venit\n" \
+               " litora, multum ille et terris iactatus et alto\n" \
+               " vi superum saevae memorem Iunonis ob iram;\n"
+
+        os.chdir(self.git_dir)
+        self._gitcmd(['checkout', '-b', 'not-master'])
+        self._git_add_file_commit('foo.txt', FOO1, 'commit 1')
+
+        os.chdir(self.clone_dir)
+        self._gitcmd(['fetch', 'origin'])
+        self._gitcmd(['checkout', '-b', 'my/branch', '--track', 'origin/not-master'])
+        self._git_add_file_commit('foo.txt', FOO2, 'commit 2')
+
+        self.client.get_repository_info()
+        self.assertEqual(self.client.diff(None), (diff, None))
+
+
+class MercurialTestBase(unittest.TestCase):
+
+    def setUp(self):
+        self._hg_env = {}
+
+    def _hgcmd(self, command, split_lines=False,
+                ignore_errors=False, extra_ignore_errors=(),
+                translate_newlines=True, hg_dir=None):
+        if hg_dir:
+            full_command = ['hg', '--cwd', hg_dir]
+        else:
+            full_command = ['hg']
+
+        # We're *not* doing `env = env or {}` here because
+        # we want the caller to be able to *enable* reading
+        # of user and system-level hgrc configuration.
+        env = self._hg_env.copy()
+
+        if not env:
+            env = {
+                'HGRCPATH': os.devnull,
+                'HGPLAIN': '1',
+            }
+
+        full_command.extend(command)
+
+        return execute(full_command, env, split_lines, ignore_errors,
+                       extra_ignore_errors, translate_newlines)
+
+    def _hg_add_file_commit(self, filename, data, msg):
+        outfile = open(filename, 'w')
+        outfile.write(data)
+        outfile.close()
+        self._hgcmd(['add', filename])
+        self._hgcmd(['commit', '-m', msg])
+
+
+class MercurialClientTests(MercurialTestBase):
+    TESTSERVER = 'http://127.0.0.1:8080'
+    CLONE_HGRC = dedent("""
+    [paths]
+    default = %(hg_dir)s
+    cloned = %(clone_dir)s
+
+    [reviewboard]
+    url = %(test_server)s
+
+    [diff]
+    git = true
+    """).rstrip()
+
+    def setUp(self):
+        MercurialTestBase.setUp(self)
+        if not is_exe_in_path('hg'):
+            raise nose.SkipTest('hg not found in path')
+
+        self.orig_dir = os.getcwd()
+
+        self.hg_dir = _get_tmpdir()
+        os.chdir(self.hg_dir)
+        self._hgcmd(['init'], hg_dir=self.hg_dir)
+        foo = open(os.path.join(self.hg_dir, 'foo.txt'), 'w')
+        foo.write(FOO)
+        foo.close()
+
+        self._hgcmd(['add', 'foo.txt'])
+        self._hgcmd(['commit', '-m', 'initial commit'])
+
+        self.clone_dir = _get_tmpdir()
+        os.rmdir(self.clone_dir)
+        self._hgcmd(['clone', self.hg_dir, self.clone_dir])
+        os.chdir(self.clone_dir)
+        self.client = MercurialClient()
+
+        clone_hgrc = open(self.clone_hgrc_path, 'wb')
+        clone_hgrc.write(self.CLONE_HGRC % {
+            'hg_dir': self.hg_dir,
+            'clone_dir': self.clone_dir,
+            'test_server': self.TESTSERVER,
+        })
+        clone_hgrc.close()
+
+        self.client.get_repository_info()
+        rbtools.postreview.user_config = {}
+        rbtools.postreview.options = OptionsStub()
+        rbtools.postreview.options.parent_branch = None
+        os.chdir(self.clone_dir)
+
+    @property
+    def clone_hgrc_path(self):
+        return os.path.join(self.clone_dir, '.hg', 'hgrc')
+
+    @property
+    def hgrc_path(self):
+        return os.path.join(self.hg_dir, '.hg', 'hgrc')
+
+    def tearDown(self):
+        os.chdir(self.orig_dir)
+        shutil.rmtree(self.hg_dir)
+        shutil.rmtree(self.clone_dir)
+
+    def testGetRepositoryInfoSimple(self):
+        """Test MercurialClient get_repository_info, simple case"""
+        ri = self.client.get_repository_info()
+
+        self.assertTrue(isinstance(ri, RepositoryInfo))
+        self.assertEqual('', ri.base_path)
+
+        hgpath = ri.path
+
+        if os.path.basename(hgpath) == '.hg':
+            hgpath = os.path.dirname(hgpath)
+
+        self.assertEqual(self.hg_dir, hgpath)
+        self.assertTrue(ri.supports_parent_diffs)
+        self.assertFalse(ri.supports_changesets)
+
+    def testScanForServerSimple(self):
+        """Test MercurialClient scan_for_server, simple case"""
+        os.rename(self.clone_hgrc_path,
+            os.path.join(self.clone_dir, '._disabled_hgrc'))
+
+        self.client.hgrc = {}
+        self.client._load_hgrc()
+        ri = self.client.get_repository_info()
+
+        server = self.client.scan_for_server(ri)
+        self.assertTrue(server is None)
+
+    def testScanForServerWhenPresentInHgrc(self):
+        """Test MercurialClient scan_for_server when present in hgrc"""
+        ri = self.client.get_repository_info()
+
+        server = self.client.scan_for_server(ri)
+        self.assertEqual(self.TESTSERVER, server)
+
+    def testScanForServerReviewboardrc(self):
+        """Test MercurialClient scan_for_server when in .reviewboardrc"""
+        rc = open(os.path.join(self.clone_dir, '.reviewboardrc'), 'w')
+        rc.write('REVIEWBOARD_URL = "%s"' % self.TESTSERVER)
+        rc.close()
+
+        ri = self.client.get_repository_info()
+        server = self.client.scan_for_server(ri)
+        self.assertEqual(self.TESTSERVER, server)
+
+    def testDiffSimple(self):
+        """Test MercurialClient diff, simple case"""
+        self.client.get_repository_info()
+
+        self._hg_add_file_commit('foo.txt', FOO1, 'delete and modify stuff')
+
+        diff_result = self.client.diff(None)
+        self.assertEqual((EXPECTED_HG_DIFF_0, None), diff_result)
+
+    def testDiffSimpleMultiple(self):
+        """Test MercurialClient diff with multiple commits"""
+        self.client.get_repository_info()
+
+        self._hg_add_file_commit('foo.txt', FOO1, 'commit 1')
+        self._hg_add_file_commit('foo.txt', FOO2, 'commit 2')
+        self._hg_add_file_commit('foo.txt', FOO3, 'commit 3')
+
+        diff_result = self.client.diff(None)
+
+        self.assertEqual((EXPECTED_HG_DIFF_1, None), diff_result)
+
+    def testDiffBranchDiverge(self):
+        """Test MercurialClient diff with diverged branch"""
+        self._hg_add_file_commit('foo.txt', FOO1, 'commit 1')
+
+        self._hgcmd(['branch', 'diverged'])
+        self._hg_add_file_commit('foo.txt', FOO2, 'commit 2')
+        self.client.get_repository_info()
+
+        self.assertEqual((EXPECTED_HG_DIFF_2, None), self.client.diff(None))
+
+        self._hgcmd(['update', '-C', 'default'])
+        self.client.get_repository_info()
+
+        self.assertEqual((EXPECTED_HG_DIFF_3, None), self.client.diff(None))
+
+
+class MercurialSubversionClientTests(MercurialTestBase):
+    TESTSERVER = "http://127.0.0.1:8080"
+
+    def __init__(self, *args, **kwargs):
+        self._tmpbase = ''
+        self.clone_dir = ''
+        self.svn_repo = ''
+        self.svn_checkout = ''
+        self.client = None
+        self._svnserve_pid = 0
+        self._max_svnserve_pid_tries = 12
+        self._svnserve_port = os.environ.get('SVNSERVE_PORT')
+        self._required_exes = ('svnadmin', 'svnserve', 'svn')
+        MercurialTestBase.__init__(self, *args, **kwargs)
+
+    def setUp(self):
+        MercurialTestBase.setUp(self)
+        self._hg_env = {'FOO': 'BAR'}
+
+        for exe in self._required_exes:
+            if not is_exe_in_path(exe):
+                raise nose.SkipTest('missing svn stuff!  giving up!')
+
+        if not self._has_hgsubversion():
+            raise nose.SkipTest('unable to use `hgsubversion` extension!  '
+                                'giving up!')
+
+        if not self._tmpbase:
+            self._tmpbase = _get_tmpdir()
+
+        self._create_svn_repo()
+        self._fire_up_svnserve()
+        self._fill_in_svn_repo()
+
+        try:
+            self._get_testing_clone()
+        except (OSError, IOError):
+            msg = 'could not clone from svn repo!  skipping...'
+            raise nose.SkipTest(msg), None, sys.exc_info()[2]
+
+        self._spin_up_client()
+        self._stub_in_config_and_options()
+        os.chdir(self.clone_dir)
+
+    def _has_hgsubversion(self):
+        output = self._hgcmd(['svn', '--help'],
+                             ignore_errors=True, extra_ignore_errors=(255))
+
+        return not re.search("unknown command ['\"]svn['\"]", output, re.I)
+
+    def tearDown(self):
+        shutil.rmtree(self.clone_dir)
+        os.kill(self._svnserve_pid, 9)
+
+        if self._tmpbase:
+            shutil.rmtree(self._tmpbase)
+
+    def _svn_add_file_commit(self, filename, data, msg):
+        outfile = open(filename, 'w')
+        outfile.write(data)
+        outfile.close()
+        execute(['svn', 'add', filename])
+        execute(['svn', 'commit', '-m', msg])
+
+    def _create_svn_repo(self):
+        self.svn_repo = os.path.join(self._tmpbase, 'svnrepo')
+        execute(['svnadmin', 'create', self.svn_repo])
+
+    def _fire_up_svnserve(self):
+        if not self._svnserve_port:
+            self._svnserve_port = str(randint(30000, 40000))
+
+        pid_file = os.path.join(self._tmpbase, 'svnserve.pid')
+        execute(['svnserve', '--pid-file', pid_file, '-d',
+                 '--listen-port', self._svnserve_port, '-r', self._tmpbase])
+
+        for i in range(0, self._max_svnserve_pid_tries):
+            try:
+                self._svnserve_pid = int(open(pid_file).read().strip())
+                return
+
+            except (IOError, OSError):
+                time.sleep(0.25)
+
+        # This will re-raise the last exception, which will be either
+        # IOError or OSError if the above fails and this branch is reached
+        raise
+
+    def _fill_in_svn_repo(self):
+        self.svn_checkout = os.path.join(self._tmpbase, 'checkout.svn')
+        execute(['svn', 'checkout', 'file://%s' % self.svn_repo,
+                 self.svn_checkout])
+        os.chdir(self.svn_checkout)
+
+        for subtree in ('trunk', 'branches', 'tags'):
+            execute(['svn', 'mkdir', subtree])
+
+        execute(['svn', 'commit', '-m', 'filling in T/b/t'])
+        os.chdir(os.path.join(self.svn_checkout, 'trunk'))
+
+        for i, data in enumerate([FOO, FOO1, FOO2]):
+            self._svn_add_file_commit('foo.txt', data, 'foo commit %s' % i)
+
+    def _get_testing_clone(self):
+        self.clone_dir = os.path.join(self._tmpbase, 'checkout.hg')
+        self._hgcmd([
+            'clone', 'svn://127.0.0.1:%s/svnrepo' % self._svnserve_port,
+            self.clone_dir,
+        ])
+
+    def _spin_up_client(self):
+        os.chdir(self.clone_dir)
+        self.client = MercurialClient()
+
+    def _stub_in_config_and_options(self):
+        rbtools.postreview.user_config = {}
+        rbtools.postreview.options = OptionsStub()
+        rbtools.postreview.options.parent_branch = None
+
+    def testGetRepositoryInfoSimple(self):
+        """Test MercurialClient (+svn) get_repository_info, simple case"""
+        ri = self.client.get_repository_info()
+
+        self.assertEqual('svn', self.client._type)
+        self.assertEqual('/trunk', ri.base_path)
+        self.assertEqual('svn://127.0.0.1:%s/svnrepo' % self._svnserve_port,
+                        ri.path)
+
+    def testScanForServerSimple(self):
+        """Test MercurialClient (+svn) scan_for_server, simple case"""
+        ri = self.client.get_repository_info()
+        server = self.client.scan_for_server(ri)
+
+        self.assertTrue(server is None)
+
+    def testScanForServerReviewboardrc(self):
+        """Test MercurialClient (+svn) scan_for_server in .reviewboardrc"""
+        rc_filename = os.path.join(self.clone_dir, '.reviewboardrc')
+        rc = open(rc_filename, 'w')
+        rc.write('REVIEWBOARD_URL = "%s"' % self.TESTSERVER)
+        rc.close()
+
+        ri = self.client.get_repository_info()
+        server = self.client.scan_for_server(ri)
+
+        self.assertEqual(self.TESTSERVER, server)
+
+    def testScanForServerProperty(self):
+        """Test MercurialClient (+svn) scan_for_server in svn property"""
+        os.chdir(self.svn_checkout)
+        execute(['svn', 'update'])
+        execute(['svn', 'propset', 'reviewboard:url', self.TESTSERVER,
+                 self.svn_checkout])
+        execute(['svn', 'commit', '-m', 'adding reviewboard:url property'])
+
+        os.chdir(self.clone_dir)
+        self._hgcmd(['pull'])
+        self._hgcmd(['update', '-C'])
+
+        ri = self.client.get_repository_info()
+
+        self.assertEqual(self.TESTSERVER, self.client.scan_for_server(ri))
+
+    def testDiffSimple(self):
+        """Test MercurialClient (+svn) diff, simple case"""
+        self.client.get_repository_info()
+
+        self._hg_add_file_commit('foo.txt', FOO4, 'edit 4')
+
+        self.assertEqual(EXPECTED_HG_SVN_DIFF_0, self.client.diff(None)[0])
+
+    def testDiffSimpleMultiple(self):
+        """Test MercurialClient (+svn) diff with multiple commits"""
+        self.client.get_repository_info()
+
+        self._hg_add_file_commit('foo.txt', FOO4, 'edit 4')
+        self._hg_add_file_commit('foo.txt', FOO5, 'edit 5')
+        self._hg_add_file_commit('foo.txt', FOO6, 'edit 6')
+
+        self.assertEqual(EXPECTED_HG_SVN_DIFF_1, self.client.diff(None)[0])
+
+
+class SVNClientTests(unittest.TestCase):
+    def test_relative_paths(self):
+        """Testing SvnRepositoryInfo._get_relative_path"""
+        info = SvnRepositoryInfo('http://svn.example.com/svn/', '/', '')
+        self.assertEqual(info._get_relative_path('/foo', '/bar'), None)
+        self.assertEqual(info._get_relative_path('/', '/trunk/myproject'),
+                         None)
+        self.assertEqual(info._get_relative_path('/trunk/myproject', '/'),
+                         '/trunk/myproject')
+        self.assertEqual(
+            info._get_relative_path('/trunk/myproject', ''),
+            '/trunk/myproject')
+        self.assertEqual(
+            info._get_relative_path('/trunk/myproject', '/trunk'),
+            '/myproject')
+        self.assertEqual(
+            info._get_relative_path('/trunk/myproject', '/trunk/myproject'),
+            '/')
 
 
 class ApiTests(MockHttpUnitTest):
+    def setUp(self):
+        super(ApiTests, self).setUp()
+
+        self.http_response = {
+            'api/': json.dumps({
+                'stat': 'ok',
+                'links': {
+                    'info': {
+                        'href': 'api/info/',
+                        'method': 'GET',
+                    },
+                },
+            }),
+        }
+
+    def test_check_api_version_1_5_2_higher(self):
+        """Testing checking the API version compatibility (RB >= 1.5.2)"""
+        self.http_response.update(self._build_info_resource('1.5.2'))
+        self.server.check_api_version()
+        self.assertFalse(self.server.deprecated_api)
+
+        self.http_response.update(self._build_info_resource('1.5.3alpha0'))
+        self.server.check_api_version()
+        self.assertFalse(self.server.deprecated_api)
+
+    def test_check_api_version_1_5_1_lower(self):
+        """Testing checking the API version compatibility (RB < 1.5.2)"""
+        self.http_response.update(self._build_info_resource('1.5.1'))
+        self.server.check_api_version()
+        self.assertTrue(self.server.deprecated_api)
+
+    def test_check_api_version_old_api(self):
+        """Testing checking the API version compatibility (RB < 1.5.0)"""
+        self.http_response = {
+            'api/': APIError(404, 0),
+        }
+
+        self.server.check_api_version()
+        self.assertTrue(self.server.deprecated_api)
+
+    def _build_info_resource(self, package_version):
+        return {
+            'api/info/': json.dumps({
+                'stat': 'ok',
+                'info': {
+                    'product': {
+                        'package_version': package_version,
+                    },
+                },
+            }),
+        }
+
+
+class DeprecatedApiTests(MockHttpUnitTest):
+    deprecated_api = True
+
     SAMPLE_ERROR_STR = json.dumps({
         'stat': 'fail',
         'err': {
@@ -429,7 +867,7 @@
         self.http_response = self.SAMPLE_ERROR_STR
 
         try:
-            data = self.server.api_get('/foo/')
+            self.server.api_get('/foo/')
 
             # Shouldn't be reached
             self._assert(False)
@@ -444,7 +882,7 @@
         self.http_response = self.SAMPLE_ERROR_STR
 
         try:
-            data = self.server.api_post('/foo/')
+            self.server.api_post('/foo/')
 
             # Shouldn't be reached
             self._assert(False)
@@ -460,7 +898,7 @@
                                                    self.SAMPLE_ERROR_STR)
 
         try:
-            data = self.server.api_get('/foo/')
+            self.server.api_get('/foo/')
 
             # Shouldn't be reached
             self._assert(False)
@@ -476,7 +914,7 @@
                                                    self.SAMPLE_ERROR_STR)
 
         try:
-            data = self.server.api_post('/foo/')
+            self.server.api_post('/foo/')
 
             # Shouldn't be reached
             self._assert(False)
@@ -489,3 +927,207 @@
 
     def _make_http_error(self, url, code, body):
         return urllib2.HTTPError(url, code, body, {}, StringIO(body))
+
+
+FOO = """\
+ARMA virumque cano, Troiae qui primus ab oris
+Italiam, fato profugus, Laviniaque venit
+litora, multum ille et terris iactatus et alto
+vi superum saevae memorem Iunonis ob iram;
+multa quoque et bello passus, dum conderet urbem,
+inferretque deos Latio, genus unde Latinum,
+Albanique patres, atque altae moenia Romae.
+Musa, mihi causas memora, quo numine laeso,
+quidve dolens, regina deum tot volvere casus
+insignem pietate virum, tot adire labores
+impulerit. Tantaene animis caelestibus irae?
+
+"""
+
+FOO1 = """\
+ARMA virumque cano, Troiae qui primus ab oris
+Italiam, fato profugus, Laviniaque venit
+litora, multum ille et terris iactatus et alto
+vi superum saevae memorem Iunonis ob iram;
+multa quoque et bello passus, dum conderet urbem,
+inferretque deos Latio, genus unde Latinum,
+Albanique patres, atque altae moenia Romae.
+Musa, mihi causas memora, quo numine laeso,
+
+"""
+
+FOO2 = """\
+ARMA virumque cano, Troiae qui primus ab oris
+ARMA virumque cano, Troiae qui primus ab oris
+ARMA virumque cano, Troiae qui primus ab oris
+Italiam, fato profugus, Laviniaque venit
+litora, multum ille et terris iactatus et alto
+vi superum saevae memorem Iunonis ob iram;
+multa quoque et bello passus, dum conderet urbem,
+inferretque deos Latio, genus unde Latinum,
+Albanique patres, atque altae moenia Romae.
+Musa, mihi causas memora, quo numine laeso,
+
+"""
+
+FOO3 = """\
+ARMA virumque cano, Troiae qui primus ab oris
+ARMA virumque cano, Troiae qui primus ab oris
+Italiam, fato profugus, Laviniaque venit
+litora, multum ille et terris iactatus et alto
+vi superum saevae memorem Iunonis ob iram;
+dum conderet urbem,
+inferretque deos Latio, genus unde Latinum,
+Albanique patres, atque altae moenia Romae.
+Albanique patres, atque altae moenia Romae.
+Musa, mihi causas memora, quo numine laeso,
+
+"""
+
+FOO4 = """\
+Italiam, fato profugus, Laviniaque venit
+litora, multum ille et terris iactatus et alto
+vi superum saevae memorem Iunonis ob iram;
+dum conderet urbem,
+
+
+
+
+
+inferretque deos Latio, genus unde Latinum,
+Albanique patres, atque altae moenia Romae.
+Musa, mihi causas memora, quo numine laeso,
+
+"""
+
+FOO5 = """\
+litora, multum ille et terris iactatus et alto
+Italiam, fato profugus, Laviniaque venit
+vi superum saevae memorem Iunonis ob iram;
+dum conderet urbem,
+Albanique patres, atque altae moenia Romae.
+Albanique patres, atque altae moenia Romae.
+Musa, mihi causas memora, quo numine laeso,
+inferretque deos Latio, genus unde Latinum,
+
+ARMA virumque cano, Troiae qui primus ab oris
+ARMA virumque cano, Troiae qui primus ab oris
+"""
+
+FOO6 = """\
+ARMA virumque cano, Troiae qui primus ab oris
+ARMA virumque cano, Troiae qui primus ab oris
+Italiam, fato profugus, Laviniaque venit
+litora, multum ille et terris iactatus et alto
+vi superum saevae memorem Iunonis ob iram;
+dum conderet urbem, inferretque deos Latio, genus
+unde Latinum, Albanique patres, atque altae
+moenia Romae. Albanique patres, atque altae
+moenia Romae. Musa, mihi causas memora, quo numine laeso,
+
+"""
+
+EXPECTED_HG_DIFF_0 = """\
+diff --git a/foo.txt b/foo.txt
+--- a/foo.txt
++++ b/foo.txt
+@@ -6,7 +6,4 @@
+ inferretque deos Latio, genus unde Latinum,
+ Albanique patres, atque altae moenia Romae.
+ Musa, mihi causas memora, quo numine laeso,
+-quidve dolens, regina deum tot volvere casus
+-insignem pietate virum, tot adire labores
+-impulerit. Tantaene animis caelestibus irae?
+ 
+"""
+
+EXPECTED_HG_DIFF_1 = """\
+diff --git a/foo.txt b/foo.txt
+--- a/foo.txt
++++ b/foo.txt
+@@ -1,12 +1,11 @@
++ARMA virumque cano, Troiae qui primus ab oris
+ ARMA virumque cano, Troiae qui primus ab oris
+ Italiam, fato profugus, Laviniaque venit
+ litora, multum ille et terris iactatus et alto
+ vi superum saevae memorem Iunonis ob iram;
+-multa quoque et bello passus, dum conderet urbem,
++dum conderet urbem,
+ inferretque deos Latio, genus unde Latinum,
+ Albanique patres, atque altae moenia Romae.
++Albanique patres, atque altae moenia Romae.
+ Musa, mihi causas memora, quo numine laeso,
+-quidve dolens, regina deum tot volvere casus
+-insignem pietate virum, tot adire labores
+-impulerit. Tantaene animis caelestibus irae?
+ 
+"""
+
+EXPECTED_HG_DIFF_2 = """\
+diff --git a/foo.txt b/foo.txt
+--- a/foo.txt
++++ b/foo.txt
+@@ -1,3 +1,5 @@
++ARMA virumque cano, Troiae qui primus ab oris
++ARMA virumque cano, Troiae qui primus ab oris
+ ARMA virumque cano, Troiae qui primus ab oris
+ Italiam, fato profugus, Laviniaque venit
+ litora, multum ille et terris iactatus et alto
+"""
+
+EXPECTED_HG_DIFF_3 = """\
+diff --git a/foo.txt b/foo.txt
+--- a/foo.txt
++++ b/foo.txt
+@@ -6,7 +6,4 @@
+ inferretque deos Latio, genus unde Latinum,
+ Albanique patres, atque altae moenia Romae.
+ Musa, mihi causas memora, quo numine laeso,
+-quidve dolens, regina deum tot volvere casus
+-insignem pietate virum, tot adire labores
+-impulerit. Tantaene animis caelestibus irae?
+ 
+"""
+
+EXPECTED_HG_SVN_DIFF_0 = """\
+Index: foo.txt
+===================================================================
+--- foo.txt\t(revision 4)
++++ foo.txt\t(working copy)
+@@ -1,4 +1,1 @@
+-ARMA virumque cano, Troiae qui primus ab oris
+-ARMA virumque cano, Troiae qui primus ab oris
+-ARMA virumque cano, Troiae qui primus ab oris
+ Italiam, fato profugus, Laviniaque venit
+@@ -6,3 +3,8 @@
+ vi superum saevae memorem Iunonis ob iram;
+-multa quoque et bello passus, dum conderet urbem,
++dum conderet urbem,
++
++
++
++
++
+ inferretque deos Latio, genus unde Latinum,
+"""
+
+EXPECTED_HG_SVN_DIFF_1 = """\
+Index: foo.txt
+===================================================================
+--- foo.txt\t(revision 4)
++++ foo.txt\t(working copy)
+@@ -1,2 +1,1 @@
+-ARMA virumque cano, Troiae qui primus ab oris
+ ARMA virumque cano, Troiae qui primus ab oris
+@@ -6,6 +5,6 @@
+ vi superum saevae memorem Iunonis ob iram;
+-multa quoque et bello passus, dum conderet urbem,
+-inferretque deos Latio, genus unde Latinum,
+-Albanique patres, atque altae moenia Romae.
+-Musa, mihi causas memora, quo numine laeso,
++dum conderet urbem, inferretque deos Latio, genus
++unde Latinum, Albanique patres, atque altae
++moenia Romae. Albanique patres, atque altae
++moenia Romae. Musa, mihi causas memora, quo numine laeso,
+ 
+"""

=== modified file 'setup.cfg'
--- setup.cfg	2010-07-31 18:31:05 +0000
+++ setup.cfg	2011-11-06 07:43:24 +0000
@@ -1,6 +1,7 @@
 [egg_info]
-tag_build = .dev
-tag_svn_revision = 1
+tag_build = 
+tag_svn_revision = 0
+tag_date = 0
 
 [aliases]
 snapshot = egg_info -Dr
@@ -12,3 +13,4 @@
 rc1 = egg_info -DRb rc1
 rc2 = egg_info -DRb rc2
 release = egg_info -DRb ''
+