Merge lp:~dcaro/bzr/fix-1008223 into lp:bzr

[David Caro]

Just changed the file bzrlib/repofmt/pack_repo.py.

Please review the changes and check that it does not break something more.

thanks,
David

[Martin Packman (gz)]

Thanks for the contribution!

Clearly this _parse_index_sizes helper method isn't terribly robust which isn't great. However, the pack-names file should only have space separated integer values, and is already aware of general corruption or truncation thanks being in btree index format. That means for the string "None" to have snuck in, there has to have been a logic bug somewhere in the code passing values in.

It would be great to work out how that happened, but may well not be possible. I've not seen any other reports of this issue, so it's likely not present in the current bzr codebase.

So, I'm not sure it's worthwhile adding logic here, especially without a test validating that it doesn't result in a broken pack reference anyway.

Perhaps a better code change would be:

* Test that passing a list including None to that helper raises that more useful exception.
* Wrap the contents of RepositoryPackCollection._parse_index_sizes in try/except ValueError and re-raise as a BzrError mentioning broken pack-names file. Could be @staticmethod too.

And maybe as well:

* Test that passing a NewPack with bad indexes raises an error.
* In RepositoryPackCollection.allocate assert that every object in a_new_pack.index_sizes is actually an integer.

review: Needs Information

[David Caro (dcaro)]

Hi Martin,

I'll look into it, meanwhile, I've sent you the file you requested on the bug report.

I personally prefer to find out the root of the error rather than just throwing an exception when the error is present (mainly because this error is making my code break :S, so I'd love to get rid of it, maybe I'm doing something wrong, I don't know, let's find how to figure out why).
Do you have any idea of how those 'None' string got in there? Is the RepositoryPackCollection.allocate method the one that generates those values? (I've not looked into it yet, I'm looking for some guidance to get started)

I'll try to find out how those values got there, but a little guiding through the code will be greatly appreciated, ;)

By the way, thanks for the fast and complete response! Even if I can't find out the root of the problem, I'll code the solutions you suggested, they will, at least, warn that the pack-names file is broken.

review: Disapprove

[Martin Packman (gz)]

Right, the allocate method is the entry point for these values. Doesn't solve the mystery though, as the incorrect values would have been set at some earlier point, and doesn't help repair the repo. Have a poke around in bzrlib/repofmt/pack_repo.py and bzrlib/tests/test_repository.py for more.

[Martin Packman (gz)]

I'll mark this as rejected for now, feel free to poke me any time about getting the bzr side more robust.

Unmerged revisions

6525. By David Caro on 2012-06-03

Fix for bug lp-1008223

=== modified file 'bzrlib/repofmt/pack_repo.py'
--- bzrlib/repofmt/pack_repo.py	2011-12-19 19:15:58 +0000
+++ bzrlib/repofmt/pack_repo.py	2012-06-03 20:53:19 +0000
@@ -1097,7 +1097,7 @@
 
     def _parse_index_sizes(self, value):
         """Parse a string of index sizes."""
-        return tuple([int(digits) for digits in value.split(' ')])
+        return tuple([int(digits) for digits in value.replace('None', '0').split(' ')])
 
     def get_pack_by_name(self, name):
         """Get a Pack object by name.