[MERGE] Replace Repository.get_data_stream with get_data_stream_for_search

[Robert Collins]

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