Closed Bug 655942 Opened 12 years ago Closed 11 years ago

[shipping] diff view should be clever for revisions not in the same repo


(Webtools Graveyard :: Elmo, defect, P2)



(Not tracked)



(Reporter: Pike, Assigned: Pike)



(Keywords: dogfood)


(1 file, 1 obsolete file)

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


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.
Blocks: 655943
I'll start hacking on this.
Assignee: nobody → l10n
Attached patch one big patch for queue (obsolete) — Splinter Review
This is a pretty deep patch queue that I'd prefer to land in patches, the better view than this patch is in

Requesting review here either way, but feel free to comment on the queue, too.
Attachment #634963 - Flags: review?(peterbe)
Comment on attachment 634963 [details] [diff] [review]
one big patch for queue

Review of attachment 634963 [details] [diff] [review]:

* The (ab)use of 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/
@@ +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 "" (ie. apps/pushes/tests/

::: apps/pushes/
@@ +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/ 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/
@@ +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/
@@ +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):

    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.
Attachment #634963 - Flags: review?(peterbe) → review-
I've addressed the comments they're distributed among the various patch steps in the queue, notably, the test factor patch has the 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.
Attachment #634963 - Attachment is obsolete: true
Attachment #635259 - Flags: review?(peterbe)
Comment on attachment 635259 [details] [diff] [review]
big patch, with review comments

Review of attachment 635259 [details] [diff] [review]:

Great work!

::: apps/pushes/tests/
@@ +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`
Attachment #635259 - Flags: review?(peterbe) → review+
Commits pushed to develop at
bug 655942, first move to class-based view
bug 655942, factor some worker code of the diff view
bug 655942, factor pushes tests, no functional change
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.
Closed: 11 years ago
Resolution: --- → FIXED
Whiteboard: [good first bug]
Target Milestone: --- → 2.2
Product: Webtools → Webtools Graveyard
You need to log in before you can comment on or make changes to this bug.