Thanks for the change! I'm happy to see these improvements made.

I know that this is a long review, but the first ones are usually long, especially when it's a big change. This is mostly stylistic stuff that I want to see made so that the new code is consistent.

New imports should be listed alphabetically.

These should be defined in the constructor, not as part of the class.

Any constants should be ALL_UPPERCASE, with no leading _.

The comment should be aligned with the variable name, not the contents. Also, please describe why the order counts, and the purpose for the constant.

Can the call to _load_hgrc just be in the get_repository_info function?

Blank line between these.

The "not ..." should all be aligned.

__calculate_remote_path would be a better name.

Multi-line conditionals should be inside a (...) instead of using \

Should be in sentence casing.

Same sort of naming as above.

Blank line here.

Blank line here.

I prefer more explicit, spelled-out statements, like:

if files is None:
   files = []

Avoid the assignment unless necessary.

It'd be nice to have a documentation block for this function. Really, same with any new functions we add.

Blank line here.

Blank line.

Can you put the execute line on its own line and store the value? Then iterate over that.

No ^--. Comments like this should precede the line they reference.

Are branch names with whitespace at all common, or even allowed?

Can you add a comment explaining what this does for future maintainers? It's a slightly complex filter if you haven't dealt with ifilter and don't know the input exactly.

Please spell this out as:

if branch_name:
   branch_name = ....
else:
   branch_name = ....

Sentence casing.

The % should line up with the string on the previous line.

What's this key=int?

See if this wraps better:

server_url = \
    super(...).scan_for_server(...)

I don't like doing the 'if server_url' each time. Please do:

if (not server_url and
    self....):
    server_url = ...

if not server_url and self._type == 'svn':
    server_url = SVNClient().....

return server_url

Should be inserted alphabetically.

We shouldn't use nose's assertion functions. We should be using the standard unittest's versions.

Two blank lines between these.

This blank line should be removed.

Also, this object should inherit from unittest.TestCase.

Tests shouldn't need to operate against a live server, or we're going to hit issues in nightly builds.

Test functions should all be in testCamelCase style, and should have a one-line docstring explaining what's being tested, in the form of: """Test MercurialClient get_repository_info"""

I know not all tests in this file do it. We need to fix this. It's inconsistent with all the other unit tests in Review Board.

We seem to do this in every test. Can we do it from setUp() instead?

Please have blank lines around groups of tests.

We shouldn't be treating paths with manual string processing. Use the os.path functions, like os.path.basename and os.path.dirname.

Same with these, and all others in the file.

Parameters to rename should be aligned.

These are all class-global. They should be defined in setUp(). Same with other classes.

setUp.

I'm not a fan of any(). It's less explicit. Please do:

for exe in ["svnadmin", "svnserve", "svn"]:
    if not is_exe_in_path(exe):
        raise nose.SkipTest('Cannot test Mercurial Subversion integration: Missing %s" % exe)
    

Blank line here.

Should include the exception information in the SkipTest so people can debug why it's not working.

Blank line here.

tearDown.

If this fails, we should know why. If it could fail because the path doesn't exist, we should test for the existence first.

Blank line here.

Blank line.

This doesn't apply just to this function, but all private functions should start with __.

Should probably be done from setUp, if we always need it.

Also, functions should never start with "get" unless they return a value.

This can be combined.

only a few points of clarification needed, methinks.
next draft of diff on its way.

fwiw, nose.tools.assert* are just aliases to unittest.TestCase.assert* with PEP8 names, but I'm going to switch to unittest.TestCase anyway for style continuity

We (at AGI) tend to prefer no docstrings for tests so that the fully qualified test name shows in verbose nosetests output, but I'll add docstrings for style continuity.

but ... it's *private*, not *protected*.  I have always operated under the impression that using leading __ was BAD BAD BAD unless *absolutely* *necessary* because of the nonsensical name mangling that's done as a result :-/

how do you mean, "combined" ? :-/ ... is it an indentation issue?  line wrapping incorrect?

BUMP

still looking for feedback from latest patch :)

BUMP again

Until this is reviewed (and accepted?) the folks at my workplace will have to continue installing RBTools from my fork on github instead of just letting pip do its job :-/
Pretty please?

A few more things from this pass. I think it's fine after this.

We create a RepositoryInfo several times here. Maybe do:


path = self.hg_root
base_path = '/'

if self.has_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)

Can we break here?

We never seem to actually do anything with this variable. Can we just call self.hgrc.read() without a return value? Or do we need this line at all?

Can do:

files = files or []

Same here.

Maybe document this briefly with a comment?

Can do:

branch_name = branch_name or "default"

Just use the @classmethod decorator on the function directly.

What's the reason for needing a classmethod though?

Can you align this like:

if (not server_url and
    (self.has_hgrc and
     self.....)):

This looks fine, but I'm getting three unit test failures. Specifically:

FAIL: Test MercurialClient diff with diverged branch
FAIL: Test MercurialClient diff, simple case
FAIL: Test MercurialClient diff with multiple commits

I have Mercurial 1.3.1. Are you seeing this at all? What version are you running?

Hi, someone found me in the #mercurial channel and I figured I'd chime in with my 2¢ (I wrote the hgsubversion bits back when I was at my previous company and used svn).

In general, I'd encourage you to take a look at 'hg showconfig' as that should let you safely parse the paths the same way hg would, including any potential include statements in the hgrc file. If you want to suppress any paths from the user's global ~/.hgrc (although I'm not sure you should suppress those), you can set the environment variable HGRCPATH to /dev/null in hg's environment.

You might be happier using 'hg showconfig' or something similar to extract the variables you really need. We support things in Mercurial's config file parser that may not work in ConfigParser (we don't use ConfigParser internally).

Why not just do a template like this:
b:{branches}\n
r:{rev}\n\n

and then split revisions on the double newline and the rest on single newlines?

A couple of minor things, but the over uses of hg seem fine to me.

Function decorators are available in 2.4, is 2.3 compatibility a concern?

same decorator question

I'm not sure about this logic - what if the repo doesn't have an hgrc but the user specified a reviewboard.url in their global hgrc?

rather than the fragile \ operator, why not do .strip() on the end of the triple-quoted block?

This property appears to be unused now.

LGTM, only one minor comment. I'll leave it to the reviewboard guys to review further since it's really their code, and I'm just a passer-by.

unused import

I don't know why, but I'm still hitting unit test failures. Notice the diff headers.


======================================================================
FAIL: Test MercurialClient diff with diverged branch
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/chipx86/src/rb/rbtools/rbtools/tests.py", line 532, in testDiffBranchDiverge
    self.assertEqual((EXPECTED_HG_DIFF_2, None), self.client.diff(None))
AssertionError: ('diff --git a/foo.txt b/foo.txt\n--- a/foo.txt\n+++ b/foo.txt\n@@ -1,3 +1,5 @@\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', None) != ('diff -r be86bd1cc883 -r d869bd392b11 foo.txt\n--- a/foo.txt\tFri May 21 01:28:59 2010 -0700\n+++ b/foo.txt\tFri May 21 01:28:59 2010 -0700\n@@ -1,3 +1,5 @@\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', None)
-------------------- >> begin captured stdout << ---------------------
>>> hg --cwd /tmp/tmpSRDOk8__rbtools_tests init
>>> hg add foo.txt
>>> hg commit -m "initial commit"
>>> hg clone /tmp/tmpSRDOk8__rbtools_tests /tmp/tmpy8HeN3__rbtools_tests
>>> hg showconfig
>>> hg root
>>> hg svn info
>>> Using candidate path 'default': '/tmp/tmpSRDOk8__rbtools_tests'
>>> repository info: Path: /tmp/tmpSRDOk8__rbtools_tests, Base path: , Supports changesets: False
>>> hg add foo.txt
>>> hg commit -m "commit 1"
>>> hg branch diverged
>>> hg add foo.txt
>>> hg commit -m "commit 2"
>>> hg showconfig
>>> hg svn info
>>> repository info: Path: /tmp/tmpSRDOk8__rbtools_tests, Base path: , Supports changesets: False
>>> hg branch
>>> hg -q outgoing --template b:{branches}
r:{rev}

 default
>>> Found outgoing changeset 2 for branch 'diverged'
>>> hg diff -r 1 -r 2

--------------------- >> end captured stdout << ----------------------

======================================================================
FAIL: Test MercurialClient diff, simple case
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/chipx86/src/rb/rbtools/rbtools/tests.py", line 510, in testDiffSimple
    self.assertEqual((EXPECTED_HG_DIFF_0, None), diff_result)
AssertionError: ('diff --git a/foo.txt b/foo.txt\n--- a/foo.txt\n+++ b/foo.txt\n@@ -6,7 +6,4 @@\n inferretque deos Latio, genus unde Latinum,\n Albanique patres, atque altae moenia Romae.\n Musa, mihi causas memora, quo numine laeso,\n-quidve dolens, regina deum tot volvere casus\n-insignem pietate virum, tot adire labores\n-impulerit. Tantaene animis caelestibus irae?\n \n', None) != ('diff -r 47bb0a09fdfc -r 48b4dcabcef4 foo.txt\n--- a/foo.txt\tFri May 21 01:29:00 2010 -0700\n+++ b/foo.txt\tFri May 21 01:29:01 2010 -0700\n@@ -6,7 +6,4 @@\n inferretque deos Latio, genus unde Latinum,\n Albanique patres, atque altae moenia Romae.\n Musa, mihi causas memora, quo numine laeso,\n-quidve dolens, regina deum tot volvere casus\n-insignem pietate virum, tot adire labores\n-impulerit. Tantaene animis caelestibus irae?\n \n', None)
-------------------- >> begin captured stdout << ---------------------
>>> hg --cwd /tmp/tmpI7q20N__rbtools_tests init
>>> hg add foo.txt
>>> hg commit -m "initial commit"
>>> hg clone /tmp/tmpI7q20N__rbtools_tests /tmp/tmpWVGDkR__rbtools_tests
>>> hg showconfig
>>> hg root
>>> hg svn info
>>> Using candidate path 'default': '/tmp/tmpI7q20N__rbtools_tests'
>>> repository info: Path: /tmp/tmpI7q20N__rbtools_tests, Base path: , Supports changesets: False
>>> hg showconfig
>>> hg svn info
>>> repository info: Path: /tmp/tmpI7q20N__rbtools_tests, Base path: , Supports changesets: False
>>> hg add foo.txt
>>> hg commit -m "delete and modify stuff"
>>> hg branch
>>> hg -q outgoing --template b:{branches}
r:{rev}

 default
>>> Found outgoing changeset 1 for branch 'default'
>>> hg diff -r 0 -r 1

--------------------- >> end captured stdout << ----------------------

======================================================================
FAIL: Test MercurialClient diff with multiple commits
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/chipx86/src/rb/rbtools/rbtools/tests.py", line 522, in testDiffSimpleMultiple
    self.assertEqual((EXPECTED_HG_DIFF_1, None), diff_result)
AssertionError: ('diff --git a/foo.txt b/foo.txt\n--- a/foo.txt\n+++ b/foo.txt\n@@ -1,12 +1,11 @@\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-multa quoque et bello passus, dum conderet urbem,\n+dum conderet urbem,\n inferretque deos Latio, genus unde Latinum,\n Albanique patres, atque altae moenia Romae.\n+Albanique patres, atque altae moenia Romae.\n Musa, mihi causas memora, quo numine laeso,\n-quidve dolens, regina deum tot volvere casus\n-insignem pietate virum, tot adire labores\n-impulerit. Tantaene animis caelestibus irae?\n \n', None) != ('diff -r 821c9698ff13 -r 53714ccbbd24 foo.txt\n--- a/foo.txt\tFri May 21 01:29:02 2010 -0700\n+++ b/foo.txt\tFri May 21 01:29:04 2010 -0700\n@@ -1,12 +1,11 @@\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-multa quoque et bello passus, dum conderet urbem,\n+dum conderet urbem,\n inferretque deos Latio, genus unde Latinum,\n Albanique patres, atque altae moenia Romae.\n+Albanique patres, atque altae moenia Romae.\n Musa, mihi causas memora, quo numine laeso,\n-quidve dolens, regina deum tot volvere casus\n-insignem pietate virum, tot adire labores\n-impulerit. Tantaene animis caelestibus irae?\n \n', None)
-------------------- >> begin captured stdout << ---------------------
>>> hg --cwd /tmp/tmp5Okq3z__rbtools_tests init
>>> hg add foo.txt
>>> hg commit -m "initial commit"
>>> hg clone /tmp/tmp5Okq3z__rbtools_tests /tmp/tmpfwL8Nz__rbtools_tests
>>> hg showconfig
>>> hg root
>>> hg svn info
>>> Using candidate path 'default': '/tmp/tmp5Okq3z__rbtools_tests'
>>> repository info: Path: /tmp/tmp5Okq3z__rbtools_tests, Base path: , Supports changesets: False
>>> hg showconfig
>>> hg svn info
>>> repository info: Path: /tmp/tmp5Okq3z__rbtools_tests, Base path: , Supports changesets: False
>>> hg add foo.txt
>>> hg commit -m "commit 1"
>>> hg add foo.txt
>>> hg commit -m "commit 2"
>>> hg add foo.txt
>>> hg commit -m "commit 3"
>>> hg branch
>>> hg -q outgoing --template b:{branches}
r:{rev}

 default
>>> Found outgoing changeset 1 for branch 'default'
>>> Found outgoing changeset 2 for branch 'default'
>>> Found outgoing changeset 3 for branch 'default'
>>> hg diff -r 0 -r 3

BUMP

I just had to spend yet-another-5-minutes explaining to a veteran developer that `post-review` will not play nice with our Mercurial setup, showing them to my forked version on github.  Not cool.  Pretty please review and *at least* give me a Ship It or wontfix or whatever so I know whether or not I should be officially forking RBTools :P

Looks reasonable enough to me from Mercurial use perspective.

Thanks Dan. Really sorry about the wait on these. They just fell off my radar.

Committed to master (9996339).

It's good that Mercurial support is being worked on, but at the company I work for, we have been doing reviews based on checked-in changesets for ages, using --revision-range. I guess I haven't wrapped my head around the new philosophy of doing this yet.

This is not a good idea, because it takes away the ability to say: --revision-range=tip~1:tip (using the parentrevspec extension). I guess HGPLAIN alone should suffice.