[MERGE][bug #300177] Pass compression_parent as part of 'get_record_stream'

[John Arbash Meinel]

-----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