Status: | Rejected |
---|---|
Rejected by: | Jelmer Vernooij |
Proposed branch: | lp:~jelmer/bzr/repo-size |
Merge into: | lp:bzr |
Diff against target: |
136 lines (+55/-1) 7 files modified
NEWS (+3/-0) bzrlib/remote.py (+10/-0) bzrlib/repository.py (+6/-1) bzrlib/smart/repository.py (+13/-0) bzrlib/smart/request.py (+3/-0) bzrlib/tests/per_repository/test_statistics.py (+10/-0) bzrlib/tests/test_smart.py (+10/-0) |
To merge this branch: | bzr merge lp:~jelmer/bzr/repo-size |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Robert Collins (community) | Needs Fixing | ||
Review via email:
|
Commit message
Description of the change
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Jelmer Vernooij (jelmer) wrote : | # |
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Robert Collins (lifeless) wrote : | # |
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.
> https:/
>
>
> 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.
> on len(Repository.
> 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
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Jelmer Vernooij (jelmer) wrote : | # |
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.
> > https:/
> >
> >
> > 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.
> > on len(Repository.
> > 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_
Cheers,
Jelmer
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Robert Collins (lifeless) wrote : | # |
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_
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.
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
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Jelmer Vernooij (jelmer) wrote : | # |
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_
> 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.
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
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Martin Pool (mbp) wrote : | # |
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://
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Robert Collins (lifeless) wrote : | # |
> > 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?
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Jelmer Vernooij (jelmer) wrote : | # |
Well, we don't have .revisions in the case of e.g. subversion, Repository.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Robert Collins (lifeless) wrote : | # |
I'm basically very hesitant to be adding more methods to Repository as a whole. Its overly big (still).
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Jelmer Vernooij (jelmer) wrote : | # |
> 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.
![](/+icing/build/overlay/assets/skins/sam/images/close.gif)
Vincent Ladeuil (vila) wrote : | # |
>>>>> "Jelmer" == Jelmer Vernooij <email address hidden> writes:
Jelmer> Well, we don't have .revisions in the case of e.g. subversion, Repository.
Set it to [] instead ;-p
Vincent
Preview Diff
1 | === modified file 'NEWS' | |||
2 | --- NEWS 2009-12-25 13:33:03 +0000 | |||
3 | +++ NEWS 2009-12-25 15:36:19 +0000 | |||
4 | @@ -63,6 +63,9 @@ | |||
5 | 63 | CamelCase. For the features that were more likely to be used, we added a | 63 | CamelCase. For the features that were more likely to be used, we added a |
6 | 64 | deprecation thunk, but not all. (John Arbash Meinel) | 64 | deprecation thunk, but not all. (John Arbash Meinel) |
7 | 65 | 65 | ||
8 | 66 | * ``Repository.__len__`` will now return the number of revisions in | ||
9 | 67 | the repository. (#123653, Jelmer Vernooij) | ||
10 | 68 | |||
11 | 66 | * ``WorkingTree.update`` implementations must now accept a ``revision`` | 69 | * ``WorkingTree.update`` implementations must now accept a ``revision`` |
12 | 67 | parameter. | 70 | parameter. |
13 | 68 | 71 | ||
14 | 69 | 72 | ||
15 | === modified file 'bzrlib/remote.py' | |||
16 | --- bzrlib/remote.py 2009-12-15 20:32:34 +0000 | |||
17 | +++ bzrlib/remote.py 2009-12-25 15:36:19 +0000 | |||
18 | @@ -886,6 +886,16 @@ | |||
19 | 886 | parents_provider = self._make_parents_provider(other_repository) | 886 | parents_provider = self._make_parents_provider(other_repository) |
20 | 887 | return graph.Graph(parents_provider) | 887 | return graph.Graph(parents_provider) |
21 | 888 | 888 | ||
22 | 889 | def __len__(self): | ||
23 | 890 | """See Repository.__len__.""" | ||
24 | 891 | path = self.bzrdir._path_for_remote_call(self._client) | ||
25 | 892 | response_tuple, response_handler = self._call_expecting_body( | ||
26 | 893 | 'Repository.__len__', path) | ||
27 | 894 | if response_tuple[0] != 'ok': | ||
28 | 895 | raise errors.UnexpectedSmartServerResponse(response_tuple) | ||
29 | 896 | body = response_handler.read_body_bytes() | ||
30 | 897 | return int(body) | ||
31 | 898 | |||
32 | 889 | def gather_stats(self, revid=None, committers=None): | 899 | def gather_stats(self, revid=None, committers=None): |
33 | 890 | """See Repository.gather_stats().""" | 900 | """See Repository.gather_stats().""" |
34 | 891 | path = self.bzrdir._path_for_remote_call(self._client) | 901 | path = self.bzrdir._path_for_remote_call(self._client) |
35 | 892 | 902 | ||
36 | === modified file 'bzrlib/repository.py' | |||
37 | --- bzrlib/repository.py 2009-12-17 10:01:25 +0000 | |||
38 | +++ bzrlib/repository.py 2009-12-25 15:36:19 +0000 | |||
39 | @@ -926,6 +926,11 @@ | |||
40 | 926 | r'.* revision="(?P<revision_id>[^"]+)"' | 926 | r'.* revision="(?P<revision_id>[^"]+)"' |
41 | 927 | ) | 927 | ) |
42 | 928 | 928 | ||
43 | 929 | @needs_read_lock | ||
44 | 930 | def __len__(self): | ||
45 | 931 | """Determine the number of revisions in the repository.""" | ||
46 | 932 | return len(self.revisions.keys()) | ||
47 | 933 | |||
48 | 929 | def abort_write_group(self, suppress_errors=False): | 934 | def abort_write_group(self, suppress_errors=False): |
49 | 930 | """Commit the contents accrued within the current write group. | 935 | """Commit the contents accrued within the current write group. |
50 | 931 | 936 | ||
51 | @@ -1471,7 +1476,7 @@ | |||
52 | 1471 | # XXX: do we want to __define len__() ? | 1476 | # XXX: do we want to __define len__() ? |
53 | 1472 | # Maybe the versionedfiles object should provide a different | 1477 | # Maybe the versionedfiles object should provide a different |
54 | 1473 | # method to get the number of keys. | 1478 | # method to get the number of keys. |
56 | 1474 | result['revisions'] = len(self.revisions.keys()) | 1479 | result['revisions'] = len(self) |
57 | 1475 | # result['size'] = t | 1480 | # result['size'] = t |
58 | 1476 | return result | 1481 | return result |
59 | 1477 | 1482 | ||
60 | 1478 | 1483 | ||
61 | === modified file 'bzrlib/smart/repository.py' | |||
62 | --- bzrlib/smart/repository.py 2009-09-03 00:33:35 +0000 | |||
63 | +++ bzrlib/smart/repository.py 2009-12-25 15:36:19 +0000 | |||
64 | @@ -328,6 +328,19 @@ | |||
65 | 328 | return SuccessfulSmartServerResponse(('no', )) | 328 | return SuccessfulSmartServerResponse(('no', )) |
66 | 329 | 329 | ||
67 | 330 | 330 | ||
68 | 331 | class SmartServerRepositorySize(SmartServerRepositoryRequest): | ||
69 | 332 | |||
70 | 333 | def do_repository_request(self, repository): | ||
71 | 334 | """Return the number of revisions in a repository. | ||
72 | 335 | |||
73 | 336 | :param repository: The repository to query in. | ||
74 | 337 | |||
75 | 338 | :return: A SmartServerResponse ('ok',), a encoded body containing | ||
76 | 339 | the number of revisions. | ||
77 | 340 | """ | ||
78 | 341 | return SuccessfulSmartServerResponse(('ok', ), "%d" % len(repository)) | ||
79 | 342 | |||
80 | 343 | |||
81 | 331 | class SmartServerRepositoryGatherStats(SmartServerRepositoryRequest): | 344 | class SmartServerRepositoryGatherStats(SmartServerRepositoryRequest): |
82 | 332 | 345 | ||
83 | 333 | def do_repository_request(self, repository, revid, committers): | 346 | def do_repository_request(self, repository, revid, committers): |
84 | 334 | 347 | ||
85 | === modified file 'bzrlib/smart/request.py' | |||
86 | --- bzrlib/smart/request.py 2009-12-21 17:00:29 +0000 | |||
87 | +++ bzrlib/smart/request.py 2009-12-25 15:36:19 +0000 | |||
88 | @@ -592,6 +592,9 @@ | |||
89 | 592 | request_handlers.register_lazy('Repository.gather_stats', | 592 | request_handlers.register_lazy('Repository.gather_stats', |
90 | 593 | 'bzrlib.smart.repository', | 593 | 'bzrlib.smart.repository', |
91 | 594 | 'SmartServerRepositoryGatherStats') | 594 | 'SmartServerRepositoryGatherStats') |
92 | 595 | request_handlers.register_lazy('Repository.__len__', | ||
93 | 596 | 'bzrlib.smart.repository', | ||
94 | 597 | 'SmartServerRepositorySize') | ||
95 | 595 | request_handlers.register_lazy('Repository.get_parent_map', | 598 | request_handlers.register_lazy('Repository.get_parent_map', |
96 | 596 | 'bzrlib.smart.repository', | 599 | 'bzrlib.smart.repository', |
97 | 597 | 'SmartServerRepositoryGetParentMap') | 600 | 'SmartServerRepositoryGetParentMap') |
98 | 598 | 601 | ||
99 | === modified file 'bzrlib/tests/per_repository/test_statistics.py' | |||
100 | --- bzrlib/tests/per_repository/test_statistics.py 2009-03-23 14:59:43 +0000 | |||
101 | +++ bzrlib/tests/per_repository/test_statistics.py 2009-12-25 15:36:19 +0000 | |||
102 | @@ -62,3 +62,13 @@ | |||
103 | 62 | 'revisions': 0 | 62 | 'revisions': 0 |
104 | 63 | }, | 63 | }, |
105 | 64 | stats) | 64 | stats) |
106 | 65 | |||
107 | 66 | def test_len(self): | ||
108 | 67 | tree = self.make_branch_and_memory_tree('.') | ||
109 | 68 | self.assertEquals(0, len(tree.branch.repository)) | ||
110 | 69 | tree.lock_write() | ||
111 | 70 | tree.add('') | ||
112 | 71 | rev1 = tree.commit('first post', committer='person 1', | ||
113 | 72 | timestamp=1170491381, timezone=0) | ||
114 | 73 | tree.unlock() | ||
115 | 74 | self.assertEquals(1, len(tree.branch.repository)) | ||
116 | 65 | 75 | ||
117 | === modified file 'bzrlib/tests/test_smart.py' | |||
118 | --- bzrlib/tests/test_smart.py 2009-10-29 00:16:50 +0000 | |||
119 | +++ bzrlib/tests/test_smart.py 2009-12-25 15:36:19 +0000 | |||
120 | @@ -1472,6 +1472,16 @@ | |||
121 | 1472 | rev_id_utf8, 'yes')) | 1472 | rev_id_utf8, 'yes')) |
122 | 1473 | 1473 | ||
123 | 1474 | 1474 | ||
124 | 1475 | class TestSmartServerRepositorySize(tests.TestCaseWithMemoryTransport): | ||
125 | 1476 | |||
126 | 1477 | def test_empty(self): | ||
127 | 1478 | backing = self.get_transport() | ||
128 | 1479 | request = smart.repository.SmartServerRepositorySize(backing) | ||
129 | 1480 | repository = self.make_repository('.') | ||
130 | 1481 | self.assertEqual(SmartServerResponse(('ok', ), "0"), | ||
131 | 1482 | request.execute('')) | ||
132 | 1483 | |||
133 | 1484 | |||
134 | 1475 | class TestSmartServerRepositoryIsShared(tests.TestCaseWithMemoryTransport): | 1485 | class TestSmartServerRepositoryIsShared(tests.TestCaseWithMemoryTransport): |
135 | 1476 | 1486 | ||
136 | 1477 | def test_is_shared(self): | 1487 | def test_is_shared(self): |
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 revisions. Currently Repository. gather_ stats() relies revisions) , so foreign branch plugins have to override
provide Repository.
on len(Repository.
it.
Cheers,
Jelmer