The diff view currently enforces that the two revisions are in the same repository, which makes reviewing in our new branching scheme hard. It's kinda easy to fix, as we have the data in the database, by finding the common ancestor with a code snippet like cs=Changeset.objects.order_by('-pk').filter(repositories__name__endswith='aurora/'+loc).filter(repositories__name__endswith='2.0/'+loc)[0] You have to find the repos for the changesets really first, the line above is just what I use right now, hard-coded to 2.0 vs aurora. I'd like to see this soon.

I'll start hacking on this.

This is a pretty deep patch queue that I'd prefer to land in patches, the better view than this patch is in https://github.com/Pike/elmo/compare/develop...feature%2Fbug-655942-multi-repo-diff Requesting review here either way, but feel free to comment on the queue, too.

Comment on attachment 634963 [details] [diff] [review] one big patch for queue Review of attachment 634963 [details] [diff] [review]: ----------------------------------------------------------------- * The (ab)use of __init__.py is bad practice in my point of view. * The RepoMixin class is not a mixin since it aligns itself in as an inheritable class and should thus be renamed. * The repeated dbrepo() method needs to go. Great work otherwise!! I'm not going deep into the understanding of some of the inner works of the tests but it looks promising all around. ::: apps/pushes/tests/__init__.py @@ +1,1 @@ > +# This Source Code Form is subject to the terms of the Mozilla Public I disagree about this file name. Importing stuff from a package's __init__ is error-prone and confusing. It's error-prone because it can lead to circular imports. I suggest you create a new file called "base.py" (ie. apps/pushes/tests/base.py) ::: apps/pushes/tests.py @@ +24,3 @@ > > > +class RepoMixin(TestCase): Excellent! But is it really a mixin? Looks to me like it's acting as a base class. I suggest you move this into tests/base.py and call it something like RepoTestBase. @@ +44,5 @@ > + if name is None: > + name = self.repo_name > + repo = Repository.objects.create( > + name=name, > + url=urlpattern % name Smart! I like it! @@ +71,4 @@ > if self._old_repository_base is not None: > settings.REPOSITORY_BASE = self._old_repository_base > > + def dbrepo(self, name=None, changesets_from=None, I know splinter review is a bit messed up when you do a rename AND modifications. However, looking at the code in git, it appears this method is repeated. The one in the RepoMixin looks identical. Can you refactor to DRY? ::: apps/pushes/tests/test_pushes.py @@ +6,5 @@ > +import shutil > +import tempfile > +import datetime > +import time > +try: Why bother? json comes builtin in python 2.6. Just use that. I think simplejson might be slightly faster (or something like that) which is not applicable in test suites. @@ +26,5 @@ > +from pushes import repo_fixtures > +from pushes.utils import get_or_create_changeset > +from pushes.views.api import jsonify > +from pushes.utils import handlePushes, PushJS > +from . import mock_ui Again, I would strongly recommend we use `from .base import mock_ui` instead ::: apps/pushes/views/diff.py @@ +34,5 @@ > + > + > +class DiffView(View): > + > + def _universal_newlines(self, content): You should prefix this with staticmethod. Since it's not related to the class from an OO point-of-view but it belongs from a structural point of view. Ie. ``` class DiffView(View): @staticmethod def _universal_newlines(content): ... def get(self, request): content = self._universal_newlines(request.content) ``` Or else, there's no reason for it to accept the `self` if it's not going to use it.

I've addressed the comments they're distributed among the various patch steps in the queue, notably, the test factor patch has the base.py change, and I also used the RepoTestBase in test_pushes now. Left the "copy of DiffTestCase" because that test uses the self.repo_name, self.repo, while the other test uses the network from the fixture code. Also, the updated patch queue is pushed to my clone, the compare url is the same as before.

Comment on attachment 635259 [details] [diff] [review] big patch, with review comments Review of attachment 635259 [details] [diff] [review]: ----------------------------------------------------------------- Great work! ::: apps/pushes/tests/base.py @@ +8,5 @@ > +from django.conf import settings > +from mercurial.ui import ui as hg_ui > +from test_utils import TestCase > +from life.models import Repository > +from ..utils import get_or_create_changeset `pushes` is a package on the path so you can do `from pushes.utils import get_or_create_changeset`

Commits pushed to develop at https://github.com/mozilla/elmo https://github.com/mozilla/elmo/commit/9e3a35ace5ab57c51f716466781d59ba4023d98f bug 655942, first move to class-based view https://github.com/mozilla/elmo/commit/8b336057cee05d38e3137b5ad830a7d705a21aef bug 655942, factor some worker code of the diff view https://github.com/mozilla/elmo/commit/d9371ab4b38fbfeb3355a9ce52708121b95852e8 bug 655942, factor pushes tests, no functional change https://github.com/mozilla/elmo/commit/56b2e2b6cf9df3c520ac3735f3814306799dfd77 bug 655942, support diffs across repos, r=peterbe There are extensive tests to make sure that the routines to piece together the paths and copies from different repos match what hg would return, re-using hg internal code as possible.

Marking FIXED. I've ended up leaving the ..utils in, but we'll not start doing that going forward, neither I not peter are strongly in favor of that.