new 'bugs' property of revision breaks bundle compatibility with old bzr

[Alexander Belchenko]

Alexander Belchenko пишет:
> > Alexander Belchenko ?8H5B:
>> >> When I try to pull this bundle[1] I get error:
> >
>> >> Inventory sha hash mismatch for revision <email address hidden>.
>> >> 430cf9661d64e943bc4a6e333ee67a4fa7a454d1 != e18233b25d8b2a8a98f92a759329b0323aceccc9
>> >> bzr: ERROR: Testament did not match expected value.
>> >> For revision_id {<email address hidden>}, expected
>> >> {5e770fb6b106d00dcc92f89317ddf47c26ee51be}, measured
>> >> {0d6e80b020b551c26c62499af3ff98d645747ddc}
> >
>> >> The bundle created with bzr 0.14 + my gzipped_bundle plugin. Any ideas why it fails?
>> >> I regularly update my mirror of bzr.dev and create bundle to update my home computer,
>> >> and before it always works OK.
> >
> > History continue. I copy my patch with trivial fix for HACKING on my server and try to pull
> > it to my http://bzrdev.bialix.com/trivial branch. I got similar error:
> >
> > bzr: ERROR: Testament did not match expected value.
> > For revision_id {<email address hidden>}, expected
> > {b459fa9ba096e5f571d0cb52c0e6f1540e8c016f}, measured
> > {4cfab1f4864945b4cf161ecd0c874254bc17c7f3}
> >
> > As I see old bzr and new bzr don't understand bundles of each other.
> >
> > It's the regression for me. Can we fix it before 0.16 out?

Probably I found the root of the bug.
If you look at the end of bundle with patch for hacking you could see:

# properties:
# bugs:
# branch-nick: trivial

The 'bugs' appears in revno.2446, I believe.
Revno.2445 does not have such property.
I uncommit my changes and commit it again with bzr.dev.revno.2445.
Then I did new bundle and it pulled with bzr.0.14 without problem.

It seems that support for bugs in commit breaks compatibility
in bundles with old bzr.

[John A Meinel (jameinel)]

Alexander Belchenko wrote:
> Public bug reported:
>
> Alexander Belchenko пишет:
>>> Alexander Belchenko ?8H5B:
>>>>> When I try to pull this bundle[1] I get error:
>>>>> Inventory sha hash mismatch for revision <email address hidden>.
>>>>> 430cf9661d64e943bc4a6e333ee67a4fa7a454d1 != e18233b25d8b2a8a98f92a759329b0323aceccc9
>>>>> bzr: ERROR: Testament did not match expected value.
>>>>> For revision_id {<email address hidden>}, expected
>>>>> {5e770fb6b106d00dcc92f89317ddf47c26ee51be}, measured
>>>>> {0d6e80b020b551c26c62499af3ff98d645747ddc}
>>>>> The bundle created with bzr 0.14 + my gzipped_bundle plugin. Any ideas why it fails?
>>>>> I regularly update my mirror of bzr.dev and create bundle to update my home computer,
>>>>> and before it always works OK.
>>> History continue. I copy my patch with trivial fix for HACKING on my server and try to pull
>>> it to my http://bzrdev.bialix.com/trivial branch. I got similar error:
>>>
>>> bzr: ERROR: Testament did not match expected value.
>>> For revision_id {<email address hidden>}, expected
>>> {b459fa9ba096e5f571d0cb52c0e6f1540e8c016f}, measured
>>> {4cfab1f4864945b4cf161ecd0c874254bc17c7f3}
>>>
>>> As I see old bzr and new bzr don't understand bundles of each other.
>>>
>>> It's the regression for me. Can we fix it before 0.16 out?
>
> Probably I found the root of the bug.
> If you look at the end of bundle with patch for hacking you could see:
>
> # properties:
> # bugs:
> # branch-nick: trivial
>
> The 'bugs' appears in revno.2446, I believe.
> Revno.2445 does not have such property.
> I uncommit my changes and commit it again with bzr.dev.revno.2445.
> Then I did new bundle and it pulled with bzr.0.14 without problem.
>
> It seems that support for bugs in commit breaks compatibility
> in bundles with old bzr.

Well, older clients don't understand the 'bugs:' revision property.

I think the real answer is that we shouldn't be creating a 'bugs:'
property unless there are actual bugs mentioned. ie, if the string is
empty, it should not be set. I wish I would have caught that earlier.

Right now we are now creating:
<property name="bugs" />

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py 2007-04-23 07:50:15 +0000
+++ bzrlib/builtins.py 2007-04-24 14:23:10 +0000
@@ -2157,7 +2157,10 @@
             # selected-file merge commit is not done yet
             selected_list = []

- properties['bugs'] = self._get_bug_fix_properties(fixes,
tree.branch)
+ bug_property = self._get_bug_fix_properties(fixes, tree.branch)
+ if bug_property:
+ # Only add the property if we have actual bug fixes
+ properties['bugs'] = bug_property

         if local and not tree.branch.get_bound_location():
             raise errors.LocalRequiresBoundBranch()

John
=:->

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Alexander Belchenko wrote:
> Public bug reported:
>
> Probably I found the root of the bug.
> If you look at the end of bundle with patch for hacking you could see:
>
> # properties:
> # bugs:
> # branch-nick: trivial
>
> The 'bugs' appears in revno.2446, I believe.
> Revno.2445 does not have such property.
> I uncommit my changes and commit it again with bzr.dev.revno.2445.
> Then I did new bundle and it pulled with bzr.0.14 without problem.

My guess is that this exposes a latent bug in bundles. It looks as if
some code is treating properties with empty values as if the property
does not exist.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGLi260F+nu1YWqI0RAhrRAJ4kmobKhocxkO7WS24grVK9sjC8EQCeI4Ge
QBN18VQppzLIDTMpglWxcdg=
=GzpP
-----END PGP SIGNATURE-----

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John A Meinel wrote:
> Alexander Belchenko wrote:
>> It seems that support for bugs in commit breaks compatibility
>> in bundles with old bzr.
>
> Well, older clients don't understand the 'bugs:' revision property.

Older clients don't understand what a 'bugs:' revision-property means,
but they shouldn't need to understand that in order to roundtrip the
property accurately.

> I think the real answer is that we shouldn't be creating a 'bugs:'
> property unless there are actual bugs mentioned.

I disagree. I think that may be a fine short-term solution, but the
real bug is the way the bundles code handles empty properties.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFGLm9J0F+nu1YWqI0RAq02AJ4oXd/zTq8dvNTbv7lN7FIYpWQsTwCdE5XT
gTkbaxpWj61r5Rp8WGby0Sk=
=FI1p
-----END PGP SIGNATURE-----

[Martin Pool (mbp)]

I think we should add John's fix to not make empty bug properties, and also fix bundles.

[Ian Clatworthy (ian-clatworthy)]

I agree with Aaron - we need to be able to round-trip properties whether they are empty or not. Having an empty property must be supported. It may not make sense for bugs but it will in other cases.

Breaking compatibility is a terrible thing so I'm generally firmly against it. I'd rather do that though than rely on trailing whitespace as raised as an option on the mailing list.

[John A Meinel (jameinel)]

The bug was that empty properties were being parsed incorrectly. 'bugs:\n' was being parsed as:
'bugs' => 'ugs:'
rather than
'bugs' => ''

The associated branch has a fix for the reader, and a controversial fix to the writer to make older bzr versions able to read them. (It requires adding significant trailing whitespace)