[MERGE][bug #300177] Pass compression_parent as part of 'get_record_stream'
John Arbash Meinel
john at arbash-meinel.com
Fri Feb 6 22:00:38 GMT 2009
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Sorry about the very long delay in responding.
Ian Clatworthy wrote:
> John Arbash Meinel wrote:
>> The attached patch does 2 basic things:
>>
>> 1) Changes the api so that the records (ContentFactory objects) from
>> get_record_stream have a member called "compression_parents".
>>
>> 2) Change insert_record_stream to use the compression_parents field, and
>> take appropriate action.
>>
>>
>> I went for the plural form, because it doesn't require a code change if
>> we ever do multi-parent deltas.
>
> I'm no expert at this level so I need a few questions answered before
> I can vote tweak/approve.
>
> bb:comment
>
>> + # if c_parent is not None and c_parent != parents[0]:
>> + # raise AssertionError('Kndx files do not support'
>> + # ' non-left-hand compression parents.')
>
> Did you mean to leave this commented out? Or implement that logic?
I was originally going to add a parameter to "add_record()", however
since we don't actually want to support non-lhs compression parents in
the current repository formats (because of old known-broken clients), it
didn't matter. So I just removed this.
>
>> + # This indexs supports only a single compression parent, and it must be
>> + # one of the direct parents (for now)
>> + if len(compression_parents) > 1 or len(parents) < 1:
>> + return False
>> + # XXX: restore this to allow pack compression parents to be
>> + # non-left-hand
>> + # if compression_parents[0] in parents:
>> + if compression_parents[0] == parents[0]:
>> + return True
>> + return False
>
> Can you add some 'when' information to the XXX comment please?
> From memory, you mentioned that info elsewhere (email or bug report)
> but it's not present in the code anywhere?
Done. I talked about needing a repository format bump, etc.
>
>> + def test_get_and_insert_record_stream(self):
>> + builder = self.make_branch_builder('test',
>> + format=self.repository_format._matchingbzrdir)
>> + builder.start_series()
>> + builder.build_snapshot('A', None, [
>> + ('add', ('', 'TREE_ROOT', 'directory', None)),
>> + ('add', ('file', 'f-id', 'file', 'content A\n'))])
>> + builder.build_snapshot('B', ['A'], [
>> + ('modify', ('f-id', 'content-A\ncontent-B\n'))])
>> + builder.finish_series()
>> + b = builder.get_branch()
>> + self.assertEqual(b.repository._format, self.repository_format)
>
> At a minimum, this test needs a few more comments because
> insert_record_stream and get_record_stream aren't obviously being
> called, let alone being tested. The significance of the last assert
> is lost on me, but that probably just reflects my ignorance. I guess
> plenty of others will be equally ignorant :-) so a different test
> name and/or more comments here please.
>
I think this was just a test that didn't actually get finished. I
believe I was planning on forcing a text into the repository with a
mis-ordered parents, and then assert on the output.
So I went ahead and did this by filtering the object coming out of
"get_record_stream()" to swap its 'self.parents' on the fly, and assert
that the 'insert_record_stream()' handled it correctly.
>> + def test_allowed_compression_parents(self):
>> + index = self.two_graph_index()
>
> The docstring for this particular two_graph_index method has a
> cut and paste bug, explaining a delta parameter that doesn't exist.
> Please tweak that when merging (after final approval).
>
> The rest looks OK to me.
>
> Ian C.
>
I think I've addressed everything you've asked here. So now I just need
an official review from someone. (see attached patch.)
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
iEYEARECAAYFAkmMswYACgkQJdeBCYSNAANfiACfcgcB344RaP4M5AbaOJ3jeP5W
iq4AoMN3QlHdkkOhdSQhiZF38PC3Q2Vt
=HKDt
-----END PGP SIGNATURE-----
-------------- next part --------------
An embedded and charset-unspecified text was scrubbed...
Name: 300177_record_stream_compression_parents.patch
Url: https://lists.ubuntu.com/archives/bazaar/attachments/20090206/6ad32904/attachment-0001.diff
More information about the bazaar
mailing list