Summary

Support parent diffs with Mercurial

Review Request #5207 — Created Jan. 7, 2014 and submitted Jan. 15, 2014, 11:29 p.m.

Information

Owner

indygreg

Repository

RBTools

Branch

master

Bugs

3067

Depends On

Blocks

5220

Reviewers

Groups

rbtools

People

Description

This patch adds support for parent diffs to Mercurial.

Testing Done

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

Issues

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 …	david	Jan. 15, 2014, 1:26 p.m.
Can you pass the format arguments as additional arguments to logging.*()? Otherwise we do two format operations where only one …	david	Jan. 15, 2014, 1:26 p.m.

Change Summary:

Used the proper revision for parent_base.

Depends On:

-	~~5066 - Calculate outgoing Mercurial changesets from current revision~~

Diff:

Revision 2 (+68 -37)

Show changes

rbtools/clients/mercurial.py

Change Summary:

Rebased on top of mater.

Diff:

Revision 3 (+54 -35)

Show changes

rbtools/clients/mercurial.py

Change Summary:

Updated description.
Anyway, here's the deal with this patch. It works. But it's not very efficient. Under the hood, rbt is performing a lot of hg outgoing and hg parents invocations, especially when parent diffs are involved. When working with the Firefox repository (160000+ commits), the run time of rbt is painfully slow. While this patch could probably land as is (after tests are written, of course), some future optimization will be necessary.
I see two options:
1) Add caching of known changesets, parents, outgoing revisions, etc.

2) Use http://selenic.com/repo/python-hglib to establish a "pipe" to the Mercurial repo so all access is through a single process. The Mercurial process will cache certain lookups, so redundant access should be fast. Furthermore, we'll have fewer process invocations and less string parsing to worry about.
I think #2 is the most robust solution. But that would introduce a new package dependency for RBTools. Not sure if that's acceptable.

Description:

		This is a preliminary patch for supporting parent diffs with Mercurial.

		It's not ready for checkin because it needs tests. But it does appear to work in my setup.
-
-		This patch includes the patch for review #5066. I'll rebase once 5066 lands. I wanted to have a version of this patch available when #5066 is considered so you can see where I'm going with the API changes.

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?

indygreg Jan. 7, 2014, 10:30 p.m.

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.

chipx86 Jan. 8, 2014, 12:55 a.m.

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.

Change Summary:

Wrote some tests. Improved the patch a little. I'm optimistic it's very close to shipping.

Description:

~		This is a preliminary patch for supporting parent diffs with Mercurial.
	~	This patch adds support for parent diffs to Mercurial.
-
-		It's not ready for checkin because it needs tests. But it does appear to work in my setup.

Testing Done:

~		Just local manuael testing so far.
	~	Wrote some new tests. Executed tests locally and everything appears to work as expected!

Diff:

Revision 4 (+108 -35)

Show changes

	rbtools/clients/mercurial.py
	rbtools/clients/tests.py

```
Would this fix issue 3067?
```
1. IN
  
  indygreg Jan. 9, 2014, 3:09 p.m.
  I believe so.

Bugs:

+	3067

```
This is looking pretty great!
```

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

The issue has been dropped. Show all issues

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).

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

The issue has been resolved. Show all issues

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

Depends On:

+	~~5231 - Use DAG range when guessing description~~

Change Summary:

Rebased on top of master. Fixed the issue about logging formatting. Updated some comments.
I think the issue about fixing --tracking-branch and the TODO about parent diffs can be addressed by a followup. This patch is a step in the right direction and I don't think we should let perfection stand in the way of better.

Depends On:

-	~~5231 - Use DAG range when guessing description~~

Diff:

Revision 5 (+115 -39)

Show changes

	rbtools/clients/mercurial.py
	rbtools/clients/tests.py

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

indygreg Jan. 15, 2014, 6:29 a.m.

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+).

david Jan. 15, 2014, 1:58 p.m.

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.

david Jan. 15, 2014, 2:09 p.m.

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

Change Summary:

Using {branch} instead of {branches} (avoids longstanding Mercurial bug).
FWIW, Mercurial only stores one branch per changeset. {branches} is effectively {branch} in list form. We should have been using {branch} all along.

Diff:

Revision 6 (+113 -39)

Show changes

	rbtools/clients/mercurial.py
	rbtools/clients/tests.py

Ship it!

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

Status: Closed (submitted)

Change Summary:

Pushed to master (6bd0cea). Thanks!