[MERGE] Replace Repository.get_data_stream with get_data_stream_for_search
Robert Collins
robertc at robertcollins.net
Thu Jan 17 21:18:12 GMT 2008
On Thu, 2008-01-17 at 13:59 -0600, John Arbash Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Robert Collins wrote:
> > This new API takes a SearchResult rather than a simple list of
> > revisions. Next step - smart server verb changes.
> >
> > -Rob
> >
>
> Is this "for_search" or "from_search"...
I called it for search, because its the stream for the search; buy you
could also call it from search.
>
> - - self._fetch_everything_for_revisions(revs, pp)
> + if getattr(self, '_fetch_everything_for_search', None) is
> not None:
> + self._fetch_everything_for_search(search, pp)
> + else:
> + # backward compatibility
> + self._fetch_everything_for_revisions(search.get_keys, pp)
> finally:
>
> ^- Doesn't this need to be "search.get_keys()" not "search.get_keys" ?
> This would also indicate that the backward compatibility section is not
> being tested.
>
> I wonder if a simple:
>
> fetcher = X()
> del searcher._fetch_everything_for_search
> fetcher.fetch()
>
> would be a way to test it.
Arh yes; I considered not having a backward compatibility section; but I
don't know if plugins have implemented subclasses of Fetcher so I was
being cautious. This won't test it though as I don't supply an
implementation of the old method name (because a subclass without the
new method name must have the old one). I'll fix the typo but this is
into diminishing returns (and as its a subclassing issue we can't even
sensibly look for deprecation warnings on the old method name).
> > === modified file 'bzrlib/remote.py'
> > --- bzrlib/remote.py 2008-01-14 22:48:07 +0000
> > +++ bzrlib/remote.py 2008-01-17 05:30:53 +0000
> > @@ -691,6 +691,30 @@
> > """RemoteRepositories never create working trees by default."""
> > return False
> >
> > + def revision_ids_to_search_result(self, result_set):
> > + """Convert a set of revision ids to a graph SearchResult."""
> > + result_parents = set()
> > + for parents in self.get_graph().get_parent_map(
> > + result_set).itervalues():
> > + result_parents.update(parents)
> > + included_keys = result_set.intersection(result_parents)
> > + start_keys = result_set.difference(included_keys)
> > + exclude_keys = result_parents.difference(result_set)
> > + result = graph.SearchResult(start_keys, exclude_keys,
> > + len(result_set), result_set)
> > + return result
>
>
> ^- This looks oddly familiar. Is it a copy and paste from your other
> location? Is it possible to factor out the duplicate code?
>
> > - def get_data_stream(self, revision_ids):
> > + def get_data_stream_for_search(self, search):
> > + revision_ids = search.get_keys()
>
> ^- don't we need to leave a function "get_data_stream" in the Class and
> just deprecate it?
Both of these comments are for the RemoteRepository class which has IMO
a bug - it is not a subclass of Repository, so has a growing amount of
duplicate code. The former comment is for a function (like a number of
others) duplicated to it; the latter comment is where a function has
been deprecated in Repository; in RemoteRepository there won't be a
get_data_stream anymore. I'd like us to fix this bug in RemoteRepository
before 1.2, which would clean this up nicely; but if you like I can add
a copy of the deprecated version copied from Reposistory to
RemoteRepository.
-Rob
--
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20080118/e0e40e30/attachment.pgp
More information about the bazaar
mailing list