Merge lp:~jelmer/bzr/repo-size into lp:bzr

[Jelmer Vernooij]

[Jelmer Vernooij (jelmer)]

This patch adds a custom implementation of Repository.__len__ that
returns the number of revisions in the repository.

This is mainly convenient for foreign branch plugins, which do not
provide Repository.revisions. Currently Repository.gather_stats() relies
on len(Repository.revisions), so foreign branch plugins have to override
it.

Cheers,

Jelmer

[Robert Collins (lifeless)]

On Fri, 2009-12-25 at 15:36 +0000, Jelmer Vernooij wrote:
> Jelmer Vernooij has proposed merging lp:~jelmer/bzr/repo-size into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #123653 "bzr info" depends on private Repository._revision_store
> https://bugs.launchpad.net/bugs/123653
>
>
> This patch adds a custom implementation of Repository.__len__ that
> returns the number of revisions in the repository.
>
> This is mainly convenient for foreign branch plugins, which do not
> provide Repository.revisions. Currently Repository.gather_stats() relies
> on len(Repository.revisions), so foreign branch plugins have to override
> it.

Generally we avoid giving objects that are not dicts or sequences a
__len__, and I think its awfully prone to confusion here too, so we
should not do that. I thought that this had been written down somewhere
- you may wish to briefly look for it in the dev docs and if not found
add a note to this effect: only make things look like containers if they
feel like them too. (that is that you won't be surprised as a user).

Rather, I suggest you put the revision count into the output of
gather_stats: its not guaranteed to be cheap for all repositories, so
there is every reason to make it optional.

 review needs-fixing

review: Needs Fixing

[Jelmer Vernooij (jelmer)]

On Fri, 2009-12-25 at 19:21 +0000, Robert Collins wrote:
> Review: Needs Fixing
> On Fri, 2009-12-25 at 15:36 +0000, Jelmer Vernooij wrote:
> > Jelmer Vernooij has proposed merging lp:~jelmer/bzr/repo-size into lp:bzr.
> >
> > Requested reviews:
> > bzr-core (bzr-core)
> > Related bugs:
> > #123653 "bzr info" depends on private Repository._revision_store
> > https://bugs.launchpad.net/bugs/123653
> >
> >
> > This patch adds a custom implementation of Repository.__len__ that
> > returns the number of revisions in the repository.
> >
> > This is mainly convenient for foreign branch plugins, which do not
> > provide Repository.revisions. Currently Repository.gather_stats() relies
> > on len(Repository.revisions), so foreign branch plugins have to override
> > it.
>
> Generally we avoid giving objects that are not dicts or sequences a
> __len__, and I think its awfully prone to confusion here too, so we
> should not do that. I thought that this had been written down somewhere
> - you may wish to briefly look for it in the dev docs and if not found
> add a note to this effect: only make things look like containers if they
> feel like them too. (that is that you won't be surprised as a user).
>
> Rather, I suggest you put the revision count into the output of
> gather_stats: its not guaranteed to be cheap for all repositories, so
> there is every reason to make it optional.
As I mentioned in my merge request, it's already part of gather_stats()
but I'd like to have it in a separate function so I can override it for
subclasses. Would it be acceptable if I just renamed __len__ to
_get_revision_count() ?

Cheers,

Jelmer

[Robert Collins (lifeless)]

On Sat, 2009-12-26 at 00:51 +0000, Jelmer Vernooij wrote:

> > Generally we avoid giving objects that are not dicts or sequences a
> > __len__, and I think its awfully prone to confusion here too, so we
> > should not do that. I thought that this had been written down somewhere
> > - you may wish to briefly look for it in the dev docs and if not found
> > add a note to this effect: only make things look like containers if they
> > feel like them too. (that is that you won't be surprised as a user).
> >
> > Rather, I suggest you put the revision count into the output of
> > gather_stats: its not guaranteed to be cheap for all repositories, so
> > there is every reason to make it optional.
> As I mentioned in my merge request, it's already part of gather_stats()
> but I'd like to have it in a separate function so I can override it for
> subclasses. Would it be acceptable if I just renamed __len__ to
> _get_revision_count() ?

Or you could just delete printing the number of revisions. I don't think
a method on repository is a particularly good idea; if anything a method
on repository.revisions. However, gather_stats is *intended* to be
overridden, trying to avoid overriding it is odd to me :).

Another alternative would be to make the check for whether to attempt to
calculate the revision count more sophisticated.

-Rob

[Jelmer Vernooij (jelmer)]

Le samedi 26 décembre 2009 à 19:51 +0000, Robert Collins a écrit :
> On Sat, 2009-12-26 at 00:51 +0000, Jelmer Vernooij wrote:
> > > Generally we avoid giving objects that are not dicts or sequences a
> > > __len__, and I think its awfully prone to confusion here too, so we
> > > should not do that. I thought that this had been written down somewhere
> > > - you may wish to briefly look for it in the dev docs and if not found
> > > add a note to this effect: only make things look like containers if they
> > > feel like them too. (that is that you won't be surprised as a user).
> > >
> > > Rather, I suggest you put the revision count into the output of
> > > gather_stats: its not guaranteed to be cheap for all repositories, so
> > > there is every reason to make it optional.
> > As I mentioned in my merge request, it's already part of gather_stats()
> > but I'd like to have it in a separate function so I can override it for
> > subclasses. Would it be acceptable if I just renamed __len__ to
> > _get_revision_count() ?
> Or you could just delete printing the number of revisions. I don't think
> a method on repository is a particularly good idea; if anything a method
> on repository.revisions.
Except the foreign repository implementations don't necessarily
have .revisions set to something sane.

> However, gather_stats is *intended* to be
> overridden, trying to avoid overriding it is odd to me :).
>
> Another alternative would be to make the check for whether to attempt to
> calculate the revision count more sophisticated.
So, the reason I am trying to avoid overriding it is because it is quite
a big function and I would like to avoid duplicating all of that code in
all of the foreign branch plugins.

I think printing the number of revisions in "bzr info" is actually quite
useful, I've used it on several occasions.

Cheers,

Jelmer

[Martin Pool (mbp)]

2009/12/26 Robert Collins <email address hidden>:
> Generally we avoid giving objects that are not dicts or sequences a
> __len__, and I think its awfully prone to confusion here too, so we
> should not do that. I thought that this had been written down somewhere
> - you may wish to briefly look for it in the dev docs and if not found
> add a note to this effect: only make things look like containers if they
> feel like them too. (that is that you won't be surprised as a user).

+1

--
Martin <http://launchpad.net/~mbp/>

[Robert Collins (lifeless)]

> > However, gather_stats is *intended* to be
> > overridden, trying to avoid overriding it is odd to me :).
> >
> > Another alternative would be to make the check for whether to attempt to
> > calculate the revision count more sophisticated.
> So, the reason I am trying to avoid overriding it is because it is quite
> a big function and I would like to avoid duplicating all of that code in
> all of the foreign branch plugins.
>
> I think printing the number of revisions in "bzr info" is actually quite
> useful, I've used it on several occasions.

So, its expensive for you because there isn't a size/len/area style function on .revisions? How about we add one there?

[Jelmer Vernooij (jelmer)]

Well, we don't have .revisions in the case of e.g. subversion, Repository.revisions is just set to None.

[Robert Collins (lifeless)]

I'm basically very hesitant to be adding more methods to Repository as a whole. Its overly big (still).

[Jelmer Vernooij (jelmer)]

> I'm basically very hesitant to be adding more methods to Repository as a
> whole. Its overly big (still).
Darn, that's a good point. I'll look at alternatives.

[Vincent Ladeuil (vila)]

>>>>> "Jelmer" == Jelmer Vernooij <email address hidden> writes:

    Jelmer> Well, we don't have .revisions in the case of e.g. subversion, Repository.revisions is just set to None.

Set it to [] instead ;-p

    Vincent

Unmerged revisions

4926. By Jelmer Vernooij on 2009-12-25

Repository.__len__ will now return the number of revisions in the repository.

=== modified file 'NEWS'
--- NEWS	2009-12-25 13:33:03 +0000
+++ NEWS	2009-12-25 15:36:19 +0000
@@ -63,6 +63,9 @@
   CamelCase. For the features that were more likely to be used, we added a
   deprecation thunk, but not all. (John Arbash Meinel)
 
+* ``Repository.__len__`` will now return the number of revisions in 
+  the repository. (#123653, Jelmer Vernooij)
+
 * ``WorkingTree.update`` implementations must now accept a ``revision``
   parameter.
 

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2009-12-15 20:32:34 +0000
+++ bzrlib/remote.py	2009-12-25 15:36:19 +0000
@@ -886,6 +886,16 @@
         parents_provider = self._make_parents_provider(other_repository)
         return graph.Graph(parents_provider)
 
+    def __len__(self):
+        """See Repository.__len__."""
+        path = self.bzrdir._path_for_remote_call(self._client)
+        response_tuple, response_handler = self._call_expecting_body(
+            'Repository.__len__', path)
+        if response_tuple[0] != 'ok':
+            raise errors.UnexpectedSmartServerResponse(response_tuple)
+        body = response_handler.read_body_bytes()
+        return int(body)
+
     def gather_stats(self, revid=None, committers=None):
         """See Repository.gather_stats()."""
         path = self.bzrdir._path_for_remote_call(self._client)

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2009-12-17 10:01:25 +0000
+++ bzrlib/repository.py	2009-12-25 15:36:19 +0000
@@ -926,6 +926,11 @@
         r'.* revision="(?P<revision_id>[^"]+)"'
         )
 
+    @needs_read_lock
+    def __len__(self):
+        """Determine the number of revisions in the repository."""
+        return len(self.revisions.keys())
+
     def abort_write_group(self, suppress_errors=False):
         """Commit the contents accrued within the current write group.
 
@@ -1471,7 +1476,7 @@
             # XXX: do we want to __define len__() ?
             # Maybe the versionedfiles object should provide a different
             # method to get the number of keys.
-            result['revisions'] = len(self.revisions.keys())
+            result['revisions'] = len(self)
             # result['size'] = t
         return result
 

=== modified file 'bzrlib/smart/repository.py'
--- bzrlib/smart/repository.py	2009-09-03 00:33:35 +0000
+++ bzrlib/smart/repository.py	2009-12-25 15:36:19 +0000
@@ -328,6 +328,19 @@
             return SuccessfulSmartServerResponse(('no', ))
 
 
+class SmartServerRepositorySize(SmartServerRepositoryRequest):
+
+    def do_repository_request(self, repository):
+        """Return the number of revisions in a repository.
+
+        :param repository: The repository to query in.
+
+        :return: A SmartServerResponse ('ok',), a encoded body containing 
+            the number of revisions.
+        """
+        return SuccessfulSmartServerResponse(('ok', ), "%d" % len(repository))
+
+
 class SmartServerRepositoryGatherStats(SmartServerRepositoryRequest):
 
     def do_repository_request(self, repository, revid, committers):

=== modified file 'bzrlib/smart/request.py'
--- bzrlib/smart/request.py	2009-12-21 17:00:29 +0000
+++ bzrlib/smart/request.py	2009-12-25 15:36:19 +0000
@@ -592,6 +592,9 @@
 request_handlers.register_lazy('Repository.gather_stats',
                                'bzrlib.smart.repository',
                                'SmartServerRepositoryGatherStats')
+request_handlers.register_lazy('Repository.__len__',
+                               'bzrlib.smart.repository',
+                               'SmartServerRepositorySize')
 request_handlers.register_lazy('Repository.get_parent_map',
                                'bzrlib.smart.repository',
                                'SmartServerRepositoryGetParentMap')

=== modified file 'bzrlib/tests/per_repository/test_statistics.py'
--- bzrlib/tests/per_repository/test_statistics.py	2009-03-23 14:59:43 +0000
+++ bzrlib/tests/per_repository/test_statistics.py	2009-12-25 15:36:19 +0000
@@ -62,3 +62,13 @@
             'revisions': 0
             },
             stats)
+
+    def test_len(self):
+        tree = self.make_branch_and_memory_tree('.')
+        self.assertEquals(0, len(tree.branch.repository))
+        tree.lock_write()
+        tree.add('')
+        rev1 = tree.commit('first post', committer='person 1',
+            timestamp=1170491381, timezone=0)
+        tree.unlock()
+        self.assertEquals(1, len(tree.branch.repository))

=== modified file 'bzrlib/tests/test_smart.py'
--- bzrlib/tests/test_smart.py	2009-10-29 00:16:50 +0000
+++ bzrlib/tests/test_smart.py	2009-12-25 15:36:19 +0000
@@ -1472,6 +1472,16 @@
                                          rev_id_utf8, 'yes'))
 
 
+class TestSmartServerRepositorySize(tests.TestCaseWithMemoryTransport):
+
+    def test_empty(self):
+        backing = self.get_transport()
+        request = smart.repository.SmartServerRepositorySize(backing)
+        repository = self.make_repository('.')
+        self.assertEqual(SmartServerResponse(('ok', ), "0"),
+                         request.execute(''))
+
+
 class TestSmartServerRepositoryIsShared(tests.TestCaseWithMemoryTransport):
 
     def test_is_shared(self):