Last Comment Bug 655942 - [shipping] diff view should be clever for revisions not in the same repo
: [shipping] diff view should be clever for revisions not in the same repo
Status: RESOLVED FIXED
: dogfood
Product: Webtools
Classification: Server Software
Component: Elmo (show other bugs)
: Trunk
: All All
P2 normal (vote)
: 2.2
Assigned To: Axel Hecht [:Pike]
:
:
Mentors:
Depends on:
Blocks: 655943
  Show dependency treegraph
 
Reported: 2011-05-09 22:23 PDT by Axel Hecht [:Pike]
Modified: 2012-06-21 13:35 PDT (History)
2 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
one big patch for queue (61.07 KB, patch)
2012-06-20 10:04 PDT, Axel Hecht [:Pike]
peterbe: review-
Details | Diff | Splinter Review
big patch, with review comments (60.24 KB, patch)
2012-06-21 04:56 PDT, Axel Hecht [:Pike]
peterbe: review+
Details | Diff | Splinter Review

Description User image Axel Hecht [:Pike] 2011-05-09 22:23:34 PDT
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.
Comment 1 User image Axel Hecht [:Pike] 2012-06-15 04:13:09 PDT
I'll start hacking on this.
Comment 2 User image Axel Hecht [:Pike] 2012-06-20 10:04:27 PDT
Created attachment 634963 [details] [diff] [review]
one big patch for queue

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 3 User image Peter Bengtsson [:peterbe] 2012-06-20 13:54:49 PDT
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.
Comment 4 User image Axel Hecht [:Pike] 2012-06-21 04:56:29 PDT
Created attachment 635259 [details] [diff] [review]
big patch, with review comments

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 5 User image Peter Bengtsson [:peterbe] 2012-06-21 08:47:38 PDT
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`
Comment 6 User image [github robot] 2012-06-21 10:41:36 PDT
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.
Comment 7 User image Axel Hecht [:Pike] 2012-06-21 13:35:11 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.