Code review comment for lp:~spiv/bzr/fetch-spec-everything-not-in-other

Martin Pool wrote:
> Review: Needs Information
> I think I like the concept of splitting these out into something more like
> objects.
>
> I don't want to hold this hostage but it seems like this would be a good
> moment to add a little developer documentation to the architectural guide
> describing at a high level how fetch works.

Good idea. I'll see what I can write up without spending ages on it.

> + def get_network_struct(self):
> + start_keys = ' '.join(self._recipe[1])
> + stop_keys = ' '.join(self._recipe[2])
> + count = str(self._recipe[3])
> + return (self._recipe[0], '\n'.join((start_keys, stop_keys, count)))
> +
>
> rather than adhoc encoding, could you send it as bencode?

For this struct: no, because this is how existing clients encode SearchResult in
get_stream requests. (And get_parent_map requests, but that's handled
separately in remote.py. See RemoteRepository._serialise_search_recipe.)

> + medium._remember_remote_is_before((2, 3))
>
> out of scope for this, but concluding it's older than 2.3 because it doesn't
> support this particular operation is really not accurate.

Yeah. But that is definitely out of scope :)

> + """See InterRepository.searcH_missing_revision_ids()."""
>
> typo.

Fixed, thanks.

> + def test_fetch_everything_backwards_compat(self):
> + """Can fetch with EverythingResult even when the server does not have
> + the Repository.get_stream_2.3 verb.
> + """
> + local = self.make_branch('local')
> + builder = self.make_branch_builder('remote')
> + builder.build_commit(message="Commit.")
> + remote_branch_url = self.smart_server.get_url() + 'remote'
> + remote_branch = bzrdir.BzrDir.open(remote_branch_url).open_branch()
> + self.hpss_calls = []
> + local.repository.fetch(remote_branch.repository,
> + fetch_spec=graph.EverythingResult(remote_branch.repository))
> +
>
> I don't understand how this tests that the server doesn't have this verb.

Oops, out-of-date docstring and test. I didn't need a new verb in the end, I
just added more search types to the existing get_stream_1.19 verb. I'll fix
that: instead I should be testing that a BadSearch reply to the new search
triggers the old requests (i.e. VFS). Probably that's a bit too hard to mock
(because it involves VFS) so I might have to monkeypatch or add a knob to the
get_stream handler class...

[...]
> spiv said on irc that he's testing this; if it turns out to be faster in
> practice that should be mentioned too. But I guess this patch alone won't
> change performance.

I don't expect it to improve performance (unless compared to fetching all the
tags by hand with the existing bzr CLI!) either.

The concern jam had is that querying for all the tags on every update and other
operations might have a noticeable negative impact on branches with many tags
such as emacs. My guess and preliminary results are that this isn't a problem,
but I'll make sure before landing this.

-Andrew.