get_record_stream(include_delta_closure=???) wrong meaning

[Robert Collins]

On Mon, 2008-06-30 at 13:15 -0500, John Arbash Meinel wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> While trying to figure out how the logic was working I came across this
> flag.
> 
> The docstring says:
> ~        :param include_delta_closure: If True then the closure across any
> ~            compression parents will be included (in the opaque data).
> 
> 
> However, when looking at the implementations and callers, they seem to
> actually mean "as_fulltexts=True".
> 
> Specifically, 'fetch()' passes False, because it knows it only wants the
> deltas.
> 
> The Knit data converters use True, as does iter_files_bytes. And when
> you look at the implementation:
> 
> ~        if include_delta_closure:
> ~            # XXX: get_content_maps performs its own index queries;
> allow state
> ~            # to be passed in.
> ~            text_map, _ = self._get_content_maps(present_keys)
> ~            for key in present_keys:
> ~                yield FulltextContentFactory(key, parent_map[key], None,
> ~                    ''.join(text_map[key]))
> 
> 
> ^- This does return the "delta closure" deltas in the data stream, it
> unpacks everything into fulltexts and returns that.
> 
> Which is okay, considering it is actually what we want, and it unpacks
> everything in a proper order (such that it performs well, doesn't have
> to double extract things, etc.).
> 
> I'm mostly just arguing that the parameter is misleading, and it should
> be something like "return_fulltexts = True" or something else like it.

It could be renamed that yes.

> This may be something that Robert was thinking in terms of a Remote api,
> where it would return the deltas, and then the local abstraction might
> turn them back into fulltexts. But it seems like it would be better to
> request what you actually are asking for.

I don't know that anyone _cares_ to get the knit hunks when they want
fulltexts; but at least in my initial concept it was optional;
implementation is lacking.

I'm happy with renaming it.

-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/20080701/c3f6e428/attachment.pgp