Support parent diffs with Mercurial

Review Request #5207 — Created Jan. 7, 2014 and submitted

indygreg
RBTools
master
3067
5220
rbtools

This patch adds support for parent diffs to Mercurial.

Wrote some new tests. Executed tests locally and everything appears to work as expected!

Description From Last Updated

I think that this needs to be addressed as part of this change. I'd like to use the --tracking-branch argument ...

daviddavid

Can you pass the format arguments as additional arguments to logging.*()? Otherwise we do two format operations where only one ...

daviddavid
IN
IN
IN
chipx86
  1. I'm okay with the dependency if it's pure Python, easy_installable, and supports Python 2.5+. The general issue with dependencies is when they're deprecating versions of Python before we do, causing issues with compatibility.

    As it is, it doesn't seem they have a package we can easily add as a dependency. That's going to be a problem for us.

    I don't have a lot of experience using Mercurial for day-to-day work, or the intricacies of the commands, so I don't have a lot of advice for this. I know for git, we can basically check if a revision is pushed upstream or not, and make decisions based on that.

    For --parent, we just take the revision they give, tell git to generate a diff between the tracking branch (origin/master, for example) and that, call it the parent diff, and then generate a diff between that revision and HEAD. I gather it's just not that straight-forward with Mercurial?

    1. python-hglib is available at https://pypi.python.org/pypi/python-hglib. I /think/ it will work down to Python 2.5+, as the project is maintained by the Mercurial project and Mercurial itself still supports 2.3+ IIRC. But I haven't ran anything other than 2.7 in years. Honestly, I'm not sure why people insist on still supporting anything less than 2.7 since everything up to 2.6 is end-of-lifed and isn't even receiving security updates. But that's for a separate discussion. Even if the hglib package doesn't work right, the Mercurial people are pretty good at responding, especially if things are broken.

      Computing the parent diff is relatively straightforward with Mercurial. You call hg outgoing to compute the changesets missing from the remote and take the parent of the earliest one in the DAG as the base changeset. The problem is that a lot of the RBTools code as it is structured today involves a lot of calling to hg. On large repositories, the overhead is not insignificant. python-hglib will greatly reduce the cost of subsequent lookups because it will communicate over a pipe with a persistent process. It's almost as fast as using the native Python API directly, without the GPL contamination and API compatibility issues.

      I think we can defer optimization to a follow-up. I'll try to get some tests in order so we can land something. Perfect is the enemy of done.

    2. I'd also prefer people to use Python 2.6/2.7 everywhere, but alas.

      Okay, I think this sounds like a good plan then. We'll get this patch in once it has some tests so at least there's something for parent diff support, and then we'll focus on the optimization.

      By the way, I can't remember if I sent this link to you, but David's doing some large-scale work on improving RBTools's handling of revisions to keep things consistent and cleaner. We'd love any feedback you may have on Mercurial. https://reviewboard.hackpad.com/RBTools-Diff-Behavior-gRwNAvl3SzJ. You should be able to add comments inline, or just e-mail us if you prefer, if you have any thoughts.

  2. 
      
IN
BC
  1. Would this fix issue 3067?

  2. 
      
IN
david
  1. This is looking pretty great!

  2. rbtools/clients/mercurial.py (Diff revision 4)
     
     
     
     
     
     

    I think that this needs to be addressed as part of this change.

    I'd like to use the --tracking-branch argument for users to specify their upstream branch (and we should really rename it to --upstream-branch to get away from the very git-specific name).

  3. rbtools/clients/mercurial.py (Diff revision 4)
     
     
     

    Can you pass the format arguments as additional arguments to logging.*()? Otherwise we do two format operations where only one is wanted.

  4. 
      
IN
IN
david
  1. I'm getting several failures when running the tests for MercurialClient after this change. Did you run the entire test suite or just the new ones?

    $ nosetests -v -- rbtools.clients.tests:MercurialClientTests
    
    1. Testing MercurialClient diff, simple case ... ok
      Testing MercurialClient diff with multiple commits ... ok
      Testing MercurialClient get_repository_info, simple case ... ok
      Testing MercurialClient guess summary & description 1 commit. ... ok
      Testing MercurialClient guess summary & description middle commit ... ok
      Testing MercurialClient guess summary & description 3 commits. ... ok
      Testing MercurialClient guess summary & description 2 commits. ... ok
      Testing MercurialClient.parse_revision_spec with no arguments ... ok
      Testing MercurialClient.parse_revision_spec with one revision ... ok
      Testing MercurialClient.parse_revision_spec with r1::r2 syntax ... ok
      Testing MercurialClient.parse_revision_spec with r1..r2 syntax ... ok
      Testing MercurialClient.parse_revision_spec with parent base ... ok
      Testing MercurialClient.parse_revision_spec with two revisions ... ok
      Testing MercurialClient scan_for_server when in .reviewboardrc ... ok
      Testing MercurialClient scan_for_server, simple case ... ok
      Testing MercurialClient scan_for_server when present in hgrc ... ok

      Please send me the error output so I can hopefully track this down. It might be a Mercurial version compatibility issue (I'm running Mercurial 2.8.2+).

    2. OK, looks like it is a version issue. With 2.8, I get a bunch of errors, and with 2.8.2, I don't. I'll send you the verbose output privately but it looks like a bug in the 2.8 version.

    3. One thing we might do is just check that the version is one which will work with this and error out if not.

  2. 
      
IN
david
  1. Looks good. I'm going to make a small change before pushing this to fix hgsubversion.

  2. 
      
IN
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to master (6bd0cea). Thanks!
Loading...