filling in features and tests for hg and hg+svn

Review Request #1464 — Created March 8, 2010 and submitted

dbuch-agi
RBTools
rbtools, reviewboard
chipx86, david, durin42, mcrute
We're using ReviewBoard quite a bit now at AG Interactive and are beginning to experiment with Mercurial as our primary SCM/VCS/whatever instead of Subversion.  Initial experiments with `post-review` and pure-Mercurial repos (not hgsubversion clones) have indicated that `post-review` is geared toward use with hgsubversion clones.  This patch seeks to do the following:

    - fixes the assumption that `hg svn info` returning non-error means the working copy is an hgsubversion clone.
    - adds support for reading 'reviewboard.url' option from local hgrc (global hgrc support unimplemented)
    - adds support for posting reviews of locally-committed changes (old behavior only supported posting uncommitted changes)
    - adds support for `--parent` option to specify remote for outgoing diff
    - add tests for both hg and hg+svn client stuff
ran existing tests, wrote and ran new tests (included in patch)
DB
DB
chipx86
  1. 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.
    1. can do! :)
      back with revised diff in a bit...
  2. rbtools/postreview.py (Diff revision 1)
     
     
     
     
     
    New imports should be listed alphabetically.
  3. rbtools/postreview.py (Diff revision 1)
     
     
     
     
     
     
    These should be defined in the constructor, not as part of the class.
  4. rbtools/postreview.py (Diff revision 1)
     
     
     
    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.
  5. rbtools/postreview.py (Diff revision 1)
     
     
     
    Can the call to _load_hgrc just be in the get_repository_info function?
  6. rbtools/postreview.py (Diff revision 1)
     
     
     
    Blank line between these.
  7. rbtools/postreview.py (Diff revision 1)
     
     
     
     
    The "not ..." should all be aligned.
  8. rbtools/postreview.py (Diff revision 1)
     
     
    __calculate_remote_path would be a better name.
  9. rbtools/postreview.py (Diff revision 1)
     
     
     
    Multi-line conditionals should be inside a (...) instead of using \
  10. rbtools/postreview.py (Diff revision 1)
     
     
    Should be in sentence casing.
  11. rbtools/postreview.py (Diff revision 1)
     
     
    Same sort of naming as above.
  12. rbtools/postreview.py (Diff revision 1)
     
     
     
    Blank line here.
  13. rbtools/postreview.py (Diff revision 1)
     
     
     
    Blank line here.
  14. rbtools/postreview.py (Diff revision 1)
     
     
    I prefer more explicit, spelled-out statements, like:
    
    if files is None:
       files = []
    
    Avoid the assignment unless necessary.
  15. rbtools/postreview.py (Diff revision 1)
     
     
    It'd be nice to have a documentation block for this function. Really, same with any new functions we add.
  16. rbtools/postreview.py (Diff revision 1)
     
     
     
    Blank line here.
  17. rbtools/postreview.py (Diff revision 1)
     
     
     
    Blank line.
  18. rbtools/postreview.py (Diff revision 1)
     
     
     
    Can you put the execute line on its own line and store the value? Then iterate over that.
  19. rbtools/postreview.py (Diff revision 1)
     
     
    No ^--. Comments like this should precede the line they reference.
    
    Are branch names with whitespace at all common, or even allowed?
  20. rbtools/postreview.py (Diff revision 1)
     
     
     
    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.
  21. rbtools/postreview.py (Diff revision 1)
     
     
    Please spell this out as:
    
    if branch_name:
       branch_name = ....
    else:
       branch_name = ....
  22. rbtools/postreview.py (Diff revision 1)
     
     
    Sentence casing.
  23. rbtools/postreview.py (Diff revision 1)
     
     
    The % should line up with the string on the previous line.
  24. rbtools/postreview.py (Diff revision 1)
     
     
     
    What's this key=int?
  25. rbtools/postreview.py (Diff revision 1)
     
     
     
    See if this wraps better:
    
    server_url = \
        super(...).scan_for_server(...)
  26. rbtools/postreview.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
    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
  27. rbtools/tests.py (Diff revision 1)
     
     
     
     
     
     
     
     
    Should be inserted alphabetically.
  28. rbtools/tests.py (Diff revision 1)
     
     
    We shouldn't use nose's assertion functions. We should be using the standard unittest's versions.
  29. rbtools/tests.py (Diff revision 1)
     
     
     
     
    Two blank lines between these.
  30. rbtools/tests.py (Diff revision 1)
     
     
     
     
    This blank line should be removed.
    
    Also, this object should inherit from unittest.TestCase.
  31. rbtools/tests.py (Diff revision 1)
     
     
    Tests shouldn't need to operate against a live server, or we're going to hit issues in nightly builds.
  32. rbtools/tests.py (Diff revision 1)
     
     
    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.
  33. rbtools/tests.py (Diff revision 1)
     
     
    We seem to do this in every test. Can we do it from setUp() instead?
  34. rbtools/tests.py (Diff revision 1)
     
     
     
    Please have blank lines around groups of tests.
  35. rbtools/tests.py (Diff revision 1)
     
     
     
    We shouldn't be treating paths with manual string processing. Use the os.path functions, like os.path.basename and os.path.dirname.
  36. rbtools/tests.py (Diff revision 1)
     
     
     
     
    Same with these, and all others in the file.
  37. rbtools/tests.py (Diff revision 1)
     
     
     
    Parameters to rename should be aligned.
  38. rbtools/tests.py (Diff revision 1)
     
     
     
     
     
     
     
     
     
    These are all class-global. They should be defined in setUp(). Same with other classes.
  39. rbtools/tests.py (Diff revision 1)
     
     
    setUp.
  40. rbtools/tests.py (Diff revision 1)
     
     
     
    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)
        
  41. rbtools/tests.py (Diff revision 1)
     
     
     
    Blank line here.
  42. rbtools/tests.py (Diff revision 1)
     
     
     
    Should include the exception information in the SkipTest so people can debug why it's not working.
  43. rbtools/tests.py (Diff revision 1)
     
     
     
    Blank line here.
  44. rbtools/tests.py (Diff revision 1)
     
     
    tearDown.
  45. rbtools/tests.py (Diff revision 1)
     
     
     
     
     
    If this fails, we should know why. If it could fail because the path doesn't exist, we should test for the existence first.
  46. rbtools/tests.py (Diff revision 1)
     
     
     
    Blank line here.
  47. rbtools/tests.py (Diff revision 1)
     
     
     
    Blank line.
  48. rbtools/tests.py (Diff revision 1)
     
     
    This doesn't apply just to this function, but all private functions should start with __.
  49. rbtools/tests.py (Diff revision 1)
     
     
    Should probably be done from setUp, if we always need it.
    
    Also, functions should never start with "get" unless they return a value.
  50. rbtools/tests.py (Diff revision 1)
     
     
     
     
     
     
     
     
    This can be combined.
  51. 
      
DB
  1. only a few points of clarification needed, methinks.
    next draft of diff on its way.
    1. To reply to the original review, use the Add Comment links on the review request page under the comment. Don't reply in the diff viewer. Otherwise it's harder to see the flow of discussions.
    2. got it.   sorry 'bout that.
  2. rbtools/tests.py (Diff revision 1)
     
     
    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
  3. rbtools/tests.py (Diff revision 1)
     
     
    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.
  4. rbtools/tests.py (Diff revision 1)
     
     
    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 :-/
  5. rbtools/tests.py (Diff revision 1)
     
     
     
     
     
     
     
     
    how do you mean, "combined" ? :-/ ... is it an indentation issue?  line wrapping incorrect?
    1. I just mean you can save yourself a variable by:
      
      self._hgcmd(('clone ....').split())
      
      However, I'd really like to not use the template mechanism you have here because that can actually cause problems if there is ever an extra space you're not counting on in the middle of a filename or something. We used to use this sort of mechanism in the past and it broke for some users, so I want to avoid that in the future. We should be representing the entire thing as an array directly without having to split.
    2. got it.
      will revise.
  6. 
      
DB
DB
DB
DB
  1. BUMP
    
    still looking for feedback from latest patch :)
  2. 
      
DB
  1. 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?
    1. Don't worry, we'll get this in soon. I've been swamped with both my day job and trying to get some important changes ready for RB 1.5 beta 2.
    2. much understood!
      thank you for looking :)
  2. 
      
chipx86
  1. A few more things from this pass. I think it's fine after this.
  2. rbtools/postreview.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
    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)
  3. rbtools/postreview.py (Diff revision 3)
     
     
     
     
    Can we break here?
  4. rbtools/postreview.py (Diff revision 3)
     
     
    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?
  5. rbtools/postreview.py (Diff revision 3)
     
     
     
    Can do:
    
    files = files or []
  6. rbtools/postreview.py (Diff revision 3)
     
     
     
    Same here.
  7. rbtools/postreview.py (Diff revision 3)
     
     
    Maybe document this briefly with a comment?
  8. rbtools/postreview.py (Diff revision 3)
     
     
    Can do:
    
    branch_name = branch_name or "default"
  9. rbtools/postreview.py (Diff revision 3)
     
     
     
    Just use the @classmethod decorator on the function directly.
    
    What's the reason for needing a classmethod though?
  10. rbtools/postreview.py (Diff revision 3)
     
     
     
    Can you align this like:
    
    if (not server_url and
        (self.has_hgrc and
         self.....)):
  11. 
      
DB
chipx86
  1. 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?
    1. I'm running against Mercurial 1.5.1.  Should I rework to be compatible with older versions?
    2. Yeah, it's going to be important to be compatible with both, or some users will upgrade and won't be able to use it anymore.
    3. Should I target 1.3.1 or an even earlier version?
    4. BUMP
      
      is 1.3.1 the target of choice? :)
    5. Sorry, thought I responded to this, but I never published the reply.
      
      1.3.1 is probably good, but I'd recommend mailing the reviewboard list and asking other Mercurial users. Not being one, I'm not sure what's generally used in the wild. However, 1.3.1 is definitely a good start for now.
    6. Much thanks!  Question posted to list :)
    7. Okay, so based on mailing list feedback the approach we're taking is already valid -- that shelling out to `hg` and sticking to core commands should be safe with any version from 0.9 to 1.5.1+.  That said, I've purged my system of any mercurial libraries higher than 1.3.1 and I still cannot reproduce the failures listed above.  What to do?
      
      In the meantime, I'm going to implement the changes suggested by durin42.
  2. 
      
DU
  1. 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.
  2. rbtools/postreview.py (Diff revision 4)
     
     
    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).
    1. ah HA.  Yeah, I'll be wanting to fix that.  Much thanks for the heads-up.
  3. rbtools/postreview.py (Diff revision 4)
     
     
    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?
    1. good call.  can do.  I was too hung up on using a record-based format :P
  4. 
      
DB
DU
  1. A couple of minor things, but the over uses of hg seem fine to me.
  2. rbtools/postreview.py (Diff revision 5)
     
     
    Function decorators are available in 2.4, is 2.3 compatibility a concern?
    1. I'm guessing 2.4 is as far back as we care since it seems there were other decorators already in the module.  Chalk this up to my recent experiences developing in 2.3 and 2.6 at the same time ... made me a bit too touchy on the issue.  I'll fix 'em up.
  3. rbtools/postreview.py (Diff revision 5)
     
     
    same decorator question
  4. rbtools/postreview.py (Diff revision 5)
     
     
    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?
    1. Good catch!  I meant to take out the `has_hgrc` checks.
  5. rbtools/tests.py (Diff revision 5)
     
     
    rather than the fragile \ operator, why not do .strip() on the end of the triple-quoted block?
    1. Now you've lost me...   the '\' in this context isn't an operator, but an escape of the leading newline.  I only did it so that the body of CLONE_HGRC wouldn't start with a newline and would all be at the same indentation level rather than:
      
          CLONE_HGRC = """[paths]
          default = %(hg_dir)s
          ...
          """
      
      OR
      
          CLONE_HGRC = \
          """[paths]
          default = %(hg_dir)s
          ...
          """
      
      Out of curiosity, what do you mean when you say that '\' is fragile?
    2. \ does a literal escape of the next character, so if there's a space or something else nonprinting, the newline won't be escaped as you'd expect. It's not line continuation the way it is in C.
      
      I was more suggesting that you do 
      CLONE_HGRC = """
         [paths]
      ...
      """.strip()
      
      which would have the same effect without using the \ syntax.
  6. 
      
DB
DU
  1. 
      
  2. rbtools/postreview.py (Diff revisions 5 - 6)
     
     
    This property appears to be unused now.
  3. 
      
DB
DU
  1. 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.
  2. rbtools/postreview.py (Diff revision 7)
     
     
    unused import
    1. another good catch :)  /me removes
  3. 
      
DB
chipx86
  1. 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
    
  2. 
      
DB
DB
Review request changed

Change Summary:

Running `hg` with env that includes HGRCPATH=/dev/null and HGPLAIN=1 in every case possible to avoid the unexpected (per suggestion from durin42)

Diff:

Revision 10 (+793 -123)

Show changes

DB
  1. 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
  2. 
      
DU
  1. Looks reasonable enough to me from Mercurial use perspective.
  2. 
      
chipx86
  1. Thanks Dan. Really sorry about the wait on these. They just fell off my radar.
    
    Committed to master (9996339).
  2. 
      
RI
  1. 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.
  2. rbtools/postreview.py (Diff revision 10)
     
     
    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.
  3. 
      
Loading...